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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:

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

>> +	# 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@xxxxxx>

OK, thanks.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]