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, 03:24 UTC
Message-ID: <xmqqpni3nj8j.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <0d0696f310a6f8e13ed480b1a1e91cdc2debaa20.1573152599.git.liu.denton@gmail.com>

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

Show 9 quoted lines

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

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.

> Rewrite test_cmp_rev() to optionally accept "!" as the first argument to
> do a not-equals comparison. Rewrite `! test_cmp_rev` to `test_cmp_rev !`
> in all tests to take advantage of this new functionality.

Good.

> -# Tests that its two parameters refer to the same revision
> +# Tests that its two parameters refer to the same revision, or if '!' is
> +# provided first, that its other two parameters refer to different
> +# revisions.

OK.

Show 8 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?

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

Show 20 quoted lines

> -		if test "$r1" != "$r2"
> +		if test "$r1" "$inverted_op" "$r2"
>  		then
> +			local comp_out
> +			if "x$inverted_op" = 'x='
> +			then
> +				comp_out='the same'
> +			else
> +				comp_out='different'
> +			fi
>  			cat >&4 <<-EOF
> -			error: two revisions point to different objects:
> +			error: two revisions point to $comp_out objects:
>  			  '$1': $r1
>  			  '$2': $r2
>  			EOF
>  			return 1
>  		fi
>  	fi
>  }