Re: [PATCH] difftool: fix bug when printing usage

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 6, 2017, 18:13 UTC
Message-ID: <xmqqinon2nnt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1702061716120.3496@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 12 quoted lines

>> +test_expect_success 'basic usage requires no repo' '
>> +	lines=$(git difftool -h | grep ^usage: | wc -l) &&
>> +	test "$lines" -eq 1 &&
>
> It may be easier to debug future breakages if you wrote
>
> 	git difftool -h | grep ^usage: >output &&
> 	test_line_count = 1 output &&
> or even better (changing the semantics now):
>
> 	test_expect_code 129 git difftool -h >output &&
> 	grep ^usage: output &&

True.

Show 13 quoted lines

>> +	# create a ceiling directory to prevent Git from finding a repo
>> +	mkdir -p not/repo &&
>> +	ceiling="$PWD/not" &&
>> +	lines=$(cd not/repo &&
>> +		GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
>> +		grep ^usage: | wc -l) &&
>> +	test "$lines" -eq 1 &&
>
> Likewise, this would become
>
> 	GIT_CEILING_DIRECTORIES="$PWD/not" \
> 	test_expect_code 129 git -C not/repo difftool -h >output &&
> 	grep ^usage: output

I agree with the intent, but the execution here is "Not quite". test_expect_code being a shell function, it does not take the "one-shot environment assignment for this single invocation," like external commands do.

> More importantly: When I read $PWD all kinds of alarm bells go off in my
> head, as I immediately think of all the issues we have on Windows due to
> Git's regression test using POSIX paths all over the place.

And we appreciate that somebody who is more familiar with the issue is watching ;-).

> Insofar as I am the author of the builtin difftool:
>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

OK, thanks.