From: René Scharfe <l.s.r@web.de>
Date: Mon, 30 May 2016 20:55:35 GMT
Subject: Re: [PATCH v2 1/8] t4051: rewrite, add more tests
Message-ID: <574CA8C7.8070807@web.de>
In-Reply-To: <xmqqinxwpgfn.fsf@gitster.mtv.corp.google.com>

Am 30.05.2016 um 01:55 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> +commit_and_tag () {
>> +	message=$1 &&
>> +	shift &&
>> +	git add $@ &&
>
> Lack of dq around $@ makes me wonder if there is something funny
> going on (looking at the callers, there isn't, so we'd better quote
> it to avoid wasting time, I think).

OK.

>> +	test_tick &&
>> +	git commit -m $message &&
>> +	git tag $message
>>   }
>
> The use of $message as the sole argument to "git tag" makes the
> readers guess that it must be a single token without any funny
> character, so the readers would probably do not waste too much time
> wondreing if the lack of dq around $message in the last two is
> problematic.

Well, let's call it $tag; $message is a bit misleading here.  The saved 
letters can be invested in quotes. ;)

>> +last_context_line () {
>> +	sed -n '$ p'
>>   }
>
> I have a vague recollection that some implementations of sed are
> unhappy to see that space between the address and the operation; I'd
> feel safer without it.

Indeed most sed calls in t/ have no space there (found counter-examples 
only in annotate-tests.sh, t4201-shortlog.sh, t9824-git-p4-git-lfs.sh).

>> +check_diff () {
>> +	name=$1
>> +	desc=$2
>> +	options="-W $3"
>> +
>> +	test_expect_success "$desc" '
>> +		git diff $options "$name^" "$name" >"$name.diff"
>> +	'
>> +
>> +	test_expect_success ' diff applies' '
>> +		test_when_finished "git reset --hard" &&
>> +		git checkout --detach "$name^" &&
>
> With the presence of ^ there, --detach is unnecessary; it would not
> hurt, though.

Right.  It's just there to make that intent clear.

>> +		git apply "$name.diff" &&
>> +		git diff --exit-code "$name"
>
> Even though we may know that $name.diff" will never have a creation
> of new paths, I'd feel safer if "apply" is run with "--index".

Makes sense; the less we assume about the diff to be checked the better.

Thanks a lot!

René