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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2019, 06:58 UTC
Message-ID: <xmqq8sookyj7.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191108211909.GA80210@generichostname>

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

Show 24 quoted lines

> Hi Junio,
>
> On Fri, Nov 08, 2019 at 09:49:02PM +0900, Junio C Hamano wrote:
>> Denton Liu <liu.denton@gmail.com> 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.
>> >>
>> 	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
>
> With your suggestion, we actually introduce subtle undesired behaviour.
> The `error` calls don't actually exit the function early. To make it
> work, we need to add && to the end of the `error` calls.

Not &&-at-the-end, but yes, we'd need some early return after noticing a bad input from the caller.

You said earlier that one of the issues that motivated you to update the helper was that this obvious typo

	r1=... r2= ... &&
	! test_rev_cmp "$r1" "$rr2"

would not be noticed. For such a fix, I do not think it is sufficient to tweak the return value from the test_rev_cmp helper function if we allow callers to expect failure like so.

And for that reason, your "allow 'test_rev_cmp ! R1 R2' syntax" part of the change makes quite a lot of sense. That again allows the callers to rely on failure return from test_rev_cmp as an error.

Show 6 quoted lines

> I'm wondering why we want to do this, though. rev-parse should already
> output an error message on stderr in the case where the rev-parse fails.
>
> I guess the error message of "fatal: Needed a single revision" could
> probably be improved but that feels like an improvement that should be
> targeted to rev-parse.

Not really. The callers of rev-parse plumbing should expect that exact string, if they want to differenciate different errors from the program.

	r1=$(git rev-parse --verify "$1") &&
	r2=$(git rev-parse --verify "$2") || return 1

before we start the comparison between $r1 and $r2 may be a good way to clarify the intent of the code. Using "&&" instead of "|| return" and letting the whole function fail would not be incorrect, though.

Thanks.