[PATCH] require_work_tree broken with NONGIT_OK

[PATCH] require_work_tree broken with NONGIT_OK

[Gabriel Filion]

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

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

Re: [PATCH] require_work_tree broken with NONGIT_OK

[Jeff King]

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

> 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

Re: [PATCH] require_work_tree broken with NONGIT_OK

[Junio C Hamano]

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.

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

Re: [PATCH] require_work_tree broken with NONGIT_OK

[Gabriel Filion]

On 2010-02-15 01:38, Junio C Hamano wrote:
> 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

Re: [PATCH] require_work_tree broken with NONGIT_OK

[Jeff King]

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

Re: [PATCH] require_work_tree broken with NONGIT_OK

[Gabriel Filion]

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