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

Max Kirillov· May 11, 2014, 23:25 UTC · lore

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

Junio C Hamano· May 12, 2014, 16:59 UTC · re: Max Kirillov · lore

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

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.

Show 5 quoted lines

> 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.

Show 14 quoted lines

>  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)

Show 25 quoted lines

>  				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
>  '

Max Kirillov· May 12, 2014, 23:26 UTC · re: Junio C Hamano · lore

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

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