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

From: Denton Liu <liu.denton@gmail.com>
Date: Nov 8, 2019, 08:23 UTC
Message-ID: <20191108082310.GA2497@generichostname>
In-Reply-To: <xmqqpni3nj8j.fsf@gitster-ct.c.googlers.com>

On Fri, Nov 08, 2019 at 12:24:12PM +0900, Junio C Hamano wrote:

Show 11 quoted lines

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Currently, in the case where we are using test_cmp_rev() to report
> > not-equals, we write `! test_cmp_rev`. However, since test_cmp_rev()
> > contains
> >
> > 	r1=$(git rev-parse --verify "$1") &&
> > 	r2=$(git rev-parse --verify "$2") &&
> >
> > In the case where `git rev-parse` segfaults and dies unexpectedly, the
> > failure will be ignored.

I'll probably reword this to

	In the case where we are using test_cmp_rev() to report
	not-equals, we write `! test_cmp_rev`. However, since
	test_cmp_rev() contains

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

	`! test_cmp_rev` will succeed if any of the rev-parses fail.
	This behavior is not desired. We want the rev-parses to _always_
	be successful.

because of your puzzlement below.

Show 7 quoted lines

> 
> Good justification.  The last two lines are continuation of the
> sentence that begins the proposed log message, so downcase "In" at
> the beginning of the line.  Also, when we present the problem to be
> solved at the beginning, it is customary to describe the status quo,
> and "Currently, " is a noiseword that does not add much information,
> so drop it.

Thanks, I'll make a note to stop doing this in future patches as well.

[...]

Show 14 quoted lines

> 
> >  test_cmp_rev () {
> > +	local inverted_op
> > +	inverted_op='!='
> > +	if test $# -ge 1 && test "x$1" = 'x!'
> > +	then
> > +	    inverted_op='='
> > +	    shift
> > +	fi
> 
> I'd rather avoid having to keep track of negation to reduce mental
> burden.  How about using = by default and != when '!' was given
> (which would be more natural to readers) and call it $op, and say
> "if ! test $r1 $op $r2" where it is used?

Good idea, I felt a little uneasy doing the inverted thing but it never occurred to me to just negate the return code of `test`.

Show 17 quoted lines

> 
> 
> >  	if test $# != 2
> >  	then
> >  		error "bug in the test script: test_cmp_rev requires two revisions, but got $#"
> >  	else
> >  		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.

Thanks for the review,

Denton