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

Denton Liu [off-list ref] writes:

Hi Junio,

On Fri, Nov 08, 2019 at 09:49:02PM +0900, Junio C Hamano wrote:

quoted

Denton Liu [off-list ref] writes:

quoted

quoted

quoted

 		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.

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.