threads / patch / 22659

patchrequire_work_tree broken with NONGIT_OK

Subject: [PATCH] require_work_tree broken with NONGIT_OK

## tl;dr

6 messages between Feb 15, 2010 and Feb 15, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Gabriel Filion· Feb 15, 2010, 03:51 UTC · lore

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(-)

Show changes to git-sh-setup.sh +1 −1

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index d56426d..8de2f03 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -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

Jeff King· Feb 15, 2010, 05:34 UTC · re: Gabriel Filion · lore

Re: [PATCH] require_work_tree broken with NONGIT_OK

On Sun, Feb 14, 2010 at 10:51:47PM -0500, Gabriel Filion wrote:

Show 7 quoted lines

> 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.

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

Junio C Hamano· Feb 15, 2010, 06:38 UTC · re: Gabriel Filion · lore

Re: [PATCH] require_work_tree broken with NONGIT_OK

Gabriel Filion <lelutin@gmail.com> writes:

Show 8 quoted lines

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index d56426d..8de2f03 100755
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -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 ||

> +	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.

>  	die "fatal: $0 cannot be used without a working tree."

Gabriel Filion· Feb 15, 2010, 07:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] require_work_tree broken with NONGIT_OK

On 2010-02-15 01:38, Junio C Hamano wrote:

Show 20 quoted lines

> Gabriel Filion <lelutin@gmail.com> writes:
> 
>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
>> index d56426d..8de2f03 100755
>> --- a/git-sh-setup.sh
>> +++ b/git-sh-setup.sh
>> @@ -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 ||
> 
>> +	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.

-- 
Gabriel Filion

Jeff King· Feb 15, 2010, 07:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] require_work_tree broken with NONGIT_OK

On Sun, Feb 14, 2010 at 10:38:30PM -0800, Junio C Hamano wrote:

> > +	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

Gabriel Filion· Feb 15, 2010, 15:14 UTC · re: Jeff King · lore

Re: [PATCH] require_work_tree broken with NONGIT_OK

On 2010-02-15 02:49, Jeff King wrote:

> 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.
>

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.

-- 
Gabriel Filion