{"thread":{"id":"36642","subject":"[PATCH v2 0/2] fix 'git show -s' to not add extra terminator after merge commit","startedAt":"2014-05-12T23:09:43Z","lastAt":"2014-05-13T20:15:12Z","messageCount":7,"participants":["Max Kirillov","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"241325","messageId":"20140512230943.GC32316@wheezy.local","threadId":"36642","inReplyTo":null,"subject":"[PATCH v2 0/2] fix 'git show -s' to not add extra terminator after merge commit","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-05-12T23:09:43Z","receivedAt":"2014-05-12T23:09:43Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Since v1:\n* add Signed-off-by\n* remove notion about '--format=%s' - found it in the\n  documentation\n\nSince RFC:\n* fix the CC issues\n* add fixes for existing tests\n\nMax Kirillov (2):\n  git-show: fix 'git show -s' to not add extra terminator after merge commit\n  t: git-show: adapt tests to fixed 'git show'\n\n combine-diff.c                |  3 ++-\n t/t1507-rev-parse-upstream.sh |  2 +-\n t/t7007-show.sh               |  8 ++++++--\n t/t7600-merge.sh              | 11 +++++------\n 4 files changed, 14 insertions(+), 10 deletions(-)\n\n-- \n1.8.5.2.421.g4cdf8d0\n"},{"id":"241326","messageId":"20140512231029.GD32316@wheezy.local","threadId":"36642","inReplyTo":"20140512230943.GC32316@wheezy.local","subject":"[PATCH v2 1/2] git-show: fix 'git show -s' to not add extra terminator after merge commit","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-05-12T23:10:29Z","receivedAt":"2014-05-12T23:10:29Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"When git show -s is called for merge commit it prints extra newline\nafter any merge commit and the next one. This looks especially ugly for\n--oneline and other single-line formats. Looks very much like a bug.\n\nThe code in question exists since commit 3969cf7db1. Probably the\ncorrect condition should be in fact\n\"opt->output_format & DIFF_FORMAT_DIFFSTAT\".\n\nTest t7007-show.sh is also modified to cover this case.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n combine-diff.c  | 3 ++-\n t/t7007-show.sh | 8 ++++++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 3b92c448..ff6ceaf 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1331,7 +1331,8 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\tif (show_log_first && i == 0) {\n \t\t\tshow_log(rev);\n \n-\t\t\tif (rev->verbose_header && opt->output_format)\n+\t\t\tif (rev->verbose_header && opt->output_format &&\n+\t\t\t    opt->output_format != DIFF_FORMAT_NO_OUTPUT)\n \t\t\t\tprintf(\"%s%c\", diff_line_prefix(opt),\n \t\t\t\t       opt->line_termination);\n \t\t}\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex e41fa00..de22812 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -25,6 +25,7 @@ test_expect_success 'set up a bit of history' '\n \tgit checkout -b side HEAD^^ &&\n \ttest_commit side2 &&\n \ttest_commit side3\n+\ttest_merge merge main3\n '\n \n test_expect_success 'showing two commits' '\n@@ -109,8 +110,11 @@ test_expect_success 'showing range' '\n '\n \n test_expect_success '-s suppresses diff' '\n-\techo main3 >expect &&\n-\tgit show -s --format=%s main3 >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tmerge\n+\tmain3\n+\tEOF\n+\tgit show -s --format=%s merge main3 >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n1.8.5.2.421.g4cdf8d0\n"},{"id":"241327","messageId":"20140512231107.GE32316@wheezy.local","threadId":"36642","inReplyTo":"20140512230943.GC32316@wheezy.local","subject":"[PATCH v2 2/2] t: git-show: adapt tests to fixed 'git show'","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-05-12T23:11:07Z","receivedAt":"2014-05-12T23:11:07Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"'git show' used to print extra newline after merge commit, and it was\nrecorded so into the test reference data. Now when the behavior is\nfixed, the tests should be updated.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n t/t1507-rev-parse-upstream.sh |  2 +-\n t/t7600-merge.sh              | 11 +++++------\n 2 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 2a19e79..672280b 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -100,7 +100,7 @@ test_expect_success 'merge my-side@{u} records the correct name' '\n \tgit branch -D new ;# can fail but is ok\n \tgit branch -t new my-side@{u} &&\n \tgit merge -s ours new@{u} &&\n-\tgit show -s --pretty=format:%s >actual &&\n+\tgit show -s --pretty=tformat:%s >actual &&\n \techo \"Merge remote-tracking branch ${sq}origin/side${sq}\" >expect &&\n \ttest_cmp expect actual\n )\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 10aa028..b164621 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -57,11 +57,10 @@ create_merge_msgs () {\n \t\tgit log --no-merges ^HEAD c2 c3\n \t} >squash.1-5-9 &&\n \t: >msg.nologff &&\n-\techo >msg.nolognoff &&\n+\t: >msg.nolognoff &&\n \t{\n \t\techo \"* tag 'c3':\" &&\n-\t\techo \"  commit 3\" &&\n-\t\techo\n+\t\techo \"  commit 3\"\n \t} >msg.log\n }\n \n@@ -71,7 +70,7 @@ verify_merge () {\n \tgit diff --exit-code &&\n \tif test -n \"$3\"\n \tthen\n-\t\tgit show -s --pretty=format:%s HEAD >msg.act &&\n+\t\tgit show -s --pretty=tformat:%s HEAD >msg.act &&\n \t\ttest_cmp \"$3\" msg.act\n \tfi\n }\n@@ -620,10 +619,10 @@ test_expect_success 'merge early part of c2' '\n \tgit tag c6 &&\n \tgit branch -f c5-branch c5 &&\n \tgit merge c5-branch~1 &&\n-\tgit show -s --pretty=format:%s HEAD >actual.branch &&\n+\tgit show -s --pretty=tformat:%s HEAD >actual.branch &&\n \tgit reset --keep HEAD^ &&\n \tgit merge c5~1 &&\n-\tgit show -s --pretty=format:%s HEAD >actual.tag &&\n+\tgit show -s --pretty=tformat:%s HEAD >actual.tag &&\n \ttest_cmp expected.branch actual.branch &&\n \ttest_cmp expected.tag actual.tag\n '\n-- \n1.8.5.2.421.g4cdf8d0\n"},{"id":"241331","messageId":"5371B434.7010506@viscovery.net","threadId":"36642","inReplyTo":"20140512231029.GD32316@wheezy.local","subject":"Re: [PATCH v2 1/2] git-show: fix 'git show -s' to not add extra terminator after merge commit","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-05-13T05:57:08Z","receivedAt":"2014-05-13T05:57:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/13/2014 1:10, schrieb Max Kirillov:\n> --- a/t/t7007-show.sh\n> +++ b/t/t7007-show.sh\n> @@ -25,6 +25,7 @@ test_expect_success 'set up a bit of history' '\n>  \tgit checkout -b side HEAD^^ &&\n>  \ttest_commit side2 &&\n>  \ttest_commit side3\n> +\ttest_merge merge main3\n>  '\n\nBroken &&-chain.\n\n-- Hannes\n"},{"id":"241393","messageId":"xmqqiop9o4y5.fsf@gitster.dls.corp.google.com","threadId":"36642","inReplyTo":"20140512231107.GE32316@wheezy.local","subject":"Re: [PATCH v2 2/2] t: git-show: adapt tests to fixed 'git show'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-13T19:26:42Z","receivedAt":"2014-05-13T19:26:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> writes:\n\n> 'git show' used to print extra newline after merge commit, and it was\n> recorded so into the test reference data. Now when the behavior is\n> fixed, the tests should be updated.\n>\n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n\nHmph.  Having these as two separate commits would mean that 1/2\nalone will break the test, hurting bisectability a little bit.  The\nnecessary adjustments in this patch is small enough that we may be\nbetter off squashing them into one.\n\n>  t/t1507-rev-parse-upstream.sh |  2 +-\n>  t/t7600-merge.sh              | 11 +++++------\n>  2 files changed, 6 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\n> index 2a19e79..672280b 100755\n> --- a/t/t1507-rev-parse-upstream.sh\n> +++ b/t/t1507-rev-parse-upstream.sh\n> @@ -100,7 +100,7 @@ test_expect_success 'merge my-side@{u} records the correct name' '\n>  \tgit branch -D new ;# can fail but is ok\n>  \tgit branch -t new my-side@{u} &&\n>  \tgit merge -s ours new@{u} &&\n> -\tgit show -s --pretty=format:%s >actual &&\n> +\tgit show -s --pretty=tformat:%s >actual &&\n>  \techo \"Merge remote-tracking branch ${sq}origin/side${sq}\" >expect &&\n>  \ttest_cmp expect actual\n>  )\n\nThis seems to me that it is updating how the output is produced, not\nupdating the expected outputs from commands with arguments we have\nbeen testing.  I have been expecting to see changes to the pieces of\nthe code that prepare the expected output, so that we can compare\nold expectations, which would have been expecting something strange,\nwith new expectations from the updated code, which would show that\nthe new behaviour is a welcome change because it would produce more\nsensible output.\n\nHaving said all that, for this particular test piece, I agree with\nthe patch that using --pretty=tformat:%s is a lot more sensible and\nusing --pretty=format:%s and expecting its output to be terminated\nwith the newline was an unrealistic test.  After all, \"tformat\" is\nthe version with \"line terminator\" semantics, as opposed to \"item\nseparator\" semantics offered by \"--pretty=format:\", and the test\nmerely was depending on the bug to expect a complete line output\n(i.e. \"echo\" without \"-n\"), which you fixed.  The new test makes a\nlot more sense and is relevant to the real world use, and I would\nhave preferred to see it explained as such in the log message.\n\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 10aa028..b164621 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -57,11 +57,10 @@ create_merge_msgs () {\n>  \t\tgit log --no-merges ^HEAD c2 c3\n>  \t} >squash.1-5-9 &&\n>  \t: >msg.nologff &&\n> -\techo >msg.nolognoff &&\n> +\t: >msg.nolognoff &&\n>  \t{\n>  \t\techo \"* tag 'c3':\" &&\n> -\t\techo \"  commit 3\" &&\n> -\t\techo\n> +\t\techo \"  commit 3\"\n>  \t} >msg.log\n>  }\n\nThese are updating the expectation.  We used to expect an\nunnecessary trailing blank line, and now we do not, which clearly\nshows that the previous fix is a welcome change.\n\n> @@ -71,7 +70,7 @@ verify_merge () {\n>  \tgit diff --exit-code &&\n>  \tif test -n \"$3\"\n>  \tthen\n> -\t\tgit show -s --pretty=format:%s HEAD >msg.act &&\n> +\t\tgit show -s --pretty=tformat:%s HEAD >msg.act &&\n>  \t\ttest_cmp \"$3\" msg.act\n>  \tfi\n>  }\n\nIt is hard to judge what is fed as \"$3\" here without context, but\nthis is in line with the \"--pretty=tformat aka --format is the\nnormal thing to use\" we saw in 1507.  The same for the other hunk.\n\n> @@ -620,10 +619,10 @@ test_expect_success 'merge early part of c2' '\n>  \tgit tag c6 &&\n>  \tgit branch -f c5-branch c5 &&\n>  \tgit merge c5-branch~1 &&\n> -\tgit show -s --pretty=format:%s HEAD >actual.branch &&\n> +\tgit show -s --pretty=tformat:%s HEAD >actual.branch &&\n>  \tgit reset --keep HEAD^ &&\n>  \tgit merge c5~1 &&\n> -\tgit show -s --pretty=format:%s HEAD >actual.tag &&\n> +\tgit show -s --pretty=tformat:%s HEAD >actual.tag &&\n>  \ttest_cmp expected.branch actual.branch &&\n>  \ttest_cmp expected.tag actual.tag\n>  '\n\nHow about squashing these two into a single patch, and at the end of\nthe log message for 1/2, add this to explain the changes to the\ntest:\n\n    Also existing tests are updated to demonstrate the new\n    behaviour.  Earlier, the tests that used \"git show -s\n    --pretty=format:%s\", even though \"--pretty=format:%s\" calls for\n    item separator semantics and does not ask for the terminating\n    newline after the last item, expected the output to end with\n    such a newline.  They were relying on the buggy behaviour.  Use\n    of \"--format=%s\", which is equivalent to \"--pretty=tformat:%s\"\n    that asks for a terminating newline after each item, is a more\n    realistic way to use the command.\n\n    The update to verify_merge in t7600 adjusts the the merge\n    message that used to expect an extra blank line in the output,\n    which has been eliminated with this fix.\n\nor something like that?\n"},{"id":"241396","messageId":"20140513200910.GB6857@wheezy.local","threadId":"36642","inReplyTo":"xmqqiop9o4y5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] t: git-show: adapt tests to fixed 'git show'","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-05-13T20:09:50Z","receivedAt":"2014-05-13T20:09:50Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, May 13, 2014 at 12:26:42PM -0700, Junio C Hamano wrote:\n> Hmph.  Having these as two separate commits would mean that 1/2\n> alone will break the test, hurting bisectability a little bit.  The\n> necessary adjustments in this patch is small enough that we may be\n> better off squashing them into one.\n\nOk, will squash them.\n\n>>  t/t1507-rev-parse-upstream.sh |  2 +-\n>>  t/t7600-merge.sh              | 11 +++++------\n>>  2 files changed, 6 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\n>> index 2a19e79..672280b 100755\n>> --- a/t/t1507-rev-parse-upstream.sh\n>> +++ b/t/t1507-rev-parse-upstream.sh\n>> @@ -100,7 +100,7 @@ test_expect_success 'merge my-side@{u} records the correct name' '\n>>  \tgit branch -D new ;# can fail but is ok\n>>  \tgit branch -t new my-side@{u} &&\n>>  \tgit merge -s ours new@{u} &&\n>> -\tgit show -s --pretty=format:%s >actual &&\n>> +\tgit show -s --pretty=tformat:%s >actual &&\n>>  \techo \"Merge remote-tracking branch ${sq}origin/side${sq}\" >expect &&\n>>  \ttest_cmp expect actual\n>>  )\n> \n> This seems to me that it is updating how the output is produced, not\n> updating the expected outputs from commands with arguments we have\n> been testing.  I have been expecting to see changes to the pieces of\n> the code that prepare the expected output, so that we can compare\n> old expectations, which would have been expecting something strange,\n> with new expectations from the updated code, which would show that\n> the new behaviour is a welcome change because it would produce more\n> sensible output.\n> \n> Having said all that, for this particular test piece, I agree with\n> the patch that using --pretty=tformat:%s is a lot more sensible and\n> using --pretty=format:%s and expecting its output to be terminated\n> with the newline was an unrealistic test.  After all, \"tformat\" is\n> the version with \"line terminator\" semantics, as opposed to \"item\n> separator\" semantics offered by \"--pretty=format:\", and the test\n> merely was depending on the bug to expect a complete line output\n> (i.e. \"echo\" without \"-n\"), which you fixed.  The new test makes a\n> lot more sense and is relevant to the real world use, and I would\n> have preferred to see it explained as such in the log message.\n\nIn analogous cases with non-merge commits I have found the\nform \"--format=...\", in t3505-cherry-pick-empty.sh for\nexample, so I have decided that merges should also use it. The\nform \"--pretty=tformat:...\" seems more explicit to me, so I\nhave chosen this one.\n\nWill add the message as you have suggested.\n\n>> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n>> index 10aa028..b164621 100755\n>> --- a/t/t7600-merge.sh\n>> +++ b/t/t7600-merge.sh\n>> @@ -57,11 +57,10 @@ create_merge_msgs () {\n>>  \t\tgit log --no-merges ^HEAD c2 c3\n>>  \t} >squash.1-5-9 &&\n>>  \t: >msg.nologff &&\n>> -\techo >msg.nolognoff &&\n>> +\t: >msg.nolognoff &&\n>>  \t{\n>>  \t\techo \"* tag 'c3':\" &&\n>> -\t\techo \"  commit 3\" &&\n>> -\t\techo\n>> +\t\techo \"  commit 3\"\n>>  \t} >msg.log\n>>  }\n> \n> These are updating the expectation.  We used to expect an\n> unnecessary trailing blank line, and now we do not, which clearly\n> shows that the previous fix is a welcome change.\n> \n>> @@ -71,7 +70,7 @@ verify_merge () {\n>>  \tgit diff --exit-code &&\n>>  \tif test -n \"$3\"\n>>  \tthen\n>> -\t\tgit show -s --pretty=format:%s HEAD >msg.act &&\n>> +\t\tgit show -s --pretty=tformat:%s HEAD >msg.act &&\n>>  \t\ttest_cmp \"$3\" msg.act\n>>  \tfi\n>>  }\n> \n> It is hard to judge what is fed as \"$3\" here without context, but\n> this is in line with the \"--pretty=tformat aka --format is the\n> normal thing to use\" we saw in 1507.  The same for the other hunk.\n> \n>> @@ -620,10 +619,10 @@ test_expect_success 'merge early part of c2' '\n>>  \tgit tag c6 &&\n>>  \tgit branch -f c5-branch c5 &&\n>>  \tgit merge c5-branch~1 &&\n>> -\tgit show -s --pretty=format:%s HEAD >actual.branch &&\n>> +\tgit show -s --pretty=tformat:%s HEAD >actual.branch &&\n>>  \tgit reset --keep HEAD^ &&\n>>  \tgit merge c5~1 &&\n>> -\tgit show -s --pretty=format:%s HEAD >actual.tag &&\n>> +\tgit show -s --pretty=tformat:%s HEAD >actual.tag &&\n>>  \ttest_cmp expected.branch actual.branch &&\n>>  \ttest_cmp expected.tag actual.tag\n>>  '\n> \n> How about squashing these two into a single patch, and at the end of\n> the log message for 1/2, add this to explain the changes to the\n> test:\n> \n>     Also existing tests are updated to demonstrate the new\n>     behaviour.  Earlier, the tests that used \"git show -s\n>     --pretty=format:%s\", even though \"--pretty=format:%s\" calls for\n>     item separator semantics and does not ask for the terminating\n>     newline after the last item, expected the output to end with\n>     such a newline.  They were relying on the buggy behaviour.  Use\n>     of \"--format=%s\", which is equivalent to \"--pretty=tformat:%s\"\n>     that asks for a terminating newline after each item, is a more\n>     realistic way to use the command.\n> \n>     The update to verify_merge in t7600 adjusts the the merge\n>     message that used to expect an extra blank line in the output,\n>     which has been eliminated with this fix.\n> \n> or something like that?\n> \n> \n"},{"id":"241397","messageId":"20140513201511.GC6857@wheezy.local","threadId":"36642","inReplyTo":"5371B434.7010506@viscovery.net","subject":"Re: [PATCH v2 1/2] git-show: fix 'git show -s' to not add extra terminator after merge commit","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-05-13T20:15:12Z","receivedAt":"2014-05-13T20:15:12Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, May 13, 2014 at 07:57:08AM +0200, Johannes Sixt wrote:\n> Am 5/13/2014 1:10, schrieb Max Kirillov:\n>> --- a/t/t7007-show.sh\n>> +++ b/t/t7007-show.sh\n>> @@ -25,6 +25,7 @@ test_expect_success 'set up a bit of history' '\n>>  \tgit checkout -b side HEAD^^ &&\n>>  \ttest_commit side2 &&\n>>  \ttest_commit side3\n>> +\ttest_merge merge main3\n>>  '\n> \n> Broken &&-chain.\n\nThank you, will fix it.\n\n-- \nMax\n"}]}