On Fri, Nov 08, 2019 at 12:24:12PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@xxxxxxxxx> 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.
>
> 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.
[...]
>
> > 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`.
>
>
> > 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
