{"thread":{"id":"34829","subject":"[PATCH 0/4] t: rev-parse-parents: cleanups","startedAt":"2013-09-02T06:30:35Z","lastAt":"2013-09-09T00:45:07Z","messageCount":45,"participants":["Felipe Contreras","Jeff King","SZEDER Gábor","Jonathan Nieder","Junio C Hamano","John Keeping","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"226506","messageId":"1378103439-3225-1-git-send-email-felipe.contreras@gmail.com","threadId":"34829","inReplyTo":null,"subject":"[PATCH 0/4] t: rev-parse-parents: cleanups","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-02T06:30:35Z","receivedAt":"2013-09-02T06:30:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras (4):\n t: rev-parse-parents: fix style\n t: rev-parse-parents: fix weird ! notation\n t: rev-parse-parents: avoid yoda conditions\n t: rev-parse-parents: simplify setup\n\n t/t6101-rev-parse-parents.sh | 96 +++++++++++++++++++++++++++++++-------------\n 1 file changed, 69 insertions(+), 27 deletions(-)\n\n-- \n1.8.4-338-gefd7fa6\n"},{"id":"226507","messageId":"1378103439-3225-2-git-send-email-felipe.contreras@gmail.com","threadId":"34829","inReplyTo":"1378103439-3225-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/4] t: rev-parse-parents: fix style","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-02T06:30:36Z","receivedAt":"2013-09-02T06:30:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Pretty much all the tests follow a different style.\n\nSigned-off-by: Felipe Contreras \n---\n t/t6101-rev-parse-parents.sh | 74 ++++++++++++++++++++++++++++++++++----------\n 1 file changed, 58 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex e673c25..bead4d7 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -17,21 +17,62 @@ hide_error save_tag start2 unique_commit \"start2\" tree\n save_tag two_parents unique_commit \"next\" tree -p second -p start2\n save_tag final unique_commit \"final\" tree -p two_parents\n \n-test_expect_success 'start is valid' 'git rev-parse start | grep \"^[0-9a-f]\\{40\\}$\"'\n-test_expect_success 'start^0' \"test $(cat .git/refs/tags/start) = $(git rev-parse start^0)\"\n-test_expect_success 'start^1 not valid' \"if git rev-parse --verify start^1; then false; else :; fi\"\n-test_expect_success 'second^1 = second^' \"test $(git rev-parse second^1) = $(git rev-parse second^)\"\n-test_expect_success 'final^1^1^1' \"test $(git rev-parse start) = $(git rev-parse final^1^1^1)\"\n-test_expect_success 'final^1^1^1 = final^^^' \"test $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)\"\n-test_expect_success 'final^1^2' \"test $(git rev-parse start2) = $(git rev-parse final^1^2)\"\n-test_expect_success 'final^1^2 != final^1^1' \"test $(git rev-parse final^1^2) != $(git rev-parse final^1^1)\"\n-test_expect_success 'final^1^3 not valid' \"if git rev-parse --verify final^1^3; then false; else :; fi\"\n-test_expect_success '--verify start2^1' 'test_must_fail git rev-parse --verify start2^1'\n-test_expect_success '--verify start2^0' 'git rev-parse --verify start2^0'\n-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)\\\"\"\n-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)\\\"\"\n-\n-test_expect_success 'repack for next test' 'git repack -a -d'\n+test_expect_success 'start is valid' '\n+\tgit rev-parse start | grep \"^[0-9a-f]\\{40\\}$\"\n+'\n+\n+test_expect_success 'start^0' '\n+\ttest $(cat .git/refs/tags/start) = $(git rev-parse start^0)\n+'\n+\n+test_expect_success 'start^1 not valid' '\n+\tif git rev-parse --verify start^1; then false; else :; fi\n+'\n+\n+test_expect_success 'second^1 = second^' '\n+\ttest $(git rev-parse second^1) = $(git rev-parse second^)\n+'\n+\n+test_expect_success 'final^1^1^1' '\n+\ttest $(git rev-parse start) = $(git rev-parse final^1^1^1)\n+'\n+\n+test_expect_success 'final^1^1^1 = final^^^' '\n+\ttest $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)\n+'\n+\n+test_expect_success 'final^1^2' '\n+\ttest $(git rev-parse start2) = $(git rev-parse final^1^2)\n+'\n+\n+test_expect_success 'final^1^2 != final^1^1' '\n+\ttest $(git rev-parse final^1^2) != $(git rev-parse final^1^1)\n+'\n+\n+test_expect_success 'final^1^3 not valid' '\n+\tif git rev-parse --verify final^1^3; then false; else :; fi\n+'\n+\n+test_expect_success '--verify start2^1' '\n+\ttest_must_fail git rev-parse --verify start2^1\n+'\n+\n+test_expect_success '--verify start2^0' '\n+\tgit rev-parse --verify start2^0\n+'\n+\n+test_expect_success 'final^1^@ = final^1^1 final^1^2' '\n+\ttest \"$(git rev-parse final^1^@)\" = \"$(git rev-parse final^1^1 final^1^2)\"\n+'\n+\n+test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' '\n+\ttest \"$(git rev-parse final^1^\\!)\" = \"$(git rev-parse final^1 ^final^1^1 ^final^1^2)\"\n+'\n+\n+test_expect_success 'repack for next test' '\n+\tgit repack -a -d\n+'\n+\n test_expect_success 'short SHA-1 works' '\n \tstart=`git rev-parse --verify start` &&\n \techo $start &&\n@@ -39,6 +80,7 @@ test_expect_success 'short SHA-1 works' '\n \techo $abbrv &&\n \tabbrv=`git rev-parse --verify $abbrv` &&\n \techo $abbrv &&\n-\ttest $start = $abbrv'\n+\ttest $start = $abbrv\n+'\n \n test_done\n-- \n1.8.4-338-gefd7fa6\n"},{"id":"226508","messageId":"1378103439-3225-3-git-send-email-felipe.contreras@gmail.com","threadId":"34829","inReplyTo":"1378103439-3225-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/4] t: rev-parse-parents: fix weird ! notation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-02T06:30:37Z","receivedAt":"2013-09-02T06:30:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n t/t6101-rev-parse-parents.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex bead4d7..c10580c 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -26,7 +26,7 @@ test_expect_success 'start^0' '\n '\n \n test_expect_success 'start^1 not valid' '\n-\tif git rev-parse --verify start^1; then false; else :; fi\n+\ttest_must_fail git rev-parse --verify start^1\n '\n \n test_expect_success 'second^1 = second^' '\n@@ -50,7 +50,7 @@ test_expect_success 'final^1^2 != final^1^1' '\n '\n \n test_expect_success 'final^1^3 not valid' '\n-\tif git rev-parse --verify final^1^3; then false; else :; fi\n+\ttest_must_fail git rev-parse --verify final^1^3\n '\n \n test_expect_success '--verify start2^1' '\n-- \n1.8.4-338-gefd7fa6\n"},{"id":"226509","messageId":"1378103439-3225-4-git-send-email-felipe.contreras@gmail.com","threadId":"34829","inReplyTo":"1378103439-3225-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-02T06:30:38Z","receivedAt":"2013-09-02T06:30:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Just as 5 == X is weird, so is comparing first the expected value, and\nthen the value we are testing. So switch them around.\n\nSigned-off-by: Felipe Contreras \n---\n t/t6101-rev-parse-parents.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex c10580c..b9aef31 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -22,7 +22,7 @@ test_expect_success 'start is valid' '\n '\n \n test_expect_success 'start^0' '\n-\ttest $(cat .git/refs/tags/start) = $(git rev-parse start^0)\n+\ttest $(git rev-parse start^0) = $(cat .git/refs/tags/start)\n '\n \n test_expect_success 'start^1 not valid' '\n@@ -34,15 +34,15 @@ test_expect_success 'second^1 = second^' '\n '\n \n test_expect_success 'final^1^1^1' '\n-\ttest $(git rev-parse start) = $(git rev-parse final^1^1^1)\n+\ttest $(git rev-parse final^1^1^1) = $(git rev-parse start)\n '\n \n test_expect_success 'final^1^1^1 = final^^^' '\n-\ttest $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)\n+\ttest $(git rev-parse final^^^) = $(git rev-parse final^1^1^1)\n '\n \n test_expect_success 'final^1^2' '\n-\ttest $(git rev-parse start2) = $(git rev-parse final^1^2)\n+\ttest $(git rev-parse final^1^2) = $(git rev-parse start2)\n '\n \n test_expect_success 'final^1^2 != final^1^1' '\n-- \n1.8.4-338-gefd7fa6\n"},{"id":"226510","messageId":"1378103439-3225-5-git-send-email-felipe.contreras@gmail.com","threadId":"34829","inReplyTo":"1378103439-3225-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 4/4] t: rev-parse-parents: simplify setup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-02T06:30:39Z","receivedAt":"2013-09-02T06:30:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n t/t6101-rev-parse-parents.sh | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex b9aef31..0653f5e 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -6,16 +6,16 @@\n test_description='Test git rev-parse with different parent options'\n \n . ./test-lib.sh\n-. \"$TEST_DIRECTORY\"/lib-t6000.sh # t6xxx specific functions\n-\n-date >path0\n-git update-index --add path0\n-save_tag tree git write-tree\n-hide_error save_tag start unique_commit \"start\" tree\n-save_tag second unique_commit \"second\" tree -p start\n-hide_error save_tag start2 unique_commit \"start2\" tree\n-save_tag two_parents unique_commit \"next\" tree -p second -p start2\n-save_tag final unique_commit \"final\" tree -p two_parents\n+\n+test_expect_success 'setup' '\n+\ttest_commit start &&\n+\ttest_commit second &&\n+\tgit checkout --orphan tmp &&\n+\ttest_commit start2 &&\n+\tgit checkout master &&\n+\tgit merge -m next start2 &&\n+\ttest_commit final\n+'\n \n test_expect_success 'start is valid' '\n \tgit rev-parse start | grep \"^[0-9a-f]\\{40\\}$\"\n-- \n1.8.4-338-gefd7fa6\n"},{"id":"226585","messageId":"20130903071256.GD3608@sigill.intra.peff.net","threadId":"34829","inReplyTo":"1378103439-3225-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T07:12:56Z","receivedAt":"2013-09-03T07:12:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 02, 2013 at 01:30:38AM -0500, Felipe Contreras wrote:\n\n> Just as 5 == X is weird, so is comparing first the expected value, and\n> then the value we are testing. So switch them around.\n\nActually, our normal comparison order for test output is \"test_cmp\nexpect actual\", as it shows a test failure as a diff with the expected\noutput as the base (i.e., the diff shows what went wrong).\n\nThat reasoning does not apply to \"test a = b\", which shows no output at\nall. However, if you want to clean up and modernize these tests, it\nwould probably be better to simply convert them to use test_cmp.\n\nI wonder if we should have a:\n\n test_cmp_args () {\n echo \"$1\" >expect &&\n echo \"$1\" >actual &&\n test_cmp expect actual\n }\n\nto let these remain one-liners like:\n\n test_cmp_args \"$(git rev-parse start)\" \"$(git rev-parse final^1^1^1)\"\n\n-Peff\n"},{"id":"226601","messageId":"20130903075107.GA25540@goldbirke","threadId":"34829","inReplyTo":"20130903071256.GD3608@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-09-03T07:51:07Z","receivedAt":"2013-09-03T07:51:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Tue, Sep 03, 2013 at 03:12:56AM -0400, Jeff King wrote:\n> On Mon, Sep 02, 2013 at 01:30:38AM -0500, Felipe Contreras wrote:\n> \n> > Just as 5 == X is weird, so is comparing first the expected value, and\n> > then the value we are testing. So switch them around.\n> \n> Actually, our normal comparison order for test output is \"test_cmp\n> expect actual\", as it shows a test failure as a diff with the expected\n> output as the base (i.e., the diff shows what went wrong).\n> \n> That reasoning does not apply to \"test a = b\", which shows no output at\n> all. However, if you want to clean up and modernize these tests, it\n> would probably be better to simply convert them to use test_cmp.\n> \n> I wonder if we should have a:\n> \n> test_cmp_args () {\n> echo \"$1\" >expect &&\n> echo \"$1\" >actual &&\n> test_cmp expect actual\n> }\n> \n> to let these remain one-liners like:\n> \n> test_cmp_args \"$(git rev-parse start)\" \"$(git rev-parse final^1^1^1)\"\n\nThis idea come up before, but there is one flaw which makes this\nfunction less useful: a non-zero exit code of the commands in the\ncommand substitutions would be lost.\n\n http://thread.gmane.org/gmane.comp.version-control.git/226699/focus=227361\n\n\nBest,\nGábor\n"},{"id":"226603","messageId":"20130903080358.GA30158@sigill.intra.peff.net","threadId":"34829","inReplyTo":"20130903075107.GA25540@goldbirke","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T08:03:58Z","receivedAt":"2013-09-03T08:03:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 03, 2013 at 09:51:07AM +0200, SZEDER Gábor wrote:\n\n> > I wonder if we should have a:\n> > \n> > test_cmp_args () {\n> > echo \"$1\" >expect &&\n> > echo \"$1\" >actual &&\n> > test_cmp expect actual\n> > }\n> > \n> > to let these remain one-liners like:\n> > \n> > test_cmp_args \"$(git rev-parse start)\" \"$(git rev-parse final^1^1^1)\"\n> \n> This idea come up before, but there is one flaw which makes this\n> function less useful: a non-zero exit code of the commands in the\n> command substitutions would be lost.\n\nGood point. You'd probably have to do something gross with eval, like:\n\n test_cmp_args () {\n eval \"$1\" >expect &&\n eval \"$2\" >actual &&\n test_cmp expect actual\n }\n\nbut then the callers have to deal with an extra layer of quoting. Not\nworth it to save a few lines.\n\nThanks for a sanity check.\n\n-Peff\n"},{"id":"226608","messageId":"CAMP44s112mwgs=8n0XCnTtPM2V-O5RYE2ns+fjCvgkxj+kEY8Q@mail.gmail.com","threadId":"34829","inReplyTo":"20130903080358.GA30158@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-03T10:45:06Z","receivedAt":"2013-09-03T10:45:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 3, 2013 at 3:03 AM, Jeff King wrote:\n> On Tue, Sep 03, 2013 at 09:51:07AM +0200, SZEDER Gábor wrote:\n>\n>> > I wonder if we should have a:\n>> >\n>> > test_cmp_args () {\n>> > echo \"$1\" >expect &&\n>> > echo \"$1\" >actual &&\n>> > test_cmp expect actual\n>> > }\n>> >\n>> > to let these remain one-liners like:\n>> >\n>> > test_cmp_args \"$(git rev-parse start)\" \"$(git rev-parse final^1^1^1)\"\n>>\n>> This idea come up before, but there is one flaw which makes this\n>> function less useful: a non-zero exit code of the commands in the\n>> command substitutions would be lost.\n>\n> Good point. You'd probably have to do something gross with eval, like:\n>\n> test_cmp_args () {\n> eval \"$1\" >expect &&\n> eval \"$2\" >actual &&\n\nI don't see any reason to perpetuate these yoda comparisons.\n\neval \"$2\" >expect &&\neval \"$1\" >actual &&\n\n> test_cmp expect actual\n> }\n\n\n\n\n-- \nFelipe Contreras\n"},{"id":"226610","messageId":"20130903111006.GJ29840@goldbirke","threadId":"34829","inReplyTo":"CAMP44s112mwgs=8n0XCnTtPM2V-O5RYE2ns+fjCvgkxj+kEY8Q@mail.gmail.com","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-09-03T11:10:06Z","receivedAt":"2013-09-03T11:10:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 03, 2013 at 05:45:06AM -0500, Felipe Contreras wrote:\n> On Tue, Sep 3, 2013 at 3:03 AM, Jeff King wrote:\n> > On Tue, Sep 03, 2013 at 09:51:07AM +0200, SZEDER Gábor wrote:\n> >\n> >> > I wonder if we should have a:\n> >> >\n> >> > test_cmp_args () {\n> >> > echo \"$1\" >expect &&\n> >> > echo \"$1\" >actual &&\n> >> > test_cmp expect actual\n> >> > }\n> >> >\n> >> > to let these remain one-liners like:\n> >> >\n> >> > test_cmp_args \"$(git rev-parse start)\" \"$(git rev-parse final^1^1^1)\"\n> >>\n> >> This idea come up before, but there is one flaw which makes this\n> >> function less useful: a non-zero exit code of the commands in the\n> >> command substitutions would be lost.\n> >\n> > Good point. You'd probably have to do something gross with eval, like:\n> >\n> > test_cmp_args () {\n> > eval \"$1\" >expect &&\n> > eval \"$2\" >actual &&\n> \n> I don't see any reason to perpetuate these yoda comparisons.\n> \n> eval \"$2\" >expect &&\n> eval \"$1\" >actual &&\n\nI do. Your proposal requires the arguments in the reverse order\ncompared to test_cmp. That inconsistency would be far worse than\ntest_cmp_args \"$expect\" \"$actual\".\n\n> > test_cmp expect actual\n> > }\n\n\nBest,\nGábor\n"},{"id":"226619","messageId":"CAMP44s349-v6xtCvbDzycVj1wBwTdAgLmuGxB0pYn6CmHYkM1Q@mail.gmail.com","threadId":"34829","inReplyTo":"20130903111006.GJ29840@goldbirke","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-03T13:39:54Z","receivedAt":"2013-09-03T13:39:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 3, 2013 at 6:10 AM, SZEDER Gábor wrote:\n> On Tue, Sep 03, 2013 at 05:45:06AM -0500, Felipe Contreras wrote:\n>> On Tue, Sep 3, 2013 at 3:03 AM, Jeff King wrote:\n>> > On Tue, Sep 03, 2013 at 09:51:07AM +0200, SZEDER Gábor wrote:\n>> >\n>> >> > I wonder if we should have a:\n>> >> >\n>> >> > test_cmp_args () {\n>> >> > echo \"$1\" >expect &&\n>> >> > echo \"$1\" >actual &&\n>> >> > test_cmp expect actual\n>> >> > }\n>> >> >\n>> >> > to let these remain one-liners like:\n>> >> >\n>> >> > test_cmp_args \"$(git rev-parse start)\" \"$(git rev-parse final^1^1^1)\"\n>> >>\n>> >> This idea come up before, but there is one flaw which makes this\n>> >> function less useful: a non-zero exit code of the commands in the\n>> >> command substitutions would be lost.\n>> >\n>> > Good point. You'd probably have to do something gross with eval, like:\n>> >\n>> > test_cmp_args () {\n>> > eval \"$1\" >expect &&\n>> > eval \"$2\" >actual &&\n>>\n>> I don't see any reason to perpetuate these yoda comparisons.\n>>\n>> eval \"$2\" >expect &&\n>> eval \"$1\" >actual &&\n>\n> I do. Your proposal requires the arguments in the reverse order\n> compared to test_cmp. That inconsistency would be far worse than\n> test_cmp_args \"$expect\" \"$actual\".\n\nThere are two ways to fix an inconsistency, the other way is to fix\ntest_cmp. But that would be a change, and change is not welcome in\nGit.\n\nFor this reason alone I would prefer 'test \"$actual\" = expected', than\nthe yodaish 'test_cmp_args \"expected\" \"$actual\"'.\n\n-- \nFelipe Contreras\n"},{"id":"226622","messageId":"20130903150855.GK29840@goldbirke","threadId":"34829","inReplyTo":"CAMP44s349-v6xtCvbDzycVj1wBwTdAgLmuGxB0pYn6CmHYkM1Q@mail.gmail.com","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-09-03T15:08:55Z","receivedAt":"2013-09-03T15:08:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 03, 2013 at 08:39:54AM -0500, Felipe Contreras wrote:\n> On Tue, Sep 3, 2013 at 6:10 AM, SZEDER Gábor wrote:\n> > On Tue, Sep 03, 2013 at 05:45:06AM -0500, Felipe Contreras wrote:\n> >> On Tue, Sep 3, 2013 at 3:03 AM, Jeff King wrote:\n> >> > On Tue, Sep 03, 2013 at 09:51:07AM +0200, SZEDER Gábor wrote:\n> >> >\n> >> >> > I wonder if we should have a:\n> >> >> >\n> >> >> > test_cmp_args () {\n> >> >> > echo \"$1\" >expect &&\n> >> >> > echo \"$1\" >actual &&\n> >> >> > test_cmp expect actual\n> >> >> > }\n> >> >> >\n> >> >> > to let these remain one-liners like:\n> >> >> >\n> >> >> > test_cmp_args \"$(git rev-parse start)\" \"$(git rev-parse final^1^1^1)\"\n\nBTW, why not just use the already existing\n\n test_cmp_rev start final^1^1^1\n\nhelper function to get usable output on error and still keep these as\none-liners?\n\n> >> >> This idea come up before, but there is one flaw which makes this\n> >> >> function less useful: a non-zero exit code of the commands in the\n> >> >> command substitutions would be lost.\n> >> >\n> >> > Good point. You'd probably have to do something gross with eval, like:\n> >> >\n> >> > test_cmp_args () {\n> >> > eval \"$1\" >expect &&\n> >> > eval \"$2\" >actual &&\n> >>\n> >> I don't see any reason to perpetuate these yoda comparisons.\n> >>\n> >> eval \"$2\" >expect &&\n> >> eval \"$1\" >actual &&\n> >\n> > I do. Your proposal requires the arguments in the reverse order\n> > compared to test_cmp. That inconsistency would be far worse than\n> > test_cmp_args \"$expect\" \"$actual\".\n> \n> There are two ways to fix an inconsistency, the other way is to fix\n> test_cmp. But that would be a change, and change is not welcome in\n> Git.\n\nIt depends on the change, I suppose. I agree, changing 3k+ lines just\nto avoid yoda conditions... I doubt the gain worth the code churn.\n\n\nBest,\nGábor\n"},{"id":"226631","messageId":"20130903170419.GA29921@google.com","threadId":"34829","inReplyTo":"20130903150855.GK29840@goldbirke","subject":"[PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-03T17:04:19Z","receivedAt":"2013-09-03T17:04:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n> On Tue, Sep 03, 2013 at 08:39:54AM -0500, Felipe Contreras wrote:\n\n>> There are two ways to fix an inconsistency, the other way is to fix\n>> test_cmp. But that would be a change, and change is not welcome in\n>> Git.\n>\n> It depends on the change, I suppose. I agree, changing 3k+ lines just\n> to avoid yoda conditions... I doubt the gain worth the code churn.\n\nEspecially when the idiom being changed is not even being made better.\n;-)\n\ntest_cmp_rev follows the same order of arguments a \"diff -u\" and\nproduces the same output as plain \"git diff\". It's perfectly readable\nand normal. I think Felipe is pushing buttons and testing boundaries.\n\nBut in the process came a report of a missing test_cmp_rev use, which\nis useful. Patch follows.\n\nWhile at it, I rerolled the other patches from the series to clarify\ntheir commit messages (replacing \"fix \" with a fuller\ndescription).\n\nImprovements welcome, as always. Thanks.\n\nFelipe Contreras (3):\n rev-parse test: modernize quoting and whitespace\n rev-parse test: use test_must_fail, not \"if ; then false; fi\"\n rev-parse test: use standard test functions for setup\n\nJonathan Nieder (1):\n rev-parse test: use test_cmp instead of \"test\" builtin\n\n t/t6101-rev-parse-parents.sh | 110 +++++++++++++++++++++++++++++++------------\n 1 file changed, 79 insertions(+), 31 deletions(-)\n"},{"id":"226632","messageId":"20130903170532.GB29921@google.com","threadId":"34829","inReplyTo":"20130903170419.GA29921@google.com","subject":"[PATCH 1/4] rev-parse test: modernize quoting and whitespace","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-03T17:05:32Z","receivedAt":"2013-09-03T17:05:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Felipe Contreras \n\nInstead of cramming everything in one line, put the test body in an\nindented block after the opening test_expect_success line and quote\nand put the closing quote on a line by itself.\n\nUse single-quote instead of double-quote to quote the test body\nfor more useful --verbose output.\n\nSigned-off-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\n t/t6101-rev-parse-parents.sh | 74 ++++++++++++++++++++++++++++++++++----------\n 1 file changed, 58 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex e673c25..c47b869 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -17,21 +17,62 @@ hide_error save_tag start2 unique_commit \"start2\" tree\n save_tag two_parents unique_commit \"next\" tree -p second -p start2\n save_tag final unique_commit \"final\" tree -p two_parents\n \n-test_expect_success 'start is valid' 'git rev-parse start | grep \"^[0-9a-f]\\{40\\}$\"'\n-test_expect_success 'start^0' \"test $(cat .git/refs/tags/start) = $(git rev-parse start^0)\"\n-test_expect_success 'start^1 not valid' \"if git rev-parse --verify start^1; then false; else :; fi\"\n-test_expect_success 'second^1 = second^' \"test $(git rev-parse second^1) = $(git rev-parse second^)\"\n-test_expect_success 'final^1^1^1' \"test $(git rev-parse start) = $(git rev-parse final^1^1^1)\"\n-test_expect_success 'final^1^1^1 = final^^^' \"test $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)\"\n-test_expect_success 'final^1^2' \"test $(git rev-parse start2) = $(git rev-parse final^1^2)\"\n-test_expect_success 'final^1^2 != final^1^1' \"test $(git rev-parse final^1^2) != $(git rev-parse final^1^1)\"\n-test_expect_success 'final^1^3 not valid' \"if git rev-parse --verify final^1^3; then false; else :; fi\"\n-test_expect_success '--verify start2^1' 'test_must_fail git rev-parse --verify start2^1'\n-test_expect_success '--verify start2^0' 'git rev-parse --verify start2^0'\n-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)\\\"\"\n-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)\\\"\"\n-\n-test_expect_success 'repack for next test' 'git repack -a -d'\n+test_expect_success 'start is valid' '\n+\tgit rev-parse start | grep \"^[0-9a-f]\\{40\\}$\"\n+'\n+\n+test_expect_success 'start^0' '\n+\ttest $(cat .git/refs/tags/start) = $(git rev-parse start^0)\n+'\n+\n+test_expect_success 'start^1 not valid' '\n+\tif git rev-parse --verify start^1; then false; else :; fi\n+'\n+\n+test_expect_success 'second^1 = second^' '\n+\ttest $(git rev-parse second^1) = $(git rev-parse second^)\n+'\n+\n+test_expect_success 'final^1^1^1' '\n+\ttest $(git rev-parse start) = $(git rev-parse final^1^1^1)\n+'\n+\n+test_expect_success 'final^1^1^1 = final^^^' '\n+\ttest $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)\n+'\n+\n+test_expect_success 'final^1^2' '\n+\ttest $(git rev-parse start2) = $(git rev-parse final^1^2)\n+'\n+\n+test_expect_success 'final^1^2 != final^1^1' '\n+\ttest $(git rev-parse final^1^2) != $(git rev-parse final^1^1)\n+'\n+\n+test_expect_success 'final^1^3 not valid' '\n+\tif git rev-parse --verify final^1^3; then false; else :; fi\n+'\n+\n+test_expect_success '--verify start2^1' '\n+\ttest_must_fail git rev-parse --verify start2^1\n+'\n+\n+test_expect_success '--verify start2^0' '\n+\tgit rev-parse --verify start2^0\n+'\n+\n+test_expect_success 'final^1^@ = final^1^1 final^1^2' '\n+\ttest \"$(git rev-parse final^1^@)\" = \"$(git rev-parse final^1^1 final^1^2)\"\n+'\n+\n+test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' '\n+\ttest \"$(git rev-parse final^1^!)\" = \"$(git rev-parse final^1 ^final^1^1 ^final^1^2)\"\n+'\n+\n+test_expect_success 'repack for next test' '\n+\tgit repack -a -d\n+'\n+\n test_expect_success 'short SHA-1 works' '\n \tstart=`git rev-parse --verify start` &&\n \techo $start &&\n@@ -39,6 +80,7 @@ test_expect_success 'short SHA-1 works' '\n \techo $abbrv &&\n \tabbrv=`git rev-parse --verify $abbrv` &&\n \techo $abbrv &&\n-\ttest $start = $abbrv'\n+\ttest $start = $abbrv\n+'\n \n test_done\n-- \n1.8.4\n"},{"id":"226633","messageId":"20130903170617.GC29921@google.com","threadId":"34829","inReplyTo":"20130903170419.GA29921@google.com","subject":"[PATCH 2/4] rev-parse test: use test_must_fail, not \"if ; then false; fi\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-03T17:06:18Z","receivedAt":"2013-09-03T17:06:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Felipe Contreras \n\nThis way, if rev-parse segfaults then the test will fail instead\nof treating it the same way as a controlled failure.\n\nSigned-off-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\n t/t6101-rev-parse-parents.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex c47b869..416067c 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -26,7 +26,7 @@ test_expect_success 'start^0' '\n '\n \n test_expect_success 'start^1 not valid' '\n-\tif git rev-parse --verify start^1; then false; else :; fi\n+\ttest_must_fail git rev-parse --verify start^1\n '\n \n test_expect_success 'second^1 = second^' '\n@@ -50,7 +50,7 @@ test_expect_success 'final^1^2 != final^1^1' '\n '\n \n test_expect_success 'final^1^3 not valid' '\n-\tif git rev-parse --verify final^1^3; then false; else :; fi\n+\ttest_must_fail git rev-parse --verify final^1^3\n '\n \n test_expect_success '--verify start2^1' '\n-- \n1.8.4\n"},{"id":"226635","messageId":"20130903170715.GD29921@google.com","threadId":"34829","inReplyTo":"20130903170419.GA29921@google.com","subject":"[PATCH 3/4] rev-parse test: use test_cmp instead of \"test\" builtin","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-03T17:07:15Z","receivedAt":"2013-09-03T17:07:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Use test_cmp instead of passing two command substitutions to the\n\"test\" builtin. This way:\n\n - when tests fail, they can print a helpful diff if run with\n \"--verbose\"\n\n - the argument order \"test_cmp expect actual\" feels natural,\n unlike test = that seems a little backwards\n\n - the exit status from invoking git is checked, so if rev-parse\n starts segfaulting then the test will notice and fail\n\nUse a custom function for this instead of test_cmp_rev to emphasize\nthat we are testing the output from \"git rev-parse\" with certain\narguments, not checking that the revisions are equal in abstract.\n\nReported-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\n t/t6101-rev-parse-parents.sh | 33 +++++++++++++++++++--------------\n 1 file changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex 416067c..8a6ff66 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -8,6 +8,12 @@ test_description='Test git rev-parse with different parent options'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-t6000.sh # t6xxx specific functions\n \n+test_cmp_rev_output () {\n+\tgit rev-parse --verify \"$1\" >expect &&\n+\teval \"$2\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n date >path0\n git update-index --add path0\n save_tag tree git write-tree\n@@ -22,7 +28,7 @@ test_expect_success 'start is valid' '\n '\n \n test_expect_success 'start^0' '\n-\ttest $(cat .git/refs/tags/start) = $(git rev-parse start^0)\n+\ttest_cmp_rev_output tags/start \"git rev-parse start^0\"\n '\n \n test_expect_success 'start^1 not valid' '\n@@ -30,19 +36,19 @@ test_expect_success 'start^1 not valid' '\n '\n \n test_expect_success 'second^1 = second^' '\n-\ttest $(git rev-parse second^1) = $(git rev-parse second^)\n+\ttest_cmp_rev_output second^ \"git rev-parse second^1\"\n '\n \n test_expect_success 'final^1^1^1' '\n-\ttest $(git rev-parse start) = $(git rev-parse final^1^1^1)\n+\ttest_cmp_rev_output start \"git rev-parse final^1^1^1\"\n '\n \n test_expect_success 'final^1^1^1 = final^^^' '\n-\ttest $(git rev-parse final^1^1^1) = $(git rev-parse final^^^)\n+\ttest_cmp_rev_output final^^^ \"git rev-parse final^1^1^1\"\n '\n \n test_expect_success 'final^1^2' '\n-\ttest $(git rev-parse start2) = $(git rev-parse final^1^2)\n+\ttest_cmp_rev_output start2 \"git rev-parse final^1^2\"\n '\n \n test_expect_success 'final^1^2 != final^1^1' '\n@@ -62,11 +68,15 @@ test_expect_success '--verify start2^0' '\n '\n \n test_expect_success 'final^1^@ = final^1^1 final^1^2' '\n-\ttest \"$(git rev-parse final^1^@)\" = \"$(git rev-parse final^1^1 final^1^2)\"\n+\tgit rev-parse final^1^1 final^1^2 >expect &&\n+\tgit rev-parse final^1^@ >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' '\n-\ttest \"$(git rev-parse final^1^!)\" = \"$(git rev-parse final^1 ^final^1^1 ^final^1^2)\"\n+\tgit rev-parse final^1 ^final^1^1 ^final^1^2 >expect &&\n+\tgit rev-parse final^1^! >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'repack for next test' '\n@@ -74,13 +84,8 @@ test_expect_success 'repack for next test' '\n '\n \n test_expect_success 'short SHA-1 works' '\n-\tstart=`git rev-parse --verify start` &&\n-\techo $start &&\n-\tabbrv=`echo $start | sed s/.\\$//` &&\n-\techo $abbrv &&\n-\tabbrv=`git rev-parse --verify $abbrv` &&\n-\techo $abbrv &&\n-\ttest $start = $abbrv\n+\tstart=$(git rev-parse --verify start) &&\n+\ttest_cmp_rev_output start \"git rev-parse ${start%?}\"\n '\n \n test_done\n-- \n1.8.4\n"},{"id":"226636","messageId":"20130903171137.GE29921@google.com","threadId":"34829","inReplyTo":"20130903170419.GA29921@google.com","subject":"[PATCH 4/4] rev-parse test: use standard test functions for setup","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-03T17:11:37Z","receivedAt":"2013-09-03T17:11:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Felipe Contreras \n\nSave the reader from learning specialized t6* setup functions\nwhere familiar commands like test_commit, \"git checkout --orphan\",\nand \"git merge\" will do.\n\nWhile at it, wrap the setup commands in a test assertion so errors can\nbe caught and stray output suppressed when running without --verbose\nas in other tests.\n\nSigned-off-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\nThanks for reading.\n\n t/t6101-rev-parse-parents.sh | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex 8a6ff66..b5fa9e4 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -14,14 +14,15 @@ test_cmp_rev_output () {\n \ttest_cmp expect actual\n }\n \n-date >path0\n-git update-index --add path0\n-save_tag tree git write-tree\n-hide_error save_tag start unique_commit \"start\" tree\n-save_tag second unique_commit \"second\" tree -p start\n-hide_error save_tag start2 unique_commit \"start2\" tree\n-save_tag two_parents unique_commit \"next\" tree -p second -p start2\n-save_tag final unique_commit \"final\" tree -p two_parents\n+test_expect_success 'setup' '\n+\ttest_commit start &&\n+\ttest_commit second &&\n+\tgit checkout --orphan tmp &&\n+\ttest_commit start2 &&\n+\tgit checkout master &&\n+\tgit merge -m next start2 &&\n+\ttest_commit final\n+'\n \n test_expect_success 'start is valid' '\n \tgit rev-parse start | grep \"^[0-9a-f]\\{40\\}$\"\n-- \n1.8.4\n"},{"id":"226638","messageId":"20130903171546.GF29921@google.com","threadId":"34829","inReplyTo":"20130903171137.GE29921@google.com","subject":"[PATCH v2 4/4] rev-parse test: use standard test functions for setup","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-03T17:15:46Z","receivedAt":"2013-09-03T17:15:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Felipe Contreras \n\nSave the reader from learning specialized t6* setup functions\nwhere familiar commands like test_commit, \"git checkout --orphan\",\nand \"git merge\" will do.\n\nWhile at it, wrap the setup commands in a test assertion so errors can\nbe caught and stray output suppressed when running without --verbose\nas in other tests.\n\nSigned-off-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\nLost a line from the patch last time (removal of the\n'. \"$TEST_DIRECTORY\"/lib-t6000.sh' directive. Sorry about that ---\nfixed now.\n\n t/t6101-rev-parse-parents.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex 8a6ff66..7ea14ce 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -6,7 +6,6 @@\n test_description='Test git rev-parse with different parent options'\n \n . ./test-lib.sh\n-. \"$TEST_DIRECTORY\"/lib-t6000.sh # t6xxx specific functions\n \n test_cmp_rev_output () {\n \tgit rev-parse --verify \"$1\" >expect &&\n@@ -14,14 +13,15 @@ test_cmp_rev_output () {\n \ttest_cmp expect actual\n }\n \n-date >path0\n-git update-index --add path0\n-save_tag tree git write-tree\n-hide_error save_tag start unique_commit \"start\" tree\n-save_tag second unique_commit \"second\" tree -p start\n-hide_error save_tag start2 unique_commit \"start2\" tree\n-save_tag two_parents unique_commit \"next\" tree -p second -p start2\n-save_tag final unique_commit \"final\" tree -p two_parents\n+test_expect_success 'setup' '\n+\ttest_commit start &&\n+\ttest_commit second &&\n+\tgit checkout --orphan tmp &&\n+\ttest_commit start2 &&\n+\tgit checkout master &&\n+\tgit merge -m next start2 &&\n+\ttest_commit final\n+'\n \n test_expect_success 'start is valid' '\n \tgit rev-parse start | grep \"^[0-9a-f]\\{40\\}$\"\n-- \n1.8.4\n"},{"id":"226640","messageId":"20130903172046.GA1050@sigill.intra.peff.net","threadId":"34829","inReplyTo":"20130903170419.GA29921@google.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T17:20:47Z","receivedAt":"2013-09-03T17:20:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 03, 2013 at 10:04:19AM -0700, Jonathan Nieder wrote:\n\n> > It depends on the change, I suppose. I agree, changing 3k+ lines just\n> > to avoid yoda conditions... I doubt the gain worth the code churn.\n> \n> Especially when the idiom being changed is not even being made better.\n> ;-)\n\nYes. IMHO it is not just \"not worth the churn\" but actively making the\ncode less readable.\n\n> While at it, I rerolled the other patches from the series to clarify\n> their commit messages (replacing \"fix \" with a fuller\n> description).\n\nThe series looks fine to me, modulo the fix up in v2 of 4/4.\n\n-Peff\n"},{"id":"226643","messageId":"xmqq8uzdu817.fsf@gitster.dls.corp.google.com","threadId":"34829","inReplyTo":"CAMP44s349-v6xtCvbDzycVj1wBwTdAgLmuGxB0pYn6CmHYkM1Q@mail.gmail.com","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T17:31:00Z","receivedAt":"2013-09-03T17:31:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> There are two ways to fix an inconsistency, the other way is to fix\n> test_cmp. But that would be a change, and change is not welcome in\n> Git.\n\nIf you want to do \"test_cmp $actual $expect\", you would have to\nfirst \"fix\" people's expectation that \"diff A B\" produces a change\nnecessary to bring A to B, which would not likely to happen. We do\nthe 'test_cmp expect actual' for a reason.\n"},{"id":"226644","messageId":"xmqq4na1u7wk.fsf@gitster.dls.corp.google.com","threadId":"34829","inReplyTo":"xmqq8uzdu817.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T17:33:47Z","receivedAt":"2013-09-03T17:33:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Felipe Contreras writes:\n>\n>> There are two ways to fix an inconsistency, the other way is to fix\n>> test_cmp. But that would be a change, and change is not welcome in\n>> Git.\n>\n> If you want to do \"test_cmp $actual $expect\", you would have to\n> first \"fix\" people's expectation that \"diff A B\" produces a change\n> necessary to bring A to B, which would not likely to happen. We do\n> the 'test_cmp expect actual' for a reason.\n\nBy the way, I found the other changes (especially the last one) very\nsensible.\n"},{"id":"226664","messageId":"xmqqk3ixr7xy.fsf@gitster.dls.corp.google.com","threadId":"34829","inReplyTo":"20130903170715.GD29921@google.com","subject":"Re: [PATCH 3/4] rev-parse test: use test_cmp instead of \"test\" builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T20:01:13Z","receivedAt":"2013-09-03T20:01:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Use test_cmp instead of passing two command substitutions to the\n> \"test\" builtin. This way:\n>\n> - when tests fail, they can print a helpful diff if run with\n> \"--verbose\"\n>\n> - the argument order \"test_cmp expect actual\" feels natural,\n> unlike test = that seems a little backwards\n\nI do not mind to drop s/a little // here, by the way.\n\n> - the exit status from invoking git is checked, so if rev-parse\n> starts segfaulting then the test will notice and fail\n>\n> Use a custom function for this instead of test_cmp_rev to emphasize\n> that we are testing the output from \"git rev-parse\" with certain\n> arguments, not checking that the revisions are equal in abstract.\n>\n> Reported-by: Felipe Contreras \n> Signed-off-by: Jonathan Nieder \n> ---\n> t/t6101-rev-parse-parents.sh | 33 +++++++++++++++++++--------------\n> 1 file changed, 19 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\n> index 416067c..8a6ff66 100755\n> --- a/t/t6101-rev-parse-parents.sh\n> +++ b/t/t6101-rev-parse-parents.sh\n> @@ -8,6 +8,12 @@ test_description='Test git rev-parse with different parent options'\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-t6000.sh # t6xxx specific functions\n> \n> +test_cmp_rev_output () {\n> +\tgit rev-parse --verify \"$1\" >expect &&\n> +\teval \"$2\" >actual &&\n> +\ttest_cmp expect actual\n> +}\n\nAfter applying this patch and running \"git show | grep test_cmp_rev_output\",\nI notice that the second is always \"git rev-parse \". Do\nwe still need to eval these, or would it be sufficient to do\n\n test_cmp_rev_output () {\n git rev-parse --verify \"$1\" >expect &&\n git rev-parse --verify \"$2\" >actual &&\n test_cmp expect actual\n }\n\nhere, and make users like so:\n\n\t-\ttest_cmp_rev_output tags/start \"git rev-parse start^0\"\n\t+\ttest_cmp_rev_output tags/start start^0\n\nAm I missing something?\n"},{"id":"226680","messageId":"CAMP44s0zRZDGwd6KcVi81JEDQv38THCJctA8zUZiYht-uqY4vA@mail.gmail.com","threadId":"34829","inReplyTo":"xmqq8uzdu817.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-03T21:52:19Z","receivedAt":"2013-09-03T21:52:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 3, 2013 at 12:31 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> There are two ways to fix an inconsistency, the other way is to fix\n>> test_cmp. But that would be a change, and change is not welcome in\n>> Git.\n>\n> If you want to do \"test_cmp $actual $expect\", you would have to\n> first \"fix\" people's expectation that \"diff A B\" produces a change\n> necessary to bring A to B, which would not likely to happen. We do\n> the 'test_cmp expect actual' for a reason.\n\nNo, you just do \"diff B A\".\n\n-- \nFelipe Contreras\n"},{"id":"226681","messageId":"CAMP44s1mnz2A20bNo2um9BhO=b3WOtOGWundXuH+5nYNNDcKxA@mail.gmail.com","threadId":"34829","inReplyTo":"20130903170419.GA29921@google.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-03T21:53:25Z","receivedAt":"2013-09-03T21:53:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 3, 2013 at 12:04 PM, Jonathan Nieder wrote:\n> SZEDER Gábor wrote:\n>> On Tue, Sep 03, 2013 at 08:39:54AM -0500, Felipe Contreras wrote:\n>\n>>> There are two ways to fix an inconsistency, the other way is to fix\n>>> test_cmp. But that would be a change, and change is not welcome in\n>>> Git.\n>>\n>> It depends on the change, I suppose. I agree, changing 3k+ lines just\n>> to avoid yoda conditions... I doubt the gain worth the code churn.\n>\n> Especially when the idiom being changed is not even being made better.\n> ;-)\n\nBut it is better.\n\n> test_cmp_rev follows the same order of arguments a \"diff -u\" and\n> produces the same output as plain \"git diff\". It's perfectly readable\n> and normal. I think Felipe is pushing buttons and testing boundaries.\n\nThose are irrelevant implementation details.\n\n-- \nFelipe Contreras\n"},{"id":"226682","messageId":"CAMP44s2nssnYROFEPaGKLy0f4cSqg63ejpcwKdSxEL6nvv8y0w@mail.gmail.com","threadId":"34829","inReplyTo":"20130903170617.GC29921@google.com","subject":"Re: [PATCH 2/4] rev-parse test: use test_must_fail, not \"if ; then false; fi\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-03T21:56:25Z","receivedAt":"2013-09-03T21:56:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"> rev-parse test: use test_must_fail, not \"if ; then false; fi\"\n\nThat is way more than 50 characters, and doesn't tell me why you want\nto do that.\n\n-- \nFelipe Contreras\n"},{"id":"226683","messageId":"CAMP44s0a38N4ruQtgzcAR7e0H9pUscSA1hA355-QE64ZaN8cKw@mail.gmail.com","threadId":"34829","inReplyTo":"20130903170715.GD29921@google.com","subject":"Re: [PATCH 3/4] rev-parse test: use test_cmp instead of \"test\" builtin","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-03T22:01:18Z","receivedAt":"2013-09-03T22:01:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 3, 2013 at 12:07 PM, Jonathan Nieder wrote:\n\n> test_expect_success 'start^0' '\n> - test $(cat .git/refs/tags/start) = $(git rev-parse start^0)\n> + test_cmp_rev_output tags/start \"git rev-parse start^0\"\n> '\n\nBackwards and yodaish this is.\n\n-- \nFelipe Contreras\n"},{"id":"226696","messageId":"20130904042825.GA2661@elie.Belkin","threadId":"34829","inReplyTo":"xmqqk3ixr7xy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4] rev-parse test: use test_cmp instead of \"test\" builtin","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-04T04:28:26Z","receivedAt":"2013-09-04T04:28:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> After applying this patch and running \"git show | grep test_cmp_rev_output\",\n> I notice that the second is always \"git rev-parse \". Do\n> we still need to eval these, or would it be sufficient to do\n>\n> test_cmp_rev_output () {\n> git rev-parse --verify \"$1\" >expect &&\n> git rev-parse --verify \"$2\" >actual &&\n> test_cmp expect actual\n> }\n>\n> here, and make users like so:\n>\n>\t-\ttest_cmp_rev_output tags/start \"git rev-parse start^0\"\n>\t+\ttest_cmp_rev_output tags/start start^0\n>\n> Am I missing something?\n\nI was tempted to use test_cmp_rev, which would have the same effect.\nThe original test checked the output of \"git rev-parse start^0\", while\nthe test as modified above checks the output of \"git rev-parse\n--verify start^0\".\n\nI slightly prefer the version without --verify because \"git rev-parse\n--verify\" is well exercised elsewhere in the testsuite (but then, so\nis rev-parse without --verify, so it's not a big deal).\n\nAbbreviating as follows\n\n\tfoo () {\n\t\tgit rev-parse --verify \"$1\" >expect &&\n\t\tgit rev-parse \"$2\" >actual &&\n\t\ttest_cmp expect actual\n\t}\n\nwould also be fine with me. All it would take is a good replacement\nfor the placeholder \"foo\". The added flexibility of \"compare a rev\nto the output of an arbitrary command\" doesn't get used.\n\nJonathan\n"},{"id":"226739","messageId":"xmqqli3co7ov.fsf@gitster.dls.corp.google.com","threadId":"34829","inReplyTo":"20130903170419.GA29921@google.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-04T16:47:12Z","receivedAt":"2013-09-04T16:47:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> test_cmp_rev follows the same order of arguments a \"diff -u\" and\n> produces the same output as plain \"git diff\". It's perfectly readable\n> and normal.\n\nThis is way off tangent, but I am somewhat sympathetic to Felipe's\n\"compare actual with expect\", with reservations.\n\nIf one ignores all other constraints and focuses solely on the order\nof argument test_cmp takes, one would realize that it is backwards.\nA(ny) sanely defined \"compare A with B\" function should yield the\nresult of subtracting B from A, i.e. cmp(A,B) should be like (A-B).\nThat is what you feed qsort() and bsearch() (it is not limited to C;\nyou see the same in \"sort { $a <=> $b }\"). The definition naturally\nmakes \"cmp(A,B) < 0\" like \"A < B\" and \"cmp(A,B) > 0\" like \"A > B\".\n\nBut unfortunately, test_cmp is defined in terms of \"diff -u\" by\nfeeding its parameters in the same order as given. \"test_cmp A B\"\njust turns into \"diff -u A B\".\n\nNow, we _do_ want to see \"diff -u expect actual\", so that in the\noutput, what is _missing_ in the actual output compared to the\nexpected output is marked with \"-\", and what is _excess_ is marked\nwith \"+\". If you think about it, this output from \"diff -u expect\nactual\" is giving us the result of subtracting expect from actual.\n\nIf we want to express it in terms of \"cmp\", we should be writing\n\"cmp(actual,expect)\", not \"cmp(expect,actual)\". The latter is to\nsubtract actual from expect, which is backwards.\n\nIt would have been better if a wrapper around \"diff\" to give us a\nhigher level \"comparison\" semantics, test_cmp, had been written in\nsuch a way that hid this backward behaviour of \"diff\", when it was\nintroduced to replace hardcoded \"diff -u\".\n\nI'd actually be ecstatic if one morning when I get up, I find that\ntest_cmp implementation were replaced with (the moral equivalent of)\n\n\ttest_cmp () {\n\t\tdiff -u \"$2\" \"$1\"\n\t}\n\nand all the callsites of test_cmp in the test scripts in all the\ntopic branches in flight were also swapped to \"test_cmp actual\nexpect\" without anybody having to worry about mismerges, merge\nconflicts and confusing people who are used to the current order for\nthe past 5 years.\n\nBut I do not think that is going to happen without a careful\nplanning. We may be able to manage the code somehow (we can drop\nall the topics in flight immediately after a release, apply the\n\"swap the order\" big patch, and rebase all the topics on top), but\nretraining people would not be instantaneous \"flag day\" event, and\nwe will see mistakes for a few months after doing so.\n\nOh, well.\n"},{"id":"226741","messageId":"20130904171356.GD2582@serenity.lan","threadId":"34829","inReplyTo":"xmqqli3co7ov.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-04T17:13:57Z","receivedAt":"2013-09-04T17:13:57Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Sep 04, 2013 at 09:47:12AM -0700, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n> \n> > test_cmp_rev follows the same order of arguments a \"diff -u\" and\n> > produces the same output as plain \"git diff\". It's perfectly readable\n> > and normal.\n> \n> This is way off tangent, but I am somewhat sympathetic to Felipe's\n> \"compare actual with expect\", with reservations.\n\nThis isn't an argument either way, but note that JUnit (and NUnit and\nPHPUnit) all have assertEquals methods that take the arguments in the\norder \"expect, actual\". I've always assumed that Git's test framework\nwas imitating that, although I have no idea why that particular order is\nchosen and is so common.\n"},{"id":"226744","messageId":"xmqq1u54o5c4.fsf@gitster.dls.corp.google.com","threadId":"34829","inReplyTo":"20130904171356.GD2582@serenity.lan","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-04T17:38:03Z","receivedAt":"2013-09-04T17:38:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> On Wed, Sep 04, 2013 at 09:47:12AM -0700, Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>> \n>> > test_cmp_rev follows the same order of arguments a \"diff -u\" and\n>> > produces the same output as plain \"git diff\". It's perfectly readable\n>> > and normal.\n>> \n>> This is way off tangent, but I am somewhat sympathetic to Felipe's\n>> \"compare actual with expect\", with reservations.\n>\n> This isn't an argument either way, but note that JUnit (and NUnit and\n> PHPUnit) all have assertEquals methods that take the arguments in the\n> order \"expect, actual\". I've always assumed that Git's test framework\n> was imitating that,...\n\nNo. See 82ebb0b6 (add test_cmp function for test scripts,\n2008-03-12). The \"test_cmp\" was a replacement for \"diff -u\", and\nthe same order we fed \"diff -u\", i.e. expect then actual, was\ncarried over.\n"},{"id":"226747","messageId":"20130904183559.GA3465@sigill.intra.peff.net","threadId":"34829","inReplyTo":"xmqq1u54o5c4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-04T18:36:00Z","receivedAt":"2013-09-04T18:36:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 04, 2013 at 10:38:03AM -0700, Junio C Hamano wrote:\n\n> >> This is way off tangent, but I am somewhat sympathetic to Felipe's\n> >> \"compare actual with expect\", with reservations.\n> >\n> > This isn't an argument either way, but note that JUnit (and NUnit and\n> > PHPUnit) all have assertEquals methods that take the arguments in the\n> > order \"expect, actual\". I've always assumed that Git's test framework\n> > was imitating that,...\n> \n> No. See 82ebb0b6 (add test_cmp function for test scripts,\n> 2008-03-12). The \"test_cmp\" was a replacement for \"diff -u\", and\n> the same order we fed \"diff -u\", i.e. expect then actual, was\n> carried over.\n\nI don't think it was intentional at the time. But over the intervening 5\nyears, I have noticed that I certainly think of \"test_cmp A B\" as\n\"differences from A to B\", and the order makes sense. IOW, the \"test_cmp\nis diff\" abstraction is leaky, and that is fine (if it were not leaky,\nthen order would not matter at all, but it clearly does).\n\nBut let's take a step back. This seems like an endian-ness issue to me.\nI.e., some people prefer one order for test assertions, and other people\nprefer the other. Is anyone actually right, or is this simply a matter\nof preference? And if it is simply a matter of preference, then why\nbother going through the pain of changing the current project standard?\n\nThough I prefer the current, I can certainly live and adapt to a changed\nstandard, and I do not mind doing so if there is a good reason. But I've\nyet to see any argument beyond \"it is not what I like\". Which to me\nargues for the status quo as the path of least resistance.\n\n-Peff\n"},{"id":"226756","messageId":"xmqqob88mmax.fsf@gitster.dls.corp.google.com","threadId":"34829","inReplyTo":"20130904183559.GA3465@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-04T19:14:30Z","receivedAt":"2013-09-04T19:14:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 04, 2013 at 10:38:03AM -0700, Junio C Hamano wrote:\n>\n>> >> This is way off tangent, but I am somewhat sympathetic to Felipe's\n>> >> \"compare actual with expect\", with reservations.\n>> >\n>> > This isn't an argument either way, but note that JUnit (and NUnit and\n>> > PHPUnit) all have assertEquals methods that take the arguments in the\n>> > order \"expect, actual\". I've always assumed that Git's test framework\n>> > was imitating that,...\n>> \n>> No. See 82ebb0b6 (add test_cmp function for test scripts,\n>> 2008-03-12). The \"test_cmp\" was a replacement for \"diff -u\", and\n>> the same order we fed \"diff -u\", i.e. expect then actual, was\n>> carried over.\n>\n> I don't think it was intentional at the time.\n\nI do not think so, either. The only thing we cared about was \"are\nthey equal\". And from the point of view of test_cmp exit status,\nthat still the only thing we care about. Comparison to see which is\ngreater is a superset of equality check we needed, and in that\ncontext, the order did not and does not matter.\n\nOne only starts to notice the order of comparison when one starts\nthinking about the comparison in terms of \"what is subtracted from\nwhat\", and at that point, one realizes that \"diff A B\" that gives us\nwhat was lost from A to B as \"-\" and what was gained in B relative\nto A as \"+\" is showing the result of subtracting A from B. And that\nsubtraction is backwards from the cmp(A,B) that subtracts B from A,\nwhich is the usual convention.\n\n> Though I prefer the current, I can certainly live and adapt to a changed\n> standard, and I do not mind doing so if there is a good reason. But I've\n> yet to see any argument beyond \"it is not what I like\". Which to me\n> argues for the status quo as the path of least resistance.\n\nAs I think test_cmp is primarily about equality comparison, I do not\nthink it is worth switching and adapting. Switching makes sense\nonly in my dreams where we did not have to worry about in-flight\ntopics and people's brains that are used to the current order.\n\nThat is exactly where my \"with reservations\" came from.\n"},{"id":"227055","messageId":"CAMP44s3O=cHAtHOj41MccBDOausukb49MV-E6jib6n6czs+p3Q@mail.gmail.com","threadId":"34829","inReplyTo":"20130904183559.GA3465@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-08T03:11:49Z","receivedAt":"2013-09-08T03:11:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Sep 4, 2013 at 1:36 PM, Jeff King wrote:\n> On Wed, Sep 04, 2013 at 10:38:03AM -0700, Junio C Hamano wrote:\n>\n>> >> This is way off tangent, but I am somewhat sympathetic to Felipe's\n>> >> \"compare actual with expect\", with reservations.\n>> >\n>> > This isn't an argument either way, but note that JUnit (and NUnit and\n>> > PHPUnit) all have assertEquals methods that take the arguments in the\n>> > order \"expect, actual\". I've always assumed that Git's test framework\n>> > was imitating that,...\n>>\n>> No. See 82ebb0b6 (add test_cmp function for test scripts,\n>> 2008-03-12). The \"test_cmp\" was a replacement for \"diff -u\", and\n>> the same order we fed \"diff -u\", i.e. expect then actual, was\n>> carried over.\n>\n> I don't think it was intentional at the time. But over the intervening 5\n> years, I have noticed that I certainly think of \"test_cmp A B\" as\n> \"differences from A to B\", and the order makes sense. IOW, the \"test_cmp\n> is diff\" abstraction is leaky, and that is fine (if it were not leaky,\n> then order would not matter at all, but it clearly does).\n>\n> But let's take a step back. This seems like an endian-ness issue to me.\n> I.e., some people prefer one order for test assertions, and other people\n> prefer the other. Is anyone actually right, or is this simply a matter\n> of preference? And if it is simply a matter of preference, then why\n> bother going through the pain of changing the current project standard?\n>\n> Though I prefer the current, I can certainly live and adapt to a changed\n> standard, and I do not mind doing so if there is a good reason. But I've\n> yet to see any argument beyond \"it is not what I like\". Which to me\n> argues for the status quo as the path of least resistance.\n\nDidn't Junio already provided reasoning?\n\nHere's more; human semantics:\n\nComputer, compare A with B\ncmp(A, B)\n\nWhy would I write?\n\ncmp(B, A)\n\nCould you even construct an English sentence that starts with B, and then A?\n\n-- \nFelipe Contreras\n"},{"id":"227058","messageId":"20130908040615.GA14019@sigill.intra.peff.net","threadId":"34829","inReplyTo":"CAMP44s3O=cHAtHOj41MccBDOausukb49MV-E6jib6n6czs+p3Q@mail.gmail.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T04:06:16Z","receivedAt":"2013-09-08T04:06:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 07, 2013 at 10:11:49PM -0500, Felipe Contreras wrote:\n\n> > Though I prefer the current, I can certainly live and adapt to a changed\n> > standard, and I do not mind doing so if there is a good reason. But I've\n> > yet to see any argument beyond \"it is not what I like\". Which to me\n> > argues for the status quo as the path of least resistance.\n> \n> Didn't Junio already provided reasoning?\n\nIf the reasoning is \"cmp(actual, expect) makes more sense to humans\"\nthen I do not think it is universal. Otherwise why would so many\nexisting test frameworks do it the other way? And that is why I said it\nseems more like an issue of personal preference than a universal truth.\n\nWas there some objective argument made that I missed?\n\n> Here's more; human semantics:\n> \n> Computer, compare A with B\n> cmp(A, B)\n> \n> Why would I write?\n> \n> cmp(B, A)\n> \n> Could you even construct an English sentence that starts with B, and then A?\n\n\"Computer, given that we expect B, how does A differ?\". Or \"Computer, we\nexpect B; does A match it?\"\n\nOr any number of variations. I'm sure you will say \"but those seem\nawkward and unlike how I think about it\". But that was my point; it\nseems to be a matter of preference.\n\n-Peff\n"},{"id":"227059","messageId":"CAMP44s16RZ9JguL=NPcD8TE-gdG7vGEa5KLHzv4bf_VLLEhPRw@mail.gmail.com","threadId":"34829","inReplyTo":"20130908040615.GA14019@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-08T04:13:10Z","receivedAt":"2013-09-08T04:13:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Sep 7, 2013 at 11:06 PM, Jeff King wrote:\n> On Sat, Sep 07, 2013 at 10:11:49PM -0500, Felipe Contreras wrote:\n>\n>> > Though I prefer the current, I can certainly live and adapt to a changed\n>> > standard, and I do not mind doing so if there is a good reason. But I've\n>> > yet to see any argument beyond \"it is not what I like\". Which to me\n>> > argues for the status quo as the path of least resistance.\n>>\n>> Didn't Junio already provided reasoning?\n>\n> If the reasoning is \"cmp(actual, expect) makes more sense to humans\"\n> then I do not think it is universal.\n\nNo.\n\n---\nA(ny) sanely defined \"compare A with B\" function should yield the\nresult of subtracting B from A, i.e. cmp(A,B) should be like (A-B).\nThat is what you feed qsort() and bsearch() (it is not limited to C;\nyou see the same in \"sort { $a <=> $b }\"). The definition naturally\nmakes \"cmp(A,B) < 0\" like \"A < B\" and \"cmp(A,B) > 0\" like \"A > B\".\n---\n\n> Otherwise why would so many\n> existing test frameworks do it the other way?\n\nWhich many existing frameworks do it the other way?\n\n>> Here's more; human semantics:\n>>\n>> Computer, compare A with B\n>> cmp(A, B)\n>>\n>> Why would I write?\n>>\n>> cmp(B, A)\n>>\n>> Could you even construct an English sentence that starts with B, and then A?\n>\n> \"Computer, given that we expect B, how does A differ?\". Or \"Computer, we\n> expect B; does A match it?\"\n>\n> Or any number of variations. I'm sure you will say \"but those seem\n> awkward and unlike how I think about it\". But that was my point; it\n> seems to be a matter of preference.\n\nReally? You think any sane human being would prefer:\n\nComputer, given that we expect B, how does A differ?\n\nTo:\n\nComputer, compare A with B\n\n-- \nFelipe Contreras\n"},{"id":"227060","messageId":"CAMP44s1gAfvBfcoStEB=U=dGHHxBPXXTbOXjELqDj17G6Tiumw@mail.gmail.com","threadId":"34829","inReplyTo":"CAMP44s16RZ9JguL=NPcD8TE-gdG7vGEa5KLHzv4bf_VLLEhPRw@mail.gmail.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-08T04:14:50Z","receivedAt":"2013-09-08T04:14:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Sep 7, 2013 at 11:13 PM, Felipe Contreras\n wrote:\n> On Sat, Sep 7, 2013 at 11:06 PM, Jeff King wrote:\n>> On Sat, Sep 07, 2013 at 10:11:49PM -0500, Felipe Contreras wrote:\n>>\n>>> > Though I prefer the current, I can certainly live and adapt to a changed\n>>> > standard, and I do not mind doing so if there is a good reason. But I've\n>>> > yet to see any argument beyond \"it is not what I like\". Which to me\n>>> > argues for the status quo as the path of least resistance.\n>>>\n>>> Didn't Junio already provided reasoning?\n>>\n>> If the reasoning is \"cmp(actual, expect) makes more sense to humans\"\n>> then I do not think it is universal.\n>\n> No.\n>\n> ---\n> A(ny) sanely defined \"compare A with B\" function should yield the\n> result of subtracting B from A, i.e. cmp(A,B) should be like (A-B).\n> That is what you feed qsort() and bsearch() (it is not limited to C;\n> you see the same in \"sort { $a <=> $b }\"). The definition naturally\n> makes \"cmp(A,B) < 0\" like \"A < B\" and \"cmp(A,B) > 0\" like \"A > B\".\n> ---\n>\n>> Otherwise why would so many\n>> existing test frameworks do it the other way?\n>\n> Which many existing frameworks do it the other way?\n>\n>>> Here's more; human semantics:\n>>>\n>>> Computer, compare A with B\n>>> cmp(A, B)\n>>>\n>>> Why would I write?\n>>>\n>>> cmp(B, A)\n>>>\n>>> Could you even construct an English sentence that starts with B, and then A?\n>>\n>> \"Computer, given that we expect B, how does A differ?\". Or \"Computer, we\n>> expect B; does A match it?\"\n>>\n>> Or any number of variations. I'm sure you will say \"but those seem\n>> awkward and unlike how I think about it\". But that was my point; it\n>> seems to be a matter of preference.\n>\n> Really? You think any sane human being would prefer:\n>\n> Computer, given that we expect B, how does A differ?\n\nAnd btw, that could barely be translated to cmp(B, A), probably cmp_given(B, A).\n\n-- \nFelipe Contreras\n"},{"id":"227062","messageId":"20130908042649.GC14019@sigill.intra.peff.net","threadId":"34829","inReplyTo":"CAMP44s16RZ9JguL=NPcD8TE-gdG7vGEa5KLHzv4bf_VLLEhPRw@mail.gmail.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T04:26:49Z","receivedAt":"2013-09-08T04:26:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 07, 2013 at 11:13:10PM -0500, Felipe Contreras wrote:\n\n> > If the reasoning is \"cmp(actual, expect) makes more sense to humans\"\n> > then I do not think it is universal.\n> \n> No.\n> \n> ---\n> A(ny) sanely defined \"compare A with B\" function should yield the\n> result of subtracting B from A, i.e. cmp(A,B) should be like (A-B).\n> That is what you feed qsort() and bsearch() (it is not limited to C;\n> you see the same in \"sort { $a <=> $b }\"). The definition naturally\n> makes \"cmp(A,B) < 0\" like \"A < B\" and \"cmp(A,B) > 0\" like \"A > B\".\n> ---\n\nAh, you mean \"if you think that the compare function should behave like\nC *_cmp functions, it should be A-B\". Perhaps it is simply that I do not\nthink of the function in those terms, but more like \"show me the\ndifferences from B to A\".\n\n> > Otherwise why would so many\n> > existing test frameworks do it the other way?\n> \n> Which many existing frameworks do it the other way?\n\nJohn mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I\nbelieve that Ruby's Test::Unit::Assertions also has\nassert_equal(expected, actual).\n\n> > Or any number of variations. I'm sure you will say \"but those seem\n> > awkward and unlike how I think about it\". But that was my point; it\n> > seems to be a matter of preference.\n> \n> Really? You think any sane human being would prefer:\n> \n> Computer, given that we expect B, how does A differ?\n> \n> To:\n> \n> Computer, compare A with B\n\nI already said that is how I think about it. If you want to call me not\nsane, feel free. But I do not see that this line of discussion is going\nanywhere productive.\n\n-Peff\n"},{"id":"227067","messageId":"CAMP44s1uasvGmCMkMF2N0eTMgOdksG0HqeidXK+159HAi4jkpA@mail.gmail.com","threadId":"34829","inReplyTo":"20130908042649.GC14019@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-08T04:52:10Z","receivedAt":"2013-09-08T04:52:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Sep 7, 2013 at 11:26 PM, Jeff King wrote:\n> On Sat, Sep 07, 2013 at 11:13:10PM -0500, Felipe Contreras wrote:\n>\n>> > If the reasoning is \"cmp(actual, expect) makes more sense to humans\"\n>> > then I do not think it is universal.\n>>\n>> No.\n>>\n>> ---\n>> A(ny) sanely defined \"compare A with B\" function should yield the\n>> result of subtracting B from A, i.e. cmp(A,B) should be like (A-B).\n>> That is what you feed qsort() and bsearch() (it is not limited to C;\n>> you see the same in \"sort { $a <=> $b }\"). The definition naturally\n>> makes \"cmp(A,B) < 0\" like \"A < B\" and \"cmp(A,B) > 0\" like \"A > B\".\n>> ---\n>\n> Ah, you mean \"if you think that the compare function should behave like\n> C *_cmp functions, it should be A-B\". Perhaps it is simply that I do not\n> think of the function in those terms, but more like \"show me the\n> differences from B to A\".\n\nBut that is the problem, you are unable to ignore the implementation.\nYou don't see test_cmp(), you see 'diff -u'.\n\n>> > Otherwise why would so many\n>> > existing test frameworks do it the other way?\n>>\n>> Which many existing frameworks do it the other way?\n>\n> John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I\n> believe that Ruby's Test::Unit::Assertions also has\n> assert_equal(expected, actual).\n\nThat's because they all do first expect, then actual.\n\nassert_equal( expected, actual, failure_message = nil )\nassert_not_equal( expected, actual, failure_message = nil )\n\nThat's why.\n\n>> > Or any number of variations. I'm sure you will say \"but those seem\n>> > awkward and unlike how I think about it\". But that was my point; it\n>> > seems to be a matter of preference.\n>>\n>> Really? You think any sane human being would prefer:\n>>\n>> Computer, given that we expect B, how does A differ?\n>>\n>> To:\n>>\n>> Computer, compare A with B\n>\n> I already said that is how I think about it. If you want to call me notn\n> sane, feel free. But I do not see that this line of discussion is going\n> anywhere productive.\n\nAgain, that's because you are already thinking on the resulting diff,\nbased on the 'diff -u' command, but that's not in question here. Even\nif test_cmp() didn't return an diff (it just ran cmp), it would be\nuseful, as ultimately we want to test for failures. Ultimately what we\nwant is to check that A is equal to B, so it's natural to tell the\ncomputer \"compare A with B\", and if you don't think so, then yeah, I\nthink you are insane.\n\n-- \nFelipe Contreras\n"},{"id":"227070","messageId":"20130908050227.GG14019@sigill.intra.peff.net","threadId":"34829","inReplyTo":"CAMP44s1uasvGmCMkMF2N0eTMgOdksG0HqeidXK+159HAi4jkpA@mail.gmail.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T05:02:27Z","receivedAt":"2013-09-08T05:02:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 07, 2013 at 11:52:10PM -0500, Felipe Contreras wrote:\n\n> > Ah, you mean \"if you think that the compare function should behave like\n> > C *_cmp functions, it should be A-B\". Perhaps it is simply that I do not\n> > think of the function in those terms, but more like \"show me the\n> > differences from B to A\".\n> \n> But that is the problem, you are unable to ignore the implementation.\n> You don't see test_cmp(), you see 'diff -u'.\n\nYes, I already said earlier in the thread:\n\n I certainly think of \"test_cmp A B\" as \"differences from A to B\", and\n the order makes sense. IOW, the \"test_cmp is diff\" abstraction is\n leaky, and that is fine (if it were not leaky, then order would not\n matter at all, but it clearly does).\n\nAnd I do not think it is a problem. The point of the function is not to\nabstract away the idea of comparison. The point is to give a hook for\npeople on systems without \"diff -u\" to run the test suite.\n\n> > John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I\n> > believe that Ruby's Test::Unit::Assertions also has\n> > assert_equal(expected, actual).\n> \n> That's because they all do first expect, then actual.\n> \n> assert_equal( expected, actual, failure_message = nil )\n> assert_not_equal( expected, actual, failure_message = nil )\n> \n> That's why.\n\nI do not see any reason why \"not_equal\" would not also work as\n\"assert_not_equal(actual, expected)\". Maybe I am missing your point.\n\n-Peff\n"},{"id":"227104","messageId":"28ED017444BF442D84F8003247E2DDDC@PhilipOakley","threadId":"34829","inReplyTo":"20130908040615.GA14019@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-09-08T08:11:43Z","receivedAt":"2013-09-08T08:11:43Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \nSent: Sunday, September 08, 2013 5:06 AM\n> Was there some objective argument made that I missed?\n> \n>> Here's more; human semantics:\n>> \n\nIsn't this one of those \"pick any two from three\" tasks:\n 'human', 'objective', 'argument'.\n\nOnly 1/6 of the time is an 'objective' answer the result. \n\nPhilip\n\"Between thee and me there's nowt so queer as fowk,\nand I ain't so sure about thee.\" old Yorkshire saying.\n\n \n"},{"id":"227146","messageId":"xmqqvc2b5fkg.fsf@gitster.dls.corp.google.com","threadId":"34829","inReplyTo":"20130908042649.GC14019@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-08T18:33:19Z","receivedAt":"2013-09-08T18:33:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> A(ny) sanely defined \"compare A with B\" function should yield the\n>> result of subtracting B from A, i.e. cmp(A,B) should be like (A-B).\n>> That is what you feed qsort() and bsearch() (it is not limited to C;\n>> you see the same in \"sort { $a <=> $b }\"). The definition naturally\n>> makes \"cmp(A,B) < 0\" like \"A < B\" and \"cmp(A,B) > 0\" like \"A > B\".\n>> ---\n>\n> Ah, you mean \"if you think that the compare function should behave like\n> C *_cmp functions, it should be A-B\". Perhaps it is simply that I do not\n> think of the function in those terms, but more like \"show me the\n> differences from B to A\".\n\n\n\n>\n>> > Otherwise why would so many\n>> > existing test frameworks do it the other way?\n>> \n>> Which many existing frameworks do it the other way?\n>\n> John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I\n> believe that Ruby's Test::Unit::Assertions also has\n> assert_equal(expected, actual).\n\nEspecially the last one can be excused. \"is A and B equal\" is a\nbinary between \"yes\" and \"no\". If A and B are equal, B and A are\nequal, and it becomes more like \"which endianness is correct?\" as\nyou mentioned earlier.\n\nI think the real cause of confusion is that \"cmp(1)\" is not a\ncomparison in that sense but is an equality check; \"test_cmp\" has a\ndual purpose in that its primary use as \"did the previous step\nproduce correct result?\" is an equality check and the order does not\nreally matter, but its secondary purpose, to show how the actual\noutput deviated from the norm, has to be done by subtracting the\nexpected result from the actual result.\n\nAs I said, I am somewhat sympathetic to those who want to see such\nsubtraction spelled as cmp(Actual,Expect), but we are so used to the\norder \"diff(1)\" takes expect and actual to do that subtraction in\nthat order, so using diff(Expect,Actual) order is not that wrong.\n\nCalling the abstraction \"test_diff\" might have avoided the wasted\nbrain bandwidth in this thread, but I do not think renaming it in\ntest-lib-functions.sh is worth the trouble, either ;-)\n"},{"id":"227169","messageId":"CAMP44s2cXnrR2WHtyFKgNxi7AtuS9dA9if1rqFaH2b_35YTfiw@mail.gmail.com","threadId":"34829","inReplyTo":"xmqqvc2b5fkg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-08T23:18:14Z","receivedAt":"2013-09-08T23:18:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Sep 8, 2013 at 1:33 PM, Junio C Hamano wrote:\n> Jeff King writes:\n\n> Calling the abstraction \"test_diff\" might have avoided the wasted\n> brain bandwidth in this thread, but I do not think renaming it in\n> test-lib-functions.sh is worth the trouble, either ;-)\n\nYes, but then it wouldn't be clear what's the main purpose of\ntest_diff(). Primarily what we want is to check that they are the\nsame. The diff is secondary, and it's not actually *needed*.\n\n-- \nFelipe Contreras\n"},{"id":"227170","messageId":"CAMP44s1HXeQ1pgCeY=RsDcx4H--YHBfzmoQ4rjJ37t5N30pq5A@mail.gmail.com","threadId":"34829","inReplyTo":"20130908050227.GG14019@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-08T23:25:45Z","receivedAt":"2013-09-08T23:25:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Sep 8, 2013 at 12:02 AM, Jeff King wrote:\n> On Sat, Sep 07, 2013 at 11:52:10PM -0500, Felipe Contreras wrote:\n>\n>> > Ah, you mean \"if you think that the compare function should behave like\n>> > C *_cmp functions, it should be A-B\". Perhaps it is simply that I do not\n>> > think of the function in those terms, but more like \"show me the\n>> > differences from B to A\".\n>>\n>> But that is the problem, you are unable to ignore the implementation.\n>> You don't see test_cmp(), you see 'diff -u'.\n>\n> Yes, I already said earlier in the thread:\n>\n> I certainly think of \"test_cmp A B\" as \"differences from A to B\", and\n> the order makes sense. IOW, the \"test_cmp is diff\" abstraction is\n> leaky, and that is fine (if it were not leaky, then order would not\n> matter at all, but it clearly does).\n\nThen I don't think you can give a fair and objective opinion about\nwhat should be the ideal ordering of the arguments. You would be\nforever biased to whatever is the order of 'diff -u'.\n\n> And I do not think it is a problem. The point of the function is not to\n> abstract away the idea of comparison. The point is to give a hook for\n> people on systems without \"diff -u\" to run the test suite.\n\nThe point according to whom? I say it's the other way around.\n\n>> > John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I\n>> > believe that Ruby's Test::Unit::Assertions also has\n>> > assert_equal(expected, actual).\n>>\n>> That's because they all do first expect, then actual.\n>>\n>> assert_equal( expected, actual, failure_message = nil )\n>> assert_not_equal( expected, actual, failure_message = nil )\n>>\n>> That's why.\n>\n> I do not see any reason why \"not_equal\" would not also work as\n> \"assert_not_equal(actual, expected)\". Maybe I am missing your point.\n\nAll right, maybe it's because Ruby started in Japan, and their\nsentences have very different orderings, maybe it's legacy from\nanother test framework, maybe there's no reason at all and somebody\njust randomly picked them.\n\nEither way the fact that others are doing it differently doesn't mean\nthat's the best way, that would be argumentum ad populum, and mothers\nare keen to remind us that if other kids are jumping the bridge, that\ndoesn't mean we should too.\n\n-- \nFelipe Contreras\n"},{"id":"227172","messageId":"20130908234519.GA11860@sigill.intra.peff.net","threadId":"34829","inReplyTo":"CAMP44s1HXeQ1pgCeY=RsDcx4H--YHBfzmoQ4rjJ37t5N30pq5A@mail.gmail.com","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T23:45:19Z","receivedAt":"2013-09-08T23:45:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 08, 2013 at 06:25:45PM -0500, Felipe Contreras wrote:\n\n> > And I do not think it is a problem. The point of the function is not to\n> > abstract away the idea of comparison. The point is to give a hook for\n> > people on systems without \"diff -u\" to run the test suite.\n> \n> The point according to whom? I say it's the other way around.\n\nThe point according to 82ebb0b (add test_cmp function for test scripts,\n2008-03-12). I wish I had simply called it test_diff back then, and then\nthis conversation could have never occurred.\n\n> Either way the fact that others are doing it differently doesn't mean\n> that's the best way, that would be argumentum ad populum, and mothers\n> are keen to remind us that if other kids are jumping the bridge, that\n> doesn't mean we should too.\n\nDid I once say \"my way of thinking about it is the best way\"? No. I said\n\"I think it is a matter of preference\". I mentioned other systems using\na particular ordering to show that the set of people who prefer it the\nother way is non-zero.\n\nFeel free to respond, but I have no interest in discussing this any\nfurther with you. This thread has become a giant time sink, and I have\nnothing else to say on the matter that I have not already said.\n\n-Peff\n"},{"id":"227181","messageId":"CAMP44s2U2pUNHjhq9x1tcJfanDpYNOyORwyWkvwtoCUA9Jv+yA@mail.gmail.com","threadId":"34829","inReplyTo":"20130908234519.GA11860@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-09T00:45:07Z","receivedAt":"2013-09-09T00:45:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Sep 8, 2013 at 6:45 PM, Jeff King wrote:\n> On Sun, Sep 08, 2013 at 06:25:45PM -0500, Felipe Contreras wrote:\n>\n>> > And I do not think it is a problem. The point of the function is not to\n>> > abstract away the idea of comparison. The point is to give a hook for\n>> > people on systems without \"diff -u\" to run the test suite.\n>>\n>> The point according to whom? I say it's the other way around.\n>\n> The point according to 82ebb0b (add test_cmp function for test scripts,\n> 2008-03-12). I wish I had simply called it test_diff back then, and then\n> this conversation could have never occurred.\n\nSo? That's the original intention from the author, it's not the point\nof the function from the project's point of view, or what the point\n_should_ be.\n\n>> Either way the fact that others are doing it differently doesn't mean\n>> that's the best way, that would be argumentum ad populum, and mothers\n>> are keen to remind us that if other kids are jumping the bridge, that\n>> doesn't mean we should too.\n>\n> Did I once say \"my way of thinking about it is the best way\"? No. I said\n> \"I think it is a matter of preference\". I mentioned other systems using\n> a particular ordering to show that the set of people who prefer it the\n> other way is non-zero.\n\nThat doesn't show it they prefer it that way. They could be in the\nsame situation than us, they might prefer it a different way, but they\nare stuck with what they chose long time ago.\n\n> Feel free to respond, but I have no interest in discussing this any\n> further with you. This thread has become a giant time sink, and I have\n> nothing else to say on the matter that I have not already said.\n\nIt's all right, I don't think you were contributing much more than;\ndiff -u does $expect $actual.\n\nIn case the above gets misconstrued as an insult, it's not meant that\nway, Jeff said it himself; he cannot depart his view away from the\ndiff -u implementation.\n\n-- \nFelipe Contreras\n"}]}