Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

npm version: handle checkGit errors #6991

Closed
wants to merge 1 commit into from
Closed

Conversation

rlidwka
Copy link
Contributor

@rlidwka rlidwka commented Dec 22, 2014

We seem to have a regression in cb6ff8d. When npm checks whether git working directory is empty and errors out, this error gets ignored. Setup:

alex@y:/tmp$ mkdir test
alex@y:/tmp$ cd test
alex@y:/tmp/test$ git init
Initialized empty Git repository in /tmp/test/.git/
alex@y:/tmp/test$ echo '{"name":"foo","version":"1.2.3"}' > package.json
alex@y:/tmp/test$ git add package.json ; git commit -m init
[master (root-commit) 81af888] init
 1 file changed, 1 insertion(+)
 create mode 100644 package.json
alex@y:/tmp/test$ echo '{"name":"foox","version":"1.2.3"}' > package.json
alex@y:/tmp/test$ git status
On branch master
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

    modified:   package.json

no changes added to commit (use "git add" and/or "git commit -a")

Expected (version unchanged, npm errors out):

alex@y:/tmp/test$ ../node_modules/.bin/npm -v
2.1.0
alex@y:/tmp/test$ ../node_modules/.bin/npm version patch
npm ERR! Linux 3.16.0-25-generic
npm ERR! argv "node" "/tmp/test/node_modules/.bin/npm" "version" "patch"
npm ERR! node v0.10.25
npm ERR! npm  v2.1.0

npm ERR! Git working directory not clean.
npm ERR! M package.json
npm ERR! 
npm ERR! If you need help, you may report this error at:
npm ERR!     <http://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /tmp/test/npm-debug.log

Actual (version bumped, no new git commits):

alex@y:/tmp/test$ ../node_modules/.bin/npm -v
2.1.14
alex@y:/tmp/test$ ../node_modules/.bin/npm version patch
v1.2.4
alex@y:/tmp/test$ git log
commit 81af88874960245e70fe709cfd03faabbf252a74
Author: Alex Kocharin <alex@kocharin.ru>
Date:   Tue Dec 23 02:06:58 2014 +0300

    init

Ping @othiym23.

@othiym23
Copy link
Contributor

This is a nice, quick fix – thank you! – but the fact that the test wasn't failing before makes me want to see if we can get a test added around this to prevent this kind of regression in the future. If you can put it together, that's awesome, otherwise I'll get to it in a few weeks most likely.

@rlidwka
Copy link
Contributor Author

rlidwka commented Dec 22, 2014

I added a test similar to version-no-tags.js.

Not sure if such a minor issue is worth creating a new git repository and slowing down test execution time even further.

@othiym23
Copy link
Contributor

Landed in 1c491e6 with a few lil tweaks to make sure the test didn't modify npm's own package.json by mistake if the test for failure wasn't there. Thanks for noticing and fixing this!

Not sure if such a minor issue is worth creating a new git repository and slowing down test execution time even further.

The tests that use git are all fast, compared to many of the others, and we're probably going to rewrite almost all of the tests over the next few months, so we can rationalize everything then.

@othiym23 othiym23 closed this Dec 23, 2014
@othiym23 othiym23 removed the review label Dec 23, 2014
@othiym23 othiym23 added this to the 2.2.0 milestone Dec 23, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants