[BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit

[BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit

[Max Kirillov]

When "git show -s" is called for merge commit it prints extra newline
between any merge commit and the next one. This looks especially ugly for
--oneline and other single-line formats. Looks very much like a bug.

The code in question exists since commit 3969cf7db1. Probably the
correct condition should be in fact
"opt->output_format & DIFF_FORMAT_DIFFSTAT", but this
condition was also removed from show_log call before,
so I'm not sure about it.

Test "t7007-show" is also modified to cover this case.
---
Hi.

This actually has broken some number of existing tests
(their fix not included), and might break some existing
scripts which already expect this extra newline.

So, is it yet not too late to fix this?
 combine-diff.c  | 2 +-
 t/t7007-show.sh | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 3b92c448..d2924de 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1331,7 +1331,7 @@ void diff_tree_combined(const unsigned char *sha1,
 		if (show_log_first && i == 0) {
 			show_log(rev);
 
-			if (rev->verbose_header && opt->output_format)
+			if (rev->verbose_header && opt->output_format && opt->output_format != DIFF_FORMAT_NO_OUTPUT)
 				printf("%s%c", diff_line_prefix(opt),
 				       opt->line_termination);
 		}
diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index e41fa00..d76c7db 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -25,6 +25,7 @@ test_expect_success 'set up a bit of history' '
 	git checkout -b side HEAD^^ &&
 	test_commit side2 &&
 	test_commit side3
+	test_merge merge main3
 '
 
 test_expect_success 'showing two commits' '
@@ -109,8 +110,11 @@ test_expect_success 'showing range' '
 '
 
 test_expect_success '-s suppresses diff' '
-	echo main3 >expect &&
-	git show -s --format=%s main3 >actual &&
+	cat >expect <<-EOF &&
+	merge
+	main3
+	EOF
+	git show -s --format=%s merge main3 >actual &&
 	test_cmp expect actual
 '
 
-- 
1.8.5.2.421.g4cdf8d0

Re: [BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> When "git show -s" is called for merge commit it prints extra newline
> between any merge commit and the next one. This looks especially ugly for
> --oneline and other single-line formats. Looks very much like a bug.

Yeah, it is not limited to one-line formats.  For example:

    $ git show -s v2.0.0-rc3^0 v2.0.0-rc3^1

ends with an extra blank line before you see your prompt, but if you
swap the revs:

    $ git show -s v2.0.0-rc3^1 v2.0.0-rc3^0

you do not get the extra blank line.  Instead, you have an extra
blank in between.  And I agree that these extra blank lines look
very much like a bug.

> This actually has broken some number of existing tests
> (their fix not included), and might break some existing
> scripts which already expect this extra newline.
>
> So, is it yet not too late to fix this?

I would prefer to see it fixed eventually.

I haven't checked if the new extra check uses the right condition
yet, but from a cursory look and with a few examples I tried
manually, the version with your change seems to behave a lot more
sensibly.

A good way to double-check may be to see the fixes to the tests to
correct their wrong expectations, and if the updated expectation is
sensible.  That way, we will find if the observations we made (your
problem description and my two examples above, and a few examples I
tried manually) covered all what matters, or if there are cases
where they indeed were expecting some sensible results which this
change would break that we missed.  Offhand, I doubt we would find
any case where these extra blank lines are sensible expectations,
though.

>  combine-diff.c  | 2 +-
>  t/t7007-show.sh | 8 ++++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 3b92c448..d2924de 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1331,7 +1331,7 @@ void diff_tree_combined(const unsigned char *sha1,
>  		if (show_log_first && i == 0) {
>  			show_log(rev);
>  
> -			if (rev->verbose_header && opt->output_format)
> +			if (rev->verbose_header && opt->output_format && opt->output_format != DIFF_FORMAT_NO_OUTPUT)

Please wrap this long line, perhaps like:

			if (rev->verbose_header && opt->output_format &&
			    opt->output_format != DIFF_FORMAT_NO_OUTPUT)


>  				printf("%s%c", diff_line_prefix(opt),
>  				       opt->line_termination);
>  		}
> diff --git a/t/t7007-show.sh b/t/t7007-show.sh
> index e41fa00..d76c7db 100755
> --- a/t/t7007-show.sh
> +++ b/t/t7007-show.sh
> @@ -25,6 +25,7 @@ test_expect_success 'set up a bit of history' '
>  	git checkout -b side HEAD^^ &&
>  	test_commit side2 &&
>  	test_commit side3
> +	test_merge merge main3
>  '
>  
>  test_expect_success 'showing two commits' '
> @@ -109,8 +110,11 @@ test_expect_success 'showing range' '
>  '
>  
>  test_expect_success '-s suppresses diff' '
> -	echo main3 >expect &&
> -	git show -s --format=%s main3 >actual &&
> +	cat >expect <<-EOF &&
> +	merge
> +	main3
> +	EOF

Please make it a habit to quote the EOF marker when you are not
placing things that require variable substitutions, to tell the
readers that they can coast their eyes over the here-document
without having to worry about them.  For a short here-document
like this, it does not matter, but these things have a habit of
getting imitated without thinking by others, and it is better to be
defensive and consistent.  I.e.

	cat >expect <<-\EOF &&
        merge
        main3
        EOF

> +	git show -s --format=%s merge main3 >actual &&
>  	test_cmp expect actual
>  '

Re: [BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit

[Max Kirillov]

On Mon, May 12, 2014 at 09:59:39AM -0700, Junio C Hamano wrote:
> A good way to double-check may be to see the fixes to the tests to
> correct their wrong expectations, and if the updated expectation is
> sensible.

I have sent the fixes to tests. To me they look sensible.

Also fixed the things you pointed out.

-- 
Max