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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 8, 2019, 12:49 UTC
Message-ID: <xmqqa796mt35.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191108082310.GA2497@generichostname>

Denton Liu <liu.denton@gmail.com> writes:

Show 20 quoted lines

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