{"thread":{"id":"36634","subject":"[BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit","startedAt":"2014-05-11T23:25:05Z","lastAt":"2014-05-12T23:26:25Z","messageCount":3,"participants":["Max Kirillov","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"241271","messageId":"20140511232505.GA29104@wheezy.local","threadId":"36634","inReplyTo":null,"subject":"[BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-05-11T23:25:05Z","receivedAt":"2014-05-11T23:25:05Z","isPatch":false,"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\nbetween 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\", but this\ncondition was also removed from show_log call before,\nso I'm not sure about it.\n\nTest \"t7007-show\" is also modified to cover this case.\n---\nHi.\n\nThis actually has broken some number of existing tests\n(their fix not included), and might break some existing\nscripts which already expect this extra newline.\n\nSo, is it yet not too late to fix this?\n combine-diff.c  | 2 +-\n t/t7007-show.sh | 8 ++++++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 3b92c448..d2924de 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1331,7 +1331,7 @@ 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 && 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..d76c7db 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":"241304","messageId":"xmqqk39qu96o.fsf@gitster.dls.corp.google.com","threadId":"36634","inReplyTo":"20140511232505.GA29104@wheezy.local","subject":"Re: [BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-12T16:59:39Z","receivedAt":"2014-05-12T16:59:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> writes:\n\n> When \"git show -s\" is called for merge commit it prints extra newline\n> between 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\nYeah, it is not limited to one-line formats.  For example:\n\n    $ git show -s v2.0.0-rc3^0 v2.0.0-rc3^1\n\nends with an extra blank line before you see your prompt, but if you\nswap the revs:\n\n    $ git show -s v2.0.0-rc3^1 v2.0.0-rc3^0\n\nyou do not get the extra blank line.  Instead, you have an extra\nblank in between.  And I agree that these extra blank lines look\nvery much like a bug.\n\n> This actually has broken some number of existing tests\n> (their fix not included), and might break some existing\n> scripts which already expect this extra newline.\n>\n> So, is it yet not too late to fix this?\n\nI would prefer to see it fixed eventually.\n\nI haven't checked if the new extra check uses the right condition\nyet, but from a cursory look and with a few examples I tried\nmanually, the version with your change seems to behave a lot more\nsensibly.\n\nA good way to double-check may be to see the fixes to the tests to\ncorrect their wrong expectations, and if the updated expectation is\nsensible.  That way, we will find if the observations we made (your\nproblem description and my two examples above, and a few examples I\ntried manually) covered all what matters, or if there are cases\nwhere they indeed were expecting some sensible results which this\nchange would break that we missed.  Offhand, I doubt we would find\nany case where these extra blank lines are sensible expectations,\nthough.\n\n>  combine-diff.c  | 2 +-\n>  t/t7007-show.sh | 8 ++++++--\n>  2 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/combine-diff.c b/combine-diff.c\n> index 3b92c448..d2924de 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -1331,7 +1331,7 @@ 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 && opt->output_format != DIFF_FORMAT_NO_OUTPUT)\n\nPlease wrap this long line, perhaps like:\n\n\t\t\tif (rev->verbose_header && opt->output_format &&\n\t\t\t    opt->output_format != DIFF_FORMAT_NO_OUTPUT)\n\n\n>  \t\t\t\tprintf(\"%s%c\", diff_line_prefix(opt),\n>  \t\t\t\t       opt->line_termination);\n>  \t\t}\n> diff --git a/t/t7007-show.sh b/t/t7007-show.sh\n> index e41fa00..d76c7db 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\nPlease make it a habit to quote the EOF marker when you are not\nplacing things that require variable substitutions, to tell the\nreaders that they can coast their eyes over the here-document\nwithout having to worry about them.  For a short here-document\nlike this, it does not matter, but these things have a habit of\ngetting imitated without thinking by others, and it is better to be\ndefensive and consistent.  I.e.\n\n\tcat >expect <<-\\EOF &&\n        merge\n        main3\n        EOF\n\n> +\tgit show -s --format=%s merge main3 >actual &&\n>  \ttest_cmp expect actual\n>  '\n"},{"id":"241328","messageId":"20140512232600.GA6857@wheezy.local","threadId":"36634","inReplyTo":"xmqqk39qu96o.fsf@gitster.dls.corp.google.com","subject":"Re: [BUGFIX/RFC] 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:26:25Z","receivedAt":"2014-05-12T23:26:25Z","isPatch":false,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, May 12, 2014 at 09:59:39AM -0700, Junio C Hamano wrote:\n> A good way to double-check may be to see the fixes to the tests to\n> correct their wrong expectations, and if the updated expectation is\n> sensible.\n\nI have sent the fixes to tests. To me they look sensible.\n\nAlso fixed the things you pointed out.\n\n-- \nMax\n"}]}