Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

From: Kyle Meyer <kyle@kyleam.com>
Date: Feb 17, 2017, 23:40 UTC
Message-ID: <8737fcqtcj.fsf@kyleam.com>
In-Reply-To: <20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

[...]

Show 23 quoted lines

>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index b0ffc0b57..65918d984 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>>  '
>>  rm -f .git/$m
>>  
>> +test_expect_success "deleting current branch adds message to HEAD's log" '
>> +	git update-ref $m $A &&
>> +	git symbolic-ref HEAD $m &&
>> +	git update-ref -mdelmsg -d $m &&
>> +	! test -f .git/$m &&
>> +	grep "delmsg$" .git/logs/HEAD >/dev/null
>> +'
>> +rm -f .git/$m
>
> I think covering this with a test is good.
>
> I don't know if it's also worth testing that deleting via HEAD also
> writes the reflog. I.e.,:
>
>   git update-ref -m delete-by-head -d HEAD

Seems reasonable to cover this case as well.

> Some of the style here is a bit out-dated, but I think you are just
> matching the surrounding tests.  So that's OK by me (though a patch to
> modernize the whole thing would be welcome, too).

Right. I'd be happy to follow up with a patch updating the style in t1400-update-ref.sh.

Show 6 quoted lines

> For reference, the two things I notice are:
>
>   - we prefer test_path_is_missing to "! test -f" these days.
>
>   - we don't redirect the output of grep (it's handled already in
>     non-verbose mode, and in verbose mode we try to be...verbose).

Would moving cleanup like "rm -f .git/$m" within the test's body using test_when_finished also be preferred?

-- 
Kyle