Subject: Re: [PATCH v4 01/14] t: teach test_cmp_rev to accept ! for not-equals

From: Junio C Hamano <[email-masked]>
Date: Fri, 08 Nov 2019 21:49:02 +0900

Denton Liu <[email-masked]> writes:

>> >  		local r1 r2
>> >  		r1=$(git rev-parse --verify "$1") &&
>> >  		r2=$(git rev-parse --verify "$2") &&
>> 
>> If either of the calls fail, the assignment itself would fail, and
>> the &&-cascade would stop without executing the if statment below.
>> 
>> I see the "!" feature, but where is the promised "fix" for
>> segfaulting rev-parse?
>> 
>> Puzzled.
>
> I suppose your puzzlement comes from my badly worded commit message
> above. I meant to say that in the _hypothetical_ case that 
> `git rev-parse` segfaults, it wouldn't be caught because we're
> blanket-ignoring failures if we do `! test_cmp_rev`.
>
> But I suppose I focused too much on segfaults. I guess I didn't realise
> that the problem is more general than that; any failure of 
> `git rev-parse` should be reported.

But if that is the case, shouldn't the part that runs two rev-parse
read more like this?

	r1=$(git rev-parse --verify "$1") ||
		error "'$1' does not name a valid object"
	r2=$(git rev-parse --verify "$2") ||
		error "'$2' does not name a valid object"
	if ! test "$r1" $op "$r2"
	then
		... they do not compare the same ...
	fi

Offhand I do not know if the current callers depend on being able to
pass a string that is not an object name in either $1 or $2 and a
valid object name in the other one, and relying on the helper
function to say "$1 and $2 are different!"  If such callers exist, a
defensive change like the above that requires the caller to always
pass valid object names would need to be accompanied with changes to
these callers, too.  Overall, I think that would give us a better
end result, but it might be a bit more work.

Thanks.