t: rev-parse-parents: cleanups

Just as 5 == X is weird, so is comparing first the expected value, and then the value we are testing. So switch them around.

Quoted 2 lines · Felipe Contreras

Just as 5 == X is weird, so is comparing first the expected value, and then the value we are testing. So switch them around.

Actually, our normal comparison order for test output is "test_cmp expect actual", as it shows a test failure as a diff with the expected output as the base (i.e., the diff shows what went wrong).

That reasoning does not apply to "test a = b", which shows no output at all. However, if you want to clean up and modernize these tests, it would probably be better to simply convert them to use test_cmp.

I wonder if we should have a:

  test_cmp_args () {
          echo "$1" >expect &&
          echo "$1" >actual &&
          test_cmp expect actual
  }

to let these remain one-liners like:

  test_cmp_args "$(git rev-parse start)" "$(git rev-parse final^1^1^1)"

Quoted 8 lines

I wonder if we should have a:

  test_cmp_args () {
          echo "$1" >expect &&
          echo "$1" >actual &&
          test_cmp expect actual
  }

Quoted 3 lines

to let these remain one-liners like:

  test_cmp_args "$(git rev-parse start)" "$(git rev-parse final^1^1^1)"

This idea come up before, but there is one flaw which makes this function less useful: a non-zero exit code of the commands in the command substitutions would be lost.

Quoted 8 lines · SZEDER Gábor

Quoted 8 lines

I wonder if we should have a:

  test_cmp_args () {
          echo "$1" >expect &&
          echo "$1" >actual &&
          test_cmp expect actual
  }

Quoted 7 lines

Quoted 3 lines

to let these remain one-liners like:

  test_cmp_args "$(git rev-parse start)" "$(git rev-parse final^1^1^1)"

This idea come up before, but there is one flaw which makes this function less useful: a non-zero exit code of the commands in the command substitutions would be lost.

Good point. You'd probably have to do something gross with eval, like:

  test_cmp_args () {
    eval "$1" >expect &&
    eval "$2" >actual &&

test_cmp expect actual
}

Quoted 20 lines · Jeff King

Quoted 8 lines · SZEDER Gábor

Quoted 8 lines

I wonder if we should have a:

  test_cmp_args () {
          echo "$1" >expect &&
          echo "$1" >actual &&
          test_cmp expect actual
  }

Quoted 7 lines

Quoted 3 lines

to let these remain one-liners like:

  test_cmp_args "$(git rev-parse start)" "$(git rev-parse final^1^1^1)"

This idea come up before, but there is one flaw which makes this function less useful: a non-zero exit code of the commands in the command substitutions would be lost.

Good point. You'd probably have to do something gross with eval, like:

Quoted 3 lines

  test_cmp_args () {
    eval "$1" >expect &&
    eval "$2" >actual &&

I don't see any reason to perpetuate these yoda comparisons.

eval "$2" >expect &&
eval "$1" >actual &&

Quoted 2 lines

test_cmp expect actual
}

Quoted 25 lines · Felipe Contreras

Quoted 23 lines · Jeff King

Quoted 8 lines · SZEDER Gábor

Quoted 8 lines

I wonder if we should have a:

  test_cmp_args () {
          echo "$1" >expect &&
          echo "$1" >actual &&
          test_cmp expect actual
  }

Quoted 7 lines

Quoted 3 lines

to let these remain one-liners like:

  test_cmp_args "$(git rev-parse start)" "$(git rev-parse final^1^1^1)"

This idea come up before, but there is one flaw which makes this function less useful: a non-zero exit code of the commands in the command substitutions would be lost.

Good point. You'd probably have to do something gross with eval, like:

  test_cmp_args () {
    eval "$1" >expect &&
    eval "$2" >actual &&

Quoted 4 lines

I don't see any reason to perpetuate these yoda comparisons.

eval "$2" >expect &&
eval "$1" >actual &&

I do. Your proposal requires the arguments in the reverse order compared to test_cmp. That inconsistency would be far worse than test_cmp_args "$expect" "$actual".

Quoted 12 lines · SZEDER Gábor

Quoted 11 lines · Felipe Contreras

Quoted 10 lines · Jeff King

Quoted 8 lines · SZEDER Gábor

Quoted 8 lines

I wonder if we should have a:

  test_cmp_args () {
          echo "$1" >expect &&
          echo "$1" >actual &&
          test_cmp expect actual
  }

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

to let these remain one-liners like:

  test_cmp_args "$(git rev-parse start)" "$(git rev-parse final^1^1^1)"

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

Quoted 3 lines

This idea come up before, but there is one flaw which makes this function less useful: a non-zero exit code of the commands in the command substitutions would be lost.

Good point. You'd probably have to do something gross with eval, like:

  test_cmp_args () {
    eval "$1" >expect &&
    eval "$2" >actual &&

I don't see any reason to perpetuate these yoda comparisons.

eval "$2" >expect &&
eval "$1" >actual &&

Quoted 3 lines

I do. Your proposal requires the arguments in the reverse order compared to test_cmp. That inconsistency would be far worse than test_cmp_args "$expect" "$actual".

There are two ways to fix an inconsistency, the other way is to fix test_cmp. But that would be a change, and change is not welcome in Git.

Quoted 3 lines · Felipe Contreras

There are two ways to fix an inconsistency, the other way is to fix test_cmp. But that would be a change, and change is not welcome in Git.

It depends on the change, I suppose. I agree, changing 3k+ lines just to avoid yoda conditions... I doubt the gain worth the code churn.

rev-parse test: use test_must_fail, not "if <command>; then false; fi"

Use test_cmp instead of passing two command substitutions to the "test" builtin. This way:

- when tests fail, they can print a helpful diff if run with "--verbose"

- the argument order "test_cmp expect actual" feels natural, unlike test <known> = <unknown> that seems a little backwards

- the exit status from invoking git is checked, so if rev-parse starts segfaulting then the test will notice and fail

Use a custom function for this instead of test_cmp_rev to emphasize that we are testing the output from "git rev-parse" with certain arguments, not checking that the revisions are equal in abstract.

Reported-by Felipe Contreras <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

---
 t/t6101-rev-parse-parents.sh | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

t/t6101-rev-parse-parents.sh +0 -0

index 416067c..8a6ff66 100755

After applying this patch and running "git show | grep test_cmp_rev_output", I notice that the second is always "git rev-parse <something>". Do we still need to eval these, or would it be sufficient to do

        test_cmp_rev_output () {
                git rev-parse --verify "$1" >expect &&
                git rev-parse --verify "$2" >actual &&
                test_cmp expect actual
        }

here, and make users like so:

- test_cmp_rev_output tags/start "git rev-parse start^0"
+ test_cmp_rev_output tags/start start^0

Am I missing something?

Quoted 2 lines

It depends on the change, I suppose. I agree, changing 3k+ lines just to avoid yoda conditions... I doubt the gain worth the code churn.

Especially when the idiom being changed is not even being made better. ;-)

While at it, I rerolled the other patches from the series to clarify their commit messages (replacing "fix <something>" with a fuller description).

Quoted 6 lines · SZEDER Gábor

Quoted 3 lines · Felipe Contreras

There are two ways to fix an inconsistency, the other way is to fix test_cmp. But that would be a change, and change is not welcome in Git.

Quoted 2 lines

It depends on the change, I suppose. I agree, changing 3k+ lines just to avoid yoda conditions... I doubt the gain worth the code churn.

Especially when the idiom being changed is not even being made better. ;-)

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. I think Felipe is pushing buttons and testing boundaries.

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.

Quoted 3 lines · Jonathan Nieder

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.

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Jonathan Nieder

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.

Quoted 2 lines

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

This isn't an argument either way, but note that JUnit (and NUnit and PHPUnit) all have assertEquals methods that take the arguments in the order "expect, actual". I've always assumed that Git's test framework was imitating that,...

Quoted 7 lines

Quoted 2 lines

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

This isn't an argument either way, but note that JUnit (and NUnit and PHPUnit) all have assertEquals methods that take the arguments in the order "expect, actual". I've always assumed that Git's test framework was imitating that,...

No. See 82ebb0b6 (add test_cmp function for test scripts, 2008-03-12). The "test_cmp" was a replacement for "diff -u", and the same order we fed "diff -u", i.e. expect then actual, was carried over.

Quoted 8 lines · Junio C Hamano

Quoted 7 lines

Quoted 2 lines

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

This isn't an argument either way, but note that JUnit (and NUnit and PHPUnit) all have assertEquals methods that take the arguments in the order "expect, actual". I've always assumed that Git's test framework was imitating that,...

Quoted 4 lines

No. See 82ebb0b6 (add test_cmp function for test scripts, 2008-03-12). The "test_cmp" was a replacement for "diff -u", and the same order we fed "diff -u", i.e. expect then actual, was carried over.

I don't think it was intentional at the time.

Though I prefer the current, I can certainly live and adapt to a changed standard, and I do not mind doing so if there is a good reason. But I've yet to see any argument beyond "it is not what I like". Which to me argues for the status quo as the path of least resistance.

Quoted 8 lines · Junio C Hamano

Quoted 7 lines

Quoted 2 lines

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

This isn't an argument either way, but note that JUnit (and NUnit and PHPUnit) all have assertEquals methods that take the arguments in the order "expect, actual". I've always assumed that Git's test framework was imitating that,...

Quoted 4 lines

No. See 82ebb0b6 (add test_cmp function for test scripts, 2008-03-12). The "test_cmp" was a replacement for "diff -u", and the same order we fed "diff -u", i.e. expect then actual, was carried over.

I don't think it was intentional at the time. But over the intervening 5 years, I have noticed that I certainly think of "test_cmp A B" as "differences from A to B", and the order makes sense. IOW, the "test_cmp is diff" abstraction is leaky, and that is fine (if it were not leaky, then order would not matter at all, but it clearly does).

But let's take a step back. This seems like an endian-ness issue to me. I.e., some people prefer one order for test assertions, and other people prefer the other. Is anyone actually right, or is this simply a matter of preference? And if it is simply a matter of preference, then why bother going through the pain of changing the current project standard?

Though I prefer the current, I can certainly live and adapt to a changed standard, and I do not mind doing so if there is a good reason. But I've yet to see any argument beyond "it is not what I like". Which to me argues for the status quo as the path of least resistance.

Quoted 4 lines

Though I prefer the current, I can certainly live and adapt to a changed standard, and I do not mind doing so if there is a good reason. But I've yet to see any argument beyond "it is not what I like". Which to me argues for the status quo as the path of least resistance.

Didn't Junio already provided reasoning?

Here's more; human semantics:

Computer, compare A with B
cmp(A, B)

Why would I write?

cmp(B, A)

Could you even construct an English sentence that starts with B, and then A?

Was there some objective argument made that I missed?

Quoted 2 lines

Here's more; human semantics:

Quoted 6 lines · Felipe Contreras

Quoted 4 lines

Though I prefer the current, I can certainly live and adapt to a changed standard, and I do not mind doing so if there is a good reason. But I've yet to see any argument beyond "it is not what I like". Which to me argues for the status quo as the path of least resistance.

Didn't Junio already provided reasoning?

If the reasoning is "cmp(actual, expect) makes more sense to humans" then I do not think it is universal.

Otherwise why would so many
existing test frameworks do it the other way?

Quoted 9 lines

Here's more; human semantics:

Computer, compare A with B
cmp(A, B)

Why would I write?

cmp(B, A)

Quoted 1 line

Could you even construct an English sentence that starts with B, and then A?

"Computer, given that we expect B, how does A differ?". Or "Computer, we expect B; does A match it?"

Or any number of variations. I'm sure you will say "but those seem awkward and unlike how I think about it". But that was my point; it seems to be a matter of preference.

Quoted 2 lines

If the reasoning is "cmp(actual, expect) makes more sense to humans" then I do not think it is universal.

No.

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

Quoted 2 lines

Otherwise why would so many
existing test frameworks do it the other way?

Which many existing frameworks do it the other way?

Quoted 3 lines

Or any number of variations. I'm sure you will say "but those seem awkward and unlike how I think about it". But that was my point; it seems to be a matter of preference.

Really? You think any sane human being would prefer:

Computer, given that we expect B, how does A differ?

To:

Computer, compare A with B

Quoted 5 lines · Felipe Contreras

Quoted 2 lines

If the reasoning is "cmp(actual, expect) makes more sense to humans" then I do not think it is universal.

No.

Quoted 7 lines

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

Ah, you mean "if you think that the compare function should behave like C *_cmp functions, it should be A-B". Perhaps it is simply that I do not think of the function in those terms, but more like "show me the differences from B to A".

Quoted 4 lines

Quoted 2 lines

Otherwise why would so many
existing test frameworks do it the other way?

Which many existing frameworks do it the other way?

John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I believe that Ruby's Test::Unit::Assertions also has assert_equal(expected, actual).

Quoted 8 lines

Quoted 3 lines

Or any number of variations. I'm sure you will say "but those seem awkward and unlike how I think about it". But that was my point; it seems to be a matter of preference.

Really? You think any sane human being would prefer:

Computer, given that we expect B, how does A differ?

Quoted 3 lines

To:

Computer, compare A with B

I already said that is how I think about it. If you want to call me notn sane, feel free. But I do not see that this line of discussion is going anywhere productive.

Quoted 4 lines

Ah, you mean "if you think that the compare function should behave like C *_cmp functions, it should be A-B". Perhaps it is simply that I do not think of the function in those terms, but more like "show me the differences from B to A".

But that is the problem, you are unable to ignore the implementation. You don't see test_cmp(), you see 'diff -u'.

Quoted 3 lines

John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I believe that Ruby's Test::Unit::Assertions also has assert_equal(expected, actual).

That's because they all do first expect, then actual.

assert_equal( expected, actual, failure_message = nil ) assert_not_equal( expected, actual, failure_message = nil )

That's why.

Quoted 7 lines · Felipe Contreras

Quoted 4 lines

Ah, you mean "if you think that the compare function should behave like C *_cmp functions, it should be A-B". Perhaps it is simply that I do not think of the function in those terms, but more like "show me the differences from B to A".

But that is the problem, you are unable to ignore the implementation. You don't see test_cmp(), you see 'diff -u'.

Yes, I already said earlier in the thread:

I certainly think of "test_cmp A B" as "differences from A to B", and the order makes sense. IOW, the "test_cmp is diff" abstraction is leaky, and that is fine (if it were not leaky, then order would not matter at all, but it clearly does).

And I do not think it is a problem. The point of the function is not to abstract away the idea of comparison. The point is to give a hook for people on systems without "diff -u" to run the test suite.

Quoted 6 lines

Quoted 3 lines

John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I believe that Ruby's Test::Unit::Assertions also has assert_equal(expected, actual).

That's because they all do first expect, then actual.

Quoted 4 lines

assert_equal( expected, actual, failure_message = nil ) assert_not_equal( expected, actual, failure_message = nil )

That's why.

I do not see any reason why "not_equal" would not also work as "assert_not_equal(actual, expected)". Maybe I am missing your point.

Quoted 3 lines

And I do not think it is a problem. The point of the function is not to abstract away the idea of comparison. The point is to give a hook for people on systems without "diff -u" to run the test suite.

The point according to whom? I say it's the other way around.

Either way the fact that others are doing it differently doesn't mean that's the best way, that would be argumentum ad populum, and mothers are keen to remind us that if other kids are jumping the bridge, that doesn't mean we should too.

Quoted 5 lines · Felipe Contreras

Quoted 3 lines

And I do not think it is a problem. The point of the function is not to abstract away the idea of comparison. The point is to give a hook for people on systems without "diff -u" to run the test suite.

The point according to whom? I say it's the other way around.

The point according to 82ebb0b (add test_cmp function for test scripts, 2008-03-12). I wish I had simply called it test_diff back then, and then this conversation could have never occurred.

Quoted 4 lines

Either way the fact that others are doing it differently doesn't mean that's the best way, that would be argumentum ad populum, and mothers are keen to remind us that if other kids are jumping the bridge, that doesn't mean we should too.

Did I once say "my way of thinking about it is the best way"? No. I said "I think it is a matter of preference". I mentioned other systems using a particular ordering to show that the set of people who prefer it the other way is non-zero.

Feel free to respond, but I have no interest in discussing this any further with you. This thread has become a giant time sink, and I have nothing else to say on the matter that I have not already said.

Quoted 6 lines

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

Ah, you mean "if you think that the compare function should behave like C *_cmp functions, it should be A-B". Perhaps it is simply that I do not think of the function in those terms, but more like "show me the differences from B to A".

Quoted 4 lines

Quoted 2 lines

Otherwise why would so many
existing test frameworks do it the other way?

Which many existing frameworks do it the other way?

John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I believe that Ruby's Test::Unit::Assertions also has assert_equal(expected, actual).

Jeff King <[email protected]> writes:

Calling the abstraction "test_diff" might have avoided the wasted brain bandwidth in this thread, but I do not think renaming it in test-lib-functions.sh is worth the trouble, either ;-)

Quoted 11 lines · Jeff King

Quoted 6 lines · Felipe Contreras

Quoted 4 lines

Though I prefer the current, I can certainly live and adapt to a changed standard, and I do not mind doing so if there is a good reason. But I've yet to see any argument beyond "it is not what I like". Which to me argues for the status quo as the path of least resistance.

Didn't Junio already provided reasoning?

If the reasoning is "cmp(actual, expect) makes more sense to humans" then I do not think it is universal.

No.

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

Quoted 2 lines

Otherwise why would so many
existing test frameworks do it the other way?

Which many existing frameworks do it the other way?

Quoted 14 lines

Quoted 10 lines

Here's more; human semantics:

Computer, compare A with B
cmp(A, B)

Why would I write?

cmp(B, A)

Could you even construct an English sentence that starts with B, and then A?

"Computer, given that we expect B, how does A differ?". Or "Computer, we expect B; does A match it?"

Quoted 3 lines

Or any number of variations. I'm sure you will say "but those seem awkward and unlike how I think about it". But that was my point; it seems to be a matter of preference.

Really? You think any sane human being would prefer:

Computer, given that we expect B, how does A differ?

There are two ways to fix an inconsistency, the other way is to fix test_cmp. But that would be a change, and change is not welcome in Git.

Quoted 3 lines · Felipe Contreras

There are two ways to fix an inconsistency, the other way is to fix test_cmp. But that would be a change, and change is not welcome in Git.

If you want to do "test_cmp $actual $expect", you would have to first "fix" people's expectation that "diff A B" produces a change necessary to bring A to B, which would not likely to happen. We do the 'test_cmp expect actual' for a reason.

Quoted 3 lines · Felipe Contreras

There are two ways to fix an inconsistency, the other way is to fix test_cmp. But that would be a change, and change is not welcome in Git.

If you want to do "test_cmp $actual $expect", you would have to first "fix" people's expectation that "diff A B" produces a change necessary to bring A to B, which would not likely to happen. We do the 'test_cmp expect actual' for a reason.