Safe invoke of git command #45

mhoyer · 2016-08-03T12:52:51Z

Using exec() without any check of returned exit code makes the whole node process hang. A more real-world scenario is simply trying to run guppy in a directory that is not part of a git repository:

mkdir tmp
cd tmp
npm install guppy-cli
./node_modules/.bin/guppy pre-commit
# ... hangs

I introduced findGitRoot() helper to substitude the two exec() calls with a more advanced approach also checking the returned .code property.

Hence, this seems to be more an issue with shelljs?

Fixes #39

therealklanni · 2016-08-04T18:48:44Z

index.js

+  var gitRevParseTopLevel = exec('git rev-parse --show-toplevel');
+  if (gitRevParseTopLevel.code !== 0) {
+    console.error('guppy-cli needs a git repository to work with.');
+    return exit(1);


Since this is the lib meant to be consumed by other packages (including the bin/index.js), we should return an error to expose it to the caller, and let the caller handle the error (you can exit(1) there), rather than doing it here.

Sorry for late response. Please check my latest commit. Does it now work out for you?

therealklanni · 2016-08-04T18:50:12Z

Thanks for the contribution! Just had one remark, if you can update your code accordingly, I should be able to merge this in soon thereafter. 👍

…of exit

therealklanni · 2016-10-13T07:08:34Z

bin/index.js

@@ -2,7 +2,6 @@
 /* global error */
 'use strict';

-require('shelljs/global');


Why was this removed?

IIRC, the call of exec() in this file was the only need for this requirement. As I kicked those calls shelljs/global is not needed anymore IMHO. Hence, it was code cleaning. If it's needed somewhere else, let's bring it back in.

Sidenote: My personal opinion on this library isn't quite positive at all as it pollutes the global scope and it becomes harder to trace where such global functions like exec actually come from. But that's just a matter of taste. ;-)

Gotcha, thanks for clarifying!

I'm open to other solutions that don't pollute the global namespace. Feel free to create another PR if you'd like to see that changed.

I'll wait for your response before merging this, since it seems this PR may be obsoleted by removing shelljs.

Marcel Hoyer added 2 commits August 3, 2016 11:28

add safe findGitRoot() helper for therealklanni#39

9ec15f9

fix therealklanni#39 replacing unsafe call of exec() with findGitRoot()

636657d

mhoyer changed the title ~~Safe invoke of git command~~ Safe invoke of git command - fixes #39 Aug 3, 2016

therealklanni reviewed Aug 4, 2016
View reviewed changes

therealklanni changed the title ~~Safe invoke of git command - fixes #39~~ Safe invoke of git command Aug 4, 2016

therealklanni self-assigned this Aug 4, 2016

improve therealklanni#39 makes findGitRoot() return an error instead …

b542886

…of exit

therealklanni reviewed Oct 13, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe invoke of git command #45

Safe invoke of git command #45

mhoyer commented Aug 3, 2016 •

edited by therealklanni

Loading

therealklanni Aug 4, 2016

mhoyer Oct 8, 2016

therealklanni commented Aug 4, 2016

therealklanni Oct 13, 2016

mhoyer Oct 13, 2016

therealklanni Oct 18, 2016

therealklanni Oct 18, 2016

therealklanni Oct 18, 2016

Safe invoke of git command #45

Are you sure you want to change the base?

Safe invoke of git command #45

Conversation

mhoyer commented Aug 3, 2016 • edited by therealklanni Loading

therealklanni Aug 4, 2016

Choose a reason for hiding this comment

mhoyer Oct 8, 2016

Choose a reason for hiding this comment

therealklanni commented Aug 4, 2016

therealklanni Oct 13, 2016

Choose a reason for hiding this comment

mhoyer Oct 13, 2016

Choose a reason for hiding this comment

therealklanni Oct 18, 2016

Choose a reason for hiding this comment

therealklanni Oct 18, 2016

Choose a reason for hiding this comment

therealklanni Oct 18, 2016

Choose a reason for hiding this comment

mhoyer commented Aug 3, 2016 •

edited by therealklanni

Loading