np doesn't actually check for unpulled changes (before showing version selection in interactive mode)

[janpio]

Description

The readme states:

Ensures the working directory is clean and that there are no unpulled changes

When trying np for the first time on a random repo I had checked out, this did not happen though:

E:\Projects\Cordova\cordova-plugin-splashscreen (master -> origin) (cordova-plugin-splashscreen@5.0.3-dev)                 
λ np                                                                                                                       
                                                                                                                           
Publish a new version of cordova-plugin-splashscreen (current: 5.0.3-dev)                                                  
                                                                                                                           
Commits:                                                                                                                   
- CI: Use universal paramedic travis.yml (#210)  6800de2                                                                   
...                 
- Fixed docs typo  5f41762                                                                                                 
                                                                                                                           
Commit Range:                                                                                                              
5.0.2...master                                                                                                             
                                                                                                                           
? Select semver increment or specify new version (Use arrow keys)                                                          
> patch         5.0.3                                                                                                      
  minor         5.1.0                                                                                                      
  major         6.0.0                                                                                                      
  prepatch      5.0.4-0                                                                                                    
  preminor      5.1.0-0                                                                                                    
  premajor      6.0.0-0                                                                                                    
  prerelease    5.0.3-dev.0                                                                                                
  ──────────────                                                                                                           
  Other (specify)                                                                                                          

As

E:\Projects\Cordova\cordova-plugin-splashscreen (master -> origin) (cordova-plugin-splashscreen@5.0.3-dev)
λ git status
On branch master
Your branch is behind 'origin/master' by 4 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

nothing to commit, working tree clean

(4 unpulled changes) both the current version 5.0.3-dev and the last tag 5.0.2 used to get the commits were wrong.

Running git pull and git fetch --tags manually fixed the problem.

Expected behavior

I expected np to also check for unpulled changes (and unfetched tags possibly) and exiting before offering the version selection with wrong information to me.

Environment

np - 5.0.2
Node.js - 10.15.1
npm - 6.4.1
Git - 2.21.0.windows.1
OS - Windows 10

[itaisteinherz]

np will only check for unpulled changes after you select the version you'd like to publish, as part of the prerequisite tasks. Can you wait for those to run and see if you then get an error reported about the unpulled commits?

If you still don't get an error reported, try running git rev-list --count --left-only @{u}...HEAD, and comment the output printed by the command.

[janpio]

I now understand how it works (and indeed, selecting a version then runs into the failing check), but it wasn't what I was expected. This false start of showing a wrong version and using a wrong tag to display the commits could be avoided if the "is it up to date" check would be done before the version selection is displayed.

[itaisteinherz]

This isn't really a bug, but I agree that currently np's increment selection UI is a bit confusing in this case.
Since we shouldn't remove the unpulled changes check from the prerequisite tasks (as that needs to run even if the user has provided the next version to publish), I think that there are two ways in which we could solve this (though feel free to suggest anything else you come up with):

1. Check if there are any unpulled commits before printing the commit history, and if there are some print an error and exit.
2. While printing the commit history, in case there are any unpulled commits, print a warning/error next to the history saying that there are currently unpulled commits np can't account for, and so the user should pull those before continuing.

However both of these solutions feel a bit hacky to me.

[janpio]

I know almost nothing about how np works internally, but as from how you describe it the unpulled changes check always has to pass before publishing (correct?), I think 1) is the better choice here. It allows reusing the same code and error message here without creating (too much) additional logic.

And yep, having the same check run twice in interactive mode feels a bit hacky. But I don't really have an alternative, other than maybe moving all the checks before the increment selection UI - but that probably doesn't make sense because of other reasons (like some of the checks actually requiring the target version to work maybe?).

(While we are at it thinking about this: Are there any other checks that might be useful before showing the increment selection UI?)

[itaisteinherz]

the unpulled changes check always has to pass before publishing (correct?)

Yes

maybe moving all the checks before the increment selection UI - but that probably doesn't make sense because of other reasons (like some of the checks actually requiring the target version to work maybe?).

Yeah, that's not what we're aiming for (and also the Check for pre-release version check requires the published version).

(While we are at it thinking about this: Are there any other checks that might be useful before showing the increment selection UI?)

Not that I can think of.

[itaisteinherz]

In case you're interested, feel free to create a PR with what we discussed above and we'll proceed to getting this landed in np ✨

[janpio]

On it.

[sindresorhus]

For anyone that wants to work on this, see the feedback in #415.

[janpio]

Explicit links to stuff I could not take care of, which caused the PR to stall:

• missing tests: Exit if there are unpulled commits before interactive UI #415 (comment)
• another check requested: Exit if there are unpulled commits before interactive UI #415 (comment)
• remove the check from somewhere else: Exit if there are unpulled commits before interactive UI #415 (comment)

<3

[dopecodez]

For anyone trying to pick this up, read through the comments in #562

I'll give a quick summary of what was decided in #562 .

• The precheck should not slow down the UI too much as per this comment . Tasks like check remote history could slow down the UI considerably because it involves a network call
• We can still go forward and move the local checks like verifyWorkingTreeIsClean() or verifyRemoteHistoryIsClean() to run before UI is displayed, but ideally it should run under the hood, and only show an error in case its thrown.
• The rest of the implementations can continue after Use Ink for the initial UI #570 is implemented.