Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> test_cmp_rev follows the same order of arguments a "diff -u" and
> produces the same output as plain "git diff".  It's perfectly readable
> and normal.

This is way off tangent, but I am somewhat sympathetic to Felipe's
"compare actual with expect", with reservations.

If one ignores all other constraints and focuses solely on the order
of argument test_cmp takes, one would realize that it is backwards.
A(ny) sanely defined "compare A with B" function should yield the
result of subtracting B from A, i.e. cmp(A,B) should be like (A-B).
That is what you feed qsort() and bsearch() (it is not limited to C;
you see the same in "sort { $a <=> $b }").  The definition naturally
makes "cmp(A,B) < 0" like "A < B" and "cmp(A,B) > 0" like "A > B".

But unfortunately, test_cmp is defined in terms of "diff -u" by
feeding its parameters in the same order as given.  "test_cmp A B"
just turns into "diff -u A B".

Now, we _do_ want to see "diff -u expect actual", so that in the
output, what is _missing_ in the actual output compared to the
expected output is marked with "-", and what is _excess_ is marked
with "+".  If you think about it, this output from "diff -u expect
actual" is giving us the result of subtracting expect from actual.

If we want to express it in terms of "cmp", we should be writing
"cmp(actual,expect)", not "cmp(expect,actual)".  The latter is to
subtract actual from expect, which is backwards.

It would have been better if a wrapper around "diff" to give us a
higher level "comparison" semantics, test_cmp, had been written in
such a way that hid this backward behaviour of "diff", when it was
introduced to replace hardcoded "diff -u".

I'd actually be ecstatic if one morning when I get up, I find that
test_cmp implementation were replaced with (the moral equivalent of)

	test_cmp () {
		diff -u "$2" "$1"
	}

and all the callsites of test_cmp in the test scripts in all the
topic branches in flight were also swapped to "test_cmp actual
expect" without anybody having to worry about mismerges, merge
conflicts and confusing people who are used to the current order for
the past 5 years.

But I do not think that is going to happen without a careful
planning.  We may be able to manage the code somehow (we can drop
all the topics in flight immediately after a release, apply the
"swap the order" big patch, and rebase all the topics on top), but
retraining people would not be instantaneous "flag day" event, and
we will see mistakes for a few months after doing so.

Oh, well.