* [PATCH 1/4] rev-parse test: modernize quoting and whitespace
2013-09-03 17:04 ` [PATCH 0/4] " Jonathan Nieder
@ 2013-09-03 17:05 ` Jonathan Nieder
2013-09-03 17:06 ` [PATCH 2/4] rev-parse test: use test_must_fail, not "if <command>; then false; fi" Jonathan Nieder
` (5 subsequent siblings)
6 siblings, 0 replies; 45+ messages in thread
From: Jonathan Nieder @ 2013-09-03 17:05 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Felipe Contreras, Jeff King, git
From: Felipe Contreras <felipe.contreras@gmail.com>
Instead of cramming everything in one line, put the test body in an
indented block after the opening test_expect_success line and quote
and put the closing quote on a line by itself.
Use single-quote instead of double-quote to quote the test body
for more useful --verbose output.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t6101-rev-parse-parents.sh | 74 ++++++++++++++++++++++++++++++++++----------
1 file changed, 58 insertions(+), 16 deletions(-)
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index e673c25..c47b869 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -17,21 +17,62 @@ hide_error save_tag start2 unique_commit "start2" tree
save_tag two_parents unique_commit "next" tree -p second -p start2
save_tag final unique_commit "final" tree -p two_parents
-test_expect_success 'start is valid' 'git rev-parse start | grep "^[0-9a-f]\{40\}$"'
-test_expect_success 'start^0' "test $(cat .git/refs/tags/start) = $(git rev-parse start^0)"
-test_expect_success 'start^1 not valid' "if git rev-parse --verify start^1; then false; else :; fi"
-test_expect_success 'second^1 = second^' "test $(git rev-parse second^1) = $(git rev-parse second^)"
-test_expect_success 'final^1^1^1' "test $(git rev-parse start) = $(git rev-parse final^1^1^1)"
-test_expect_success 'final^1^1^1 = final^^^' "test $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)"
-test_expect_success 'final^1^2' "test $(git rev-parse start2) = $(git rev-parse final^1^2)"
-test_expect_success 'final^1^2 != final^1^1' "test $(git rev-parse final^1^2) != $(git rev-parse final^1^1)"
-test_expect_success 'final^1^3 not valid' "if git rev-parse --verify final^1^3; then false; else :; fi"
-test_expect_success '--verify start2^1' 'test_must_fail git rev-parse --verify start2^1'
-test_expect_success '--verify start2^0' 'git rev-parse --verify start2^0'
-test_expect_success 'final^1^@ = final^1^1 final^1^2' "test \"$(git rev-parse final^1^@)\" = \"$(git rev-parse final^1^1 final^1^2)\""
-test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' "test \"$(git rev-parse final^1^\!)\" = \"$(git rev-parse final^1 ^final^1^1 ^final^1^2)\""
-
-test_expect_success 'repack for next test' 'git repack -a -d'
+test_expect_success 'start is valid' '
+ git rev-parse start | grep "^[0-9a-f]\{40\}$"
+'
+
+test_expect_success 'start^0' '
+ test $(cat .git/refs/tags/start) = $(git rev-parse start^0)
+'
+
+test_expect_success 'start^1 not valid' '
+ if git rev-parse --verify start^1; then false; else :; fi
+'
+
+test_expect_success 'second^1 = second^' '
+ test $(git rev-parse second^1) = $(git rev-parse second^)
+'
+
+test_expect_success 'final^1^1^1' '
+ test $(git rev-parse start) = $(git rev-parse final^1^1^1)
+'
+
+test_expect_success 'final^1^1^1 = final^^^' '
+ test $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)
+'
+
+test_expect_success 'final^1^2' '
+ test $(git rev-parse start2) = $(git rev-parse final^1^2)
+'
+
+test_expect_success 'final^1^2 != final^1^1' '
+ test $(git rev-parse final^1^2) != $(git rev-parse final^1^1)
+'
+
+test_expect_success 'final^1^3 not valid' '
+ if git rev-parse --verify final^1^3; then false; else :; fi
+'
+
+test_expect_success '--verify start2^1' '
+ test_must_fail git rev-parse --verify start2^1
+'
+
+test_expect_success '--verify start2^0' '
+ git rev-parse --verify start2^0
+'
+
+test_expect_success 'final^1^@ = final^1^1 final^1^2' '
+ test "$(git rev-parse final^1^@)" = "$(git rev-parse final^1^1 final^1^2)"
+'
+
+test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' '
+ test "$(git rev-parse final^1^!)" = "$(git rev-parse final^1 ^final^1^1 ^final^1^2)"
+'
+
+test_expect_success 'repack for next test' '
+ git repack -a -d
+'
+
test_expect_success 'short SHA-1 works' '
start=`git rev-parse --verify start` &&
echo $start &&
@@ -39,6 +80,7 @@ test_expect_success 'short SHA-1 works' '
echo $abbrv &&
abbrv=`git rev-parse --verify $abbrv` &&
echo $abbrv &&
- test $start = $abbrv'
+ test $start = $abbrv
+'
test_done
--
1.8.4
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 2/4] rev-parse test: use test_must_fail, not "if <command>; then false; fi"
2013-09-03 17:04 ` [PATCH 0/4] " Jonathan Nieder
2013-09-03 17:05 ` [PATCH 1/4] rev-parse test: modernize quoting and whitespace Jonathan Nieder
@ 2013-09-03 17:06 ` Jonathan Nieder
2013-09-03 21:56 ` Felipe Contreras
2013-09-03 17:07 ` [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin Jonathan Nieder
` (4 subsequent siblings)
6 siblings, 1 reply; 45+ messages in thread
From: Jonathan Nieder @ 2013-09-03 17:06 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Felipe Contreras, Jeff King, git
From: Felipe Contreras <felipe.contreras@gmail.com>
This way, if rev-parse segfaults then the test will fail instead
of treating it the same way as a controlled failure.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t6101-rev-parse-parents.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index c47b869..416067c 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -26,7 +26,7 @@ test_expect_success 'start^0' '
'
test_expect_success 'start^1 not valid' '
- if git rev-parse --verify start^1; then false; else :; fi
+ test_must_fail git rev-parse --verify start^1
'
test_expect_success 'second^1 = second^' '
@@ -50,7 +50,7 @@ test_expect_success 'final^1^2 != final^1^1' '
'
test_expect_success 'final^1^3 not valid' '
- if git rev-parse --verify final^1^3; then false; else :; fi
+ test_must_fail git rev-parse --verify final^1^3
'
test_expect_success '--verify start2^1' '
--
1.8.4
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin
2013-09-03 17:04 ` [PATCH 0/4] " Jonathan Nieder
2013-09-03 17:05 ` [PATCH 1/4] rev-parse test: modernize quoting and whitespace Jonathan Nieder
2013-09-03 17:06 ` [PATCH 2/4] rev-parse test: use test_must_fail, not "if <command>; then false; fi" Jonathan Nieder
@ 2013-09-03 17:07 ` Jonathan Nieder
2013-09-03 20:01 ` Junio C Hamano
2013-09-03 22:01 ` Felipe Contreras
2013-09-03 17:11 ` [PATCH 4/4] rev-parse test: use standard test functions for setup Jonathan Nieder
` (3 subsequent siblings)
6 siblings, 2 replies; 45+ messages in thread
From: Jonathan Nieder @ 2013-09-03 17:07 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Felipe Contreras, Jeff King, git
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 <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t6101-rev-parse-parents.sh | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index 416067c..8a6ff66 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -8,6 +8,12 @@ test_description='Test git rev-parse with different parent options'
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-t6000.sh # t6xxx specific functions
+test_cmp_rev_output () {
+ git rev-parse --verify "$1" >expect &&
+ eval "$2" >actual &&
+ test_cmp expect actual
+}
+
date >path0
git update-index --add path0
save_tag tree git write-tree
@@ -22,7 +28,7 @@ test_expect_success 'start is valid' '
'
test_expect_success 'start^0' '
- test $(cat .git/refs/tags/start) = $(git rev-parse start^0)
+ test_cmp_rev_output tags/start "git rev-parse start^0"
'
test_expect_success 'start^1 not valid' '
@@ -30,19 +36,19 @@ test_expect_success 'start^1 not valid' '
'
test_expect_success 'second^1 = second^' '
- test $(git rev-parse second^1) = $(git rev-parse second^)
+ test_cmp_rev_output second^ "git rev-parse second^1"
'
test_expect_success 'final^1^1^1' '
- test $(git rev-parse start) = $(git rev-parse final^1^1^1)
+ test_cmp_rev_output start "git rev-parse final^1^1^1"
'
test_expect_success 'final^1^1^1 = final^^^' '
- test $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)
+ test_cmp_rev_output final^^^ "git rev-parse final^1^1^1"
'
test_expect_success 'final^1^2' '
- test $(git rev-parse start2) = $(git rev-parse final^1^2)
+ test_cmp_rev_output start2 "git rev-parse final^1^2"
'
test_expect_success 'final^1^2 != final^1^1' '
@@ -62,11 +68,15 @@ test_expect_success '--verify start2^0' '
'
test_expect_success 'final^1^@ = final^1^1 final^1^2' '
- test "$(git rev-parse final^1^@)" = "$(git rev-parse final^1^1 final^1^2)"
+ git rev-parse final^1^1 final^1^2 >expect &&
+ git rev-parse final^1^@ >actual &&
+ test_cmp expect actual
'
test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' '
- test "$(git rev-parse final^1^!)" = "$(git rev-parse final^1 ^final^1^1 ^final^1^2)"
+ git rev-parse final^1 ^final^1^1 ^final^1^2 >expect &&
+ git rev-parse final^1^! >actual &&
+ test_cmp expect actual
'
test_expect_success 'repack for next test' '
@@ -74,13 +84,8 @@ test_expect_success 'repack for next test' '
'
test_expect_success 'short SHA-1 works' '
- start=`git rev-parse --verify start` &&
- echo $start &&
- abbrv=`echo $start | sed s/.\$//` &&
- echo $abbrv &&
- abbrv=`git rev-parse --verify $abbrv` &&
- echo $abbrv &&
- test $start = $abbrv
+ start=$(git rev-parse --verify start) &&
+ test_cmp_rev_output start "git rev-parse ${start%?}"
'
test_done
--
1.8.4
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin
2013-09-03 17:07 ` [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin Jonathan Nieder
@ 2013-09-03 20:01 ` Junio C Hamano
2013-09-04 4:28 ` Jonathan Nieder
2013-09-03 22:01 ` Felipe Contreras
1 sibling, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2013-09-03 20:01 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: SZEDER Gábor, Felipe Contreras, Jeff King, git
Jonathan Nieder <jrnieder@gmail.com> writes:
> 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
I do not mind to drop s/a little // here, by the way.
> - 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 <felipe.contreras@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> t/t6101-rev-parse-parents.sh | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
> index 416067c..8a6ff66 100755
> --- a/t/t6101-rev-parse-parents.sh
> +++ b/t/t6101-rev-parse-parents.sh
> @@ -8,6 +8,12 @@ test_description='Test git rev-parse with different parent options'
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-t6000.sh # t6xxx specific functions
>
> +test_cmp_rev_output () {
> + git rev-parse --verify "$1" >expect &&
> + eval "$2" >actual &&
> + test_cmp expect actual
> +}
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?
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin
2013-09-03 20:01 ` Junio C Hamano
@ 2013-09-04 4:28 ` Jonathan Nieder
0 siblings, 0 replies; 45+ messages in thread
From: Jonathan Nieder @ 2013-09-04 4:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: SZEDER Gábor, Felipe Contreras, Jeff King, git
Junio C Hamano wrote:
> 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?
I was tempted to use test_cmp_rev, which would have the same effect.
The original test checked the output of "git rev-parse start^0", while
the test as modified above checks the output of "git rev-parse
--verify start^0".
I slightly prefer the version without --verify because "git rev-parse
--verify" is well exercised elsewhere in the testsuite (but then, so
is rev-parse without --verify, so it's not a big deal).
Abbreviating as follows
foo () {
git rev-parse --verify "$1" >expect &&
git rev-parse "$2" >actual &&
test_cmp expect actual
}
would also be fine with me. All it would take is a good replacement
for the placeholder "foo". The added flexibility of "compare a rev
to the output of an arbitrary command" doesn't get used.
Jonathan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin
2013-09-03 17:07 ` [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin Jonathan Nieder
2013-09-03 20:01 ` Junio C Hamano
@ 2013-09-03 22:01 ` Felipe Contreras
1 sibling, 0 replies; 45+ messages in thread
From: Felipe Contreras @ 2013-09-03 22:01 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: SZEDER Gábor, Jeff King, git
On Tue, Sep 3, 2013 at 12:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> test_expect_success 'start^0' '
> - test $(cat .git/refs/tags/start) = $(git rev-parse start^0)
> + test_cmp_rev_output tags/start "git rev-parse start^0"
> '
Backwards and yodaish this is.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 4/4] rev-parse test: use standard test functions for setup
2013-09-03 17:04 ` [PATCH 0/4] " Jonathan Nieder
` (2 preceding siblings ...)
2013-09-03 17:07 ` [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin Jonathan Nieder
@ 2013-09-03 17:11 ` Jonathan Nieder
2013-09-03 17:15 ` [PATCH v2 " Jonathan Nieder
2013-09-03 17:20 ` [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions Jeff King
` (2 subsequent siblings)
6 siblings, 1 reply; 45+ messages in thread
From: Jonathan Nieder @ 2013-09-03 17:11 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Felipe Contreras, Jeff King, git
From: Felipe Contreras <felipe.contreras@gmail.com>
Save the reader from learning specialized t6* setup functions
where familiar commands like test_commit, "git checkout --orphan",
and "git merge" will do.
While at it, wrap the setup commands in a test assertion so errors can
be caught and stray output suppressed when running without --verbose
as in other tests.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.
t/t6101-rev-parse-parents.sh | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index 8a6ff66..b5fa9e4 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -14,14 +14,15 @@ test_cmp_rev_output () {
test_cmp expect actual
}
-date >path0
-git update-index --add path0
-save_tag tree git write-tree
-hide_error save_tag start unique_commit "start" tree
-save_tag second unique_commit "second" tree -p start
-hide_error save_tag start2 unique_commit "start2" tree
-save_tag two_parents unique_commit "next" tree -p second -p start2
-save_tag final unique_commit "final" tree -p two_parents
+test_expect_success 'setup' '
+ test_commit start &&
+ test_commit second &&
+ git checkout --orphan tmp &&
+ test_commit start2 &&
+ git checkout master &&
+ git merge -m next start2 &&
+ test_commit final
+'
test_expect_success 'start is valid' '
git rev-parse start | grep "^[0-9a-f]\{40\}$"
--
1.8.4
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 4/4] rev-parse test: use standard test functions for setup
2013-09-03 17:11 ` [PATCH 4/4] rev-parse test: use standard test functions for setup Jonathan Nieder
@ 2013-09-03 17:15 ` Jonathan Nieder
0 siblings, 0 replies; 45+ messages in thread
From: Jonathan Nieder @ 2013-09-03 17:15 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Felipe Contreras, Jeff King, git
From: Felipe Contreras <felipe.contreras@gmail.com>
Save the reader from learning specialized t6* setup functions
where familiar commands like test_commit, "git checkout --orphan",
and "git merge" will do.
While at it, wrap the setup commands in a test assertion so errors can
be caught and stray output suppressed when running without --verbose
as in other tests.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Lost a line from the patch last time (removal of the
'. "$TEST_DIRECTORY"/lib-t6000.sh' directive. Sorry about that ---
fixed now.
t/t6101-rev-parse-parents.sh | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index 8a6ff66..7ea14ce 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -6,7 +6,6 @@
test_description='Test git rev-parse with different parent options'
. ./test-lib.sh
-. "$TEST_DIRECTORY"/lib-t6000.sh # t6xxx specific functions
test_cmp_rev_output () {
git rev-parse --verify "$1" >expect &&
@@ -14,14 +13,15 @@ test_cmp_rev_output () {
test_cmp expect actual
}
-date >path0
-git update-index --add path0
-save_tag tree git write-tree
-hide_error save_tag start unique_commit "start" tree
-save_tag second unique_commit "second" tree -p start
-hide_error save_tag start2 unique_commit "start2" tree
-save_tag two_parents unique_commit "next" tree -p second -p start2
-save_tag final unique_commit "final" tree -p two_parents
+test_expect_success 'setup' '
+ test_commit start &&
+ test_commit second &&
+ git checkout --orphan tmp &&
+ test_commit start2 &&
+ git checkout master &&
+ git merge -m next start2 &&
+ test_commit final
+'
test_expect_success 'start is valid' '
git rev-parse start | grep "^[0-9a-f]\{40\}$"
--
1.8.4
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-03 17:04 ` [PATCH 0/4] " Jonathan Nieder
` (3 preceding siblings ...)
2013-09-03 17:11 ` [PATCH 4/4] rev-parse test: use standard test functions for setup Jonathan Nieder
@ 2013-09-03 17:20 ` Jeff King
2013-09-03 21:53 ` Felipe Contreras
2013-09-04 16:47 ` Junio C Hamano
6 siblings, 0 replies; 45+ messages in thread
From: Jeff King @ 2013-09-03 17:20 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: SZEDER Gábor, Felipe Contreras, git
On Tue, Sep 03, 2013 at 10:04:19AM -0700, Jonathan Nieder wrote:
> > 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.
> ;-)
Yes. IMHO it is not just "not worth the churn" but actively making the
code less readable.
> While at it, I rerolled the other patches from the series to clarify
> their commit messages (replacing "fix <something>" with a fuller
> description).
The series looks fine to me, modulo the fix up in v2 of 4/4.
-Peff
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-03 17:04 ` [PATCH 0/4] " Jonathan Nieder
` (4 preceding siblings ...)
2013-09-03 17:20 ` [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions Jeff King
@ 2013-09-03 21:53 ` Felipe Contreras
2013-09-04 16:47 ` Junio C Hamano
6 siblings, 0 replies; 45+ messages in thread
From: Felipe Contreras @ 2013-09-03 21:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: SZEDER Gábor, Jeff King, git
On Tue, Sep 3, 2013 at 12:04 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> SZEDER Gábor wrote:
>> On Tue, Sep 03, 2013 at 08:39:54AM -0500, Felipe Contreras wrote:
>
>>> 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.
>
> Especially when the idiom being changed is not even being made better.
> ;-)
But it is 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.
Those are irrelevant implementation details.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-03 17:04 ` [PATCH 0/4] " Jonathan Nieder
` (5 preceding siblings ...)
2013-09-03 21:53 ` Felipe Contreras
@ 2013-09-04 16:47 ` Junio C Hamano
2013-09-04 17:13 ` John Keeping
6 siblings, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2013-09-04 16:47 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: SZEDER Gábor, Felipe Contreras, Jeff King, git
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.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-04 16:47 ` Junio C Hamano
@ 2013-09-04 17:13 ` John Keeping
2013-09-04 17:38 ` Junio C Hamano
0 siblings, 1 reply; 45+ messages in thread
From: John Keeping @ 2013-09-04 17:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, SZEDER Gábor, Felipe Contreras, Jeff King,
git
On Wed, Sep 04, 2013 at 09:47:12AM -0700, Junio C Hamano wrote:
> 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.
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, although I have no idea why that particular order is
chosen and is so common.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-04 17:13 ` John Keeping
@ 2013-09-04 17:38 ` Junio C Hamano
2013-09-04 18:36 ` Jeff King
0 siblings, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2013-09-04 17:38 UTC (permalink / raw)
To: John Keeping
Cc: Jonathan Nieder, SZEDER Gábor, Felipe Contreras, Jeff King,
git
John Keeping <john@keeping.me.uk> writes:
> On Wed, Sep 04, 2013 at 09:47:12AM -0700, Junio C Hamano wrote:
>> 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.
>
> 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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-04 17:38 ` Junio C Hamano
@ 2013-09-04 18:36 ` Jeff King
2013-09-04 19:14 ` Junio C Hamano
2013-09-08 3:11 ` Felipe Contreras
0 siblings, 2 replies; 45+ messages in thread
From: Jeff King @ 2013-09-04 18:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: John Keeping, Jonathan Nieder, SZEDER Gábor,
Felipe Contreras, git
On Wed, Sep 04, 2013 at 10:38:03AM -0700, Junio C Hamano wrote:
> >> 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.
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.
-Peff
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-04 18:36 ` Jeff King
@ 2013-09-04 19:14 ` Junio C Hamano
2013-09-08 3:11 ` Felipe Contreras
1 sibling, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2013-09-04 19:14 UTC (permalink / raw)
To: Jeff King
Cc: John Keeping, Jonathan Nieder, SZEDER Gábor,
Felipe Contreras, git
Jeff King <peff@peff.net> writes:
> On Wed, Sep 04, 2013 at 10:38:03AM -0700, Junio C Hamano wrote:
>
>> >> 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.
>
> I don't think it was intentional at the time.
I do not think so, either. The only thing we cared about was "are
they equal". And from the point of view of test_cmp exit status,
that still the only thing we care about. Comparison to see which is
greater is a superset of equality check we needed, and in that
context, the order did not and does not matter.
One only starts to notice the order of comparison when one starts
thinking about the comparison in terms of "what is subtracted from
what", and at that point, one realizes that "diff A B" that gives us
what was lost from A to B as "-" and what was gained in B relative
to A as "+" is showing the result of subtracting A from B. And that
subtraction is backwards from the cmp(A,B) that subtracts B from A,
which is the usual convention.
> 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.
As I think test_cmp is primarily about equality comparison, I do not
think it is worth switching and adapting. Switching makes sense
only in my dreams where we did not have to worry about in-flight
topics and people's brains that are used to the current order.
That is exactly where my "with reservations" came from.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-04 18:36 ` Jeff King
2013-09-04 19:14 ` Junio C Hamano
@ 2013-09-08 3:11 ` Felipe Contreras
2013-09-08 4:06 ` Jeff King
1 sibling, 1 reply; 45+ messages in thread
From: Felipe Contreras @ 2013-09-08 3:11 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Wed, Sep 4, 2013 at 1:36 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Sep 04, 2013 at 10:38:03AM -0700, Junio C Hamano wrote:
>
>> >> 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.
>
> 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.
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?
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 3:11 ` Felipe Contreras
@ 2013-09-08 4:06 ` Jeff King
2013-09-08 4:13 ` Felipe Contreras
2013-09-08 8:11 ` Philip Oakley
0 siblings, 2 replies; 45+ messages in thread
From: Jeff King @ 2013-09-08 4:06 UTC (permalink / raw)
To: Felipe Contreras
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sat, Sep 07, 2013 at 10:11:49PM -0500, Felipe Contreras wrote:
> > 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? And that is why I said it
seems more like an issue of personal preference than a universal truth.
Was there some objective argument made that I missed?
> 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?"
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.
-Peff
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 4:06 ` Jeff King
@ 2013-09-08 4:13 ` Felipe Contreras
2013-09-08 4:14 ` Felipe Contreras
2013-09-08 4:26 ` Jeff King
2013-09-08 8:11 ` Philip Oakley
1 sibling, 2 replies; 45+ messages in thread
From: Felipe Contreras @ 2013-09-08 4:13 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sat, Sep 7, 2013 at 11:06 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Sep 07, 2013 at 10:11:49PM -0500, Felipe Contreras wrote:
>
>> > 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".
---
> Otherwise why would so many
> existing test frameworks do it the other way?
Which many existing frameworks do it the other way?
>> 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?"
>
> 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
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 4:13 ` Felipe Contreras
@ 2013-09-08 4:14 ` Felipe Contreras
2013-09-08 4:26 ` Jeff King
1 sibling, 0 replies; 45+ messages in thread
From: Felipe Contreras @ 2013-09-08 4:14 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sat, Sep 7, 2013 at 11:13 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Sep 7, 2013 at 11:06 PM, Jeff King <peff@peff.net> wrote:
>> On Sat, Sep 07, 2013 at 10:11:49PM -0500, Felipe Contreras wrote:
>>
>>> > 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".
> ---
>
>> Otherwise why would so many
>> existing test frameworks do it the other way?
>
> Which many existing frameworks do it the other way?
>
>>> 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?"
>>
>> 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?
And btw, that could barely be translated to cmp(B, A), probably cmp_given(B, A).
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 4:13 ` Felipe Contreras
2013-09-08 4:14 ` Felipe Contreras
@ 2013-09-08 4:26 ` Jeff King
2013-09-08 4:52 ` Felipe Contreras
2013-09-08 18:33 ` Junio C Hamano
1 sibling, 2 replies; 45+ messages in thread
From: Jeff King @ 2013-09-08 4:26 UTC (permalink / raw)
To: Felipe Contreras
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sat, Sep 07, 2013 at 11:13:10PM -0500, Felipe Contreras wrote:
> > 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".
> ---
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".
> > 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).
> > 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
I already said that is how I think about it. If you want to call me not
sane, feel free. But I do not see that this line of discussion is going
anywhere productive.
-Peff
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 4:26 ` Jeff King
@ 2013-09-08 4:52 ` Felipe Contreras
2013-09-08 5:02 ` Jeff King
2013-09-08 18:33 ` Junio C Hamano
1 sibling, 1 reply; 45+ messages in thread
From: Felipe Contreras @ 2013-09-08 4:52 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sat, Sep 7, 2013 at 11:26 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Sep 07, 2013 at 11:13:10PM -0500, Felipe Contreras wrote:
>
>> > 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".
>> ---
>
> 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'.
>> > 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).
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.
>> > 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
>
> 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.
Again, that's because you are already thinking on the resulting diff,
based on the 'diff -u' command, but that's not in question here. Even
if test_cmp() didn't return an diff (it just ran cmp), it would be
useful, as ultimately we want to test for failures. Ultimately what we
want is to check that A is equal to B, so it's natural to tell the
computer "compare A with B", and if you don't think so, then yeah, I
think you are insane.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 4:52 ` Felipe Contreras
@ 2013-09-08 5:02 ` Jeff King
2013-09-08 23:25 ` Felipe Contreras
0 siblings, 1 reply; 45+ messages in thread
From: Jeff King @ 2013-09-08 5:02 UTC (permalink / raw)
To: Felipe Contreras
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sat, Sep 07, 2013 at 11:52:10PM -0500, Felipe Contreras wrote:
> > 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.
> > 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.
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.
-Peff
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 5:02 ` Jeff King
@ 2013-09-08 23:25 ` Felipe Contreras
2013-09-08 23:45 ` Jeff King
0 siblings, 1 reply; 45+ messages in thread
From: Felipe Contreras @ 2013-09-08 23:25 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sun, Sep 8, 2013 at 12:02 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Sep 07, 2013 at 11:52:10PM -0500, Felipe Contreras wrote:
>
>> > 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).
Then I don't think you can give a fair and objective opinion about
what should be the ideal ordering of the arguments. You would be
forever biased to whatever is the order of 'diff -u'.
> 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.
>> > 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.
>
> 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.
All right, maybe it's because Ruby started in Japan, and their
sentences have very different orderings, maybe it's legacy from
another test framework, maybe there's no reason at all and somebody
just randomly picked them.
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.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 23:25 ` Felipe Contreras
@ 2013-09-08 23:45 ` Jeff King
2013-09-09 0:45 ` Felipe Contreras
0 siblings, 1 reply; 45+ messages in thread
From: Jeff King @ 2013-09-08 23:45 UTC (permalink / raw)
To: Felipe Contreras
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sun, Sep 08, 2013 at 06:25:45PM -0500, Felipe Contreras wrote:
> > 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.
> 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.
-Peff
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 23:45 ` Jeff King
@ 2013-09-09 0:45 ` Felipe Contreras
0 siblings, 0 replies; 45+ messages in thread
From: Felipe Contreras @ 2013-09-09 0:45 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
On Sun, Sep 8, 2013 at 6:45 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Sep 08, 2013 at 06:25:45PM -0500, Felipe Contreras wrote:
>
>> > 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.
So? That's the original intention from the author, it's not the point
of the function from the project's point of view, or what the point
_should_ be.
>> 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.
That doesn't show it they prefer it that way. They could be in the
same situation than us, they might prefer it a different way, but they
are stuck with what they chose long time ago.
> 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.
It's all right, I don't think you were contributing much more than;
diff -u does $expect $actual.
In case the above gets misconstrued as an insult, it's not meant that
way, Jeff said it himself; he cannot depart his view away from the
diff -u implementation.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 4:26 ` Jeff King
2013-09-08 4:52 ` Felipe Contreras
@ 2013-09-08 18:33 ` Junio C Hamano
2013-09-08 23:18 ` Felipe Contreras
1 sibling, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2013-09-08 18:33 UTC (permalink / raw)
To: Jeff King
Cc: Felipe Contreras, John Keeping, Jonathan Nieder,
SZEDER Gábor, git
Jeff King <peff@peff.net> writes:
>> 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".
>
>> > 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).
Especially the last one can be excused. "is A and B equal" is a
binary between "yes" and "no". If A and B are equal, B and A are
equal, and it becomes more like "which endianness is correct?" as
you mentioned earlier.
I think the real cause of confusion is that "cmp(1)" is not a
comparison in that sense but is an equality check; "test_cmp" has a
dual purpose in that its primary use as "did the previous step
produce correct result?" is an equality check and the order does not
really matter, but its secondary purpose, to show how the actual
output deviated from the norm, has to be done by subtracting the
expected result from the actual result.
As I said, I am somewhat sympathetic to those who want to see such
subtraction spelled as cmp(Actual,Expect), but we are so used to the
order "diff(1)" takes expect and actual to do that subtraction in
that order, so using diff(Expect,Actual) order is not that wrong.
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 ;-)
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 18:33 ` Junio C Hamano
@ 2013-09-08 23:18 ` Felipe Contreras
0 siblings, 0 replies; 45+ messages in thread
From: Felipe Contreras @ 2013-09-08 23:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, John Keeping, Jonathan Nieder, SZEDER Gábor, git
On Sun, Sep 8, 2013 at 1:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> 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 ;-)
Yes, but then it wouldn't be clear what's the main purpose of
test_diff(). Primarily what we want is to check that they are the
same. The diff is secondary, and it's not actually *needed*.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
2013-09-08 4:06 ` Jeff King
2013-09-08 4:13 ` Felipe Contreras
@ 2013-09-08 8:11 ` Philip Oakley
1 sibling, 0 replies; 45+ messages in thread
From: Philip Oakley @ 2013-09-08 8:11 UTC (permalink / raw)
To: Jeff King, Felipe Contreras
Cc: Junio C Hamano, John Keeping, Jonathan Nieder, SZEDER Gábor,
git
From: "Jeff King" <peff@peff.net>
Sent: Sunday, September 08, 2013 5:06 AM
> Was there some objective argument made that I missed?
>
>> Here's more; human semantics:
>>
Isn't this one of those "pick any two from three" tasks:
'human', 'objective', 'argument'.
Only 1/6 of the time is an 'objective' answer the result.
Philip
"Between thee and me there's nowt so queer as fowk,
and I ain't so sure about thee." old Yorkshire saying.
^ permalink raw reply [flat|nested] 45+ messages in thread