require_work_tree broken with NONGIT_OK
PATCH 1/1 require_work_tree broken with NONGIT_OK
When sourcing git-sh-setup after having set NONGIT_OK, calling the function require_work_tree while outside of a git repository shows a syntax error.
This is caused by the call to "git rev-parse --is-inside-work-tree" printing a sentence when it is called outside of a git repository. Relying on the return code is better.
--- git-sh-setup.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
git-sh-setup.sh +1 -1
@@ -128,7 +128,7 @@ cd_to_toplevel () { } require_work_tree () { - test $(git rev-parse --is-inside-work-tree) = true || + test git rev-parse --is-inside-work-tree >/dev/null 2>&1 || die "fatal: $0 cannot be used without a working tree." } -- 1.6.6
Quoted 7 lines · Gabriel Filion
When sourcing git-sh-setup after having set
NONGIT_OK, calling the functionrequire_work_treewhile outside of a git repository shows a syntax error.This is caused by the call to "git rev-parse --is-inside-work-tree" printing a sentence when it is called outside of a git repository. Relying on the return code is better.
I think your fix is fine, but your analysis is slightly wrong. If we are not in a work tree, the sentence goes to stderr, and nothing goes to stdout (which is what makes "test" unhappy).
This is not just a nitpick of your commit message, but I was worried given some other recent discussion that we were accidentally sending that message to stdout, which would be a bug. But we're not.
-Peff
git-sh-setup.sh cd_to_toplevel () { · line 130
Quoted 8 lines · Gabriel Filion
git-sh-setup.sh +0
-1@@ -128,7 +128,7 @@ cd_to_toplevel () { } require_work_tree () { - test $(git rev-parse --is-inside-work-tree) = true ||
This needs to have dq around it, as "Not a git repository" case we fatal out without any output, like this:
test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
git-sh-setup.sh cd_to_toplevel () { · line 130
Quoted 1 line
git-sh-setup.sh +1
-0+ test git rev-parse --is-inside-work-tree >/dev/null 2>&1 ||
I don't think this would ever work with "test" at the beginning.
Quoted 1 line
git-sh-setup.sh +0
-0die "fatal: $0 cannot be used without a working tree."
Quoted 3 lines · Junio C Hamano
git-sh-setup.sh cd_to_toplevel () { · line 130
Quoted 1 line
git-sh-setup.sh +1
-0+ test git rev-parse --is-inside-work-tree >/dev/null 2>&1 ||I don't think this would ever work with "test" at the beginning.
Oops. I totally missed that when reviewing the patch. :-/
Thinking on this a bit more, I think Gabriel's script is a little broken. It sets NONGIT_OK to not have a git repository, but then it requires a working tree, which doesn't make any sense.
That being said, I think it is still a good change, as the correct error message is better than the shell barfing.
-Peff
Quoted 4 lines · Jeff King
Thinking on this a bit more, I think Gabriel's script is a little broken. It sets
NONGIT_OKto not have a git repository, but then it requires a working tree, which doesn't make any sense.
I hit this bug while working on a script called git-bzr over at
http://github.com/kfish/git-bzr when trying to use git-sh-setup to avoid reinventing the wheel.
Most commands in there need to be run inside a git repository and some don't. I was trying find out how to implemnt "git bzr clone", which for obvious reasons should not require a work tree..
plus, I thought requiring presence in a work tree to display help messages was not a very user-friendly concept (this was git-bzr's behaviour some days ago).
I'm thinking of changing things to use git-sh-setup by splitting the script into non-work-tree-requiring commands in the main script and commands requiring a work tree in another sub-script.
Well, all this to simply illustrate possible use cases.
I'll surely be opening another discussion about git-bzr pretty soon to see if people would be interested in helping out.
thanks again to both of you.
Signature · 1 line
Gabriel Filion
Quoted 14 lines · Junio C Hamano
git-sh-setup.sh cd_to_toplevel () { · line 130
Quoted 8 lines · Gabriel Filion
git-sh-setup.sh +0
-1@@ -128,7 +128,7 @@ cd_to_toplevel () { } require_work_tree () { - test $(git rev-parse --is-inside-work-tree) = true ||This needs to have dq around it, as "Not a git repository" case we fatal out without any output, like this:
Quoted 6 lines
test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||git-sh-setup.sh cd_to_toplevel () { · line 130
Quoted 1 line
git-sh-setup.sh +1
-0+ test git rev-parse --is-inside-work-tree >/dev/null 2>&1 ||I don't think this would ever work with "test" at the beginning.
Well, it would seem you are right! my bad for thinking that "test" was actually evaluating any command given in the expression :\
Your implementation seems to be working better and fixes the problem. Thanks for reviewing the patch.
Signature · 1 line
Gabriel Filion