[PATCH v2 0/2] fix 'git show -s' to not add extra terminator after merge commit

[PATCH v2 0/2] fix 'git show -s' to not add extra terminator after merge commit

[Max Kirillov]

Since v1:
* add Signed-off-by
* remove notion about '--format=%s' - found it in the
  documentation

Since RFC:
* fix the CC issues
* add fixes for existing tests

Max Kirillov (2):
  git-show: fix 'git show -s' to not add extra terminator after merge commit
  t: git-show: adapt tests to fixed 'git show'

 combine-diff.c                |  3 ++-
 t/t1507-rev-parse-upstream.sh |  2 +-
 t/t7007-show.sh               |  8 ++++++--
 t/t7600-merge.sh              | 11 +++++------
 4 files changed, 14 insertions(+), 10 deletions(-)

-- 
1.8.5.2.421.g4cdf8d0

[PATCH v2 1/2] 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
after 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".

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

Signed-off-by: Max Kirillov <max@max630.net>
---
 combine-diff.c  | 3 ++-
 t/t7007-show.sh | 8 ++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 3b92c448..ff6ceaf 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1331,7 +1331,8 @@ 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..de22812 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

[PATCH v2 2/2] t: git-show: adapt tests to fixed 'git show'

[Max Kirillov]

'git show' used to print extra newline after merge commit, and it was
recorded so into the test reference data. Now when the behavior is
fixed, the tests should be updated.

Signed-off-by: Max Kirillov <max@max630.net>
---
 t/t1507-rev-parse-upstream.sh |  2 +-
 t/t7600-merge.sh              | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 2a19e79..672280b 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -100,7 +100,7 @@ test_expect_success 'merge my-side@{u} records the correct name' '
 	git branch -D new ;# can fail but is ok
 	git branch -t new my-side@{u} &&
 	git merge -s ours new@{u} &&
-	git show -s --pretty=format:%s >actual &&
+	git show -s --pretty=tformat:%s >actual &&
 	echo "Merge remote-tracking branch ${sq}origin/side${sq}" >expect &&
 	test_cmp expect actual
 )
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 10aa028..b164621 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -57,11 +57,10 @@ create_merge_msgs () {
 		git log --no-merges ^HEAD c2 c3
 	} >squash.1-5-9 &&
 	: >msg.nologff &&
-	echo >msg.nolognoff &&
+	: >msg.nolognoff &&
 	{
 		echo "* tag 'c3':" &&
-		echo "  commit 3" &&
-		echo
+		echo "  commit 3"
 	} >msg.log
 }
 
@@ -71,7 +70,7 @@ verify_merge () {
 	git diff --exit-code &&
 	if test -n "$3"
 	then
-		git show -s --pretty=format:%s HEAD >msg.act &&
+		git show -s --pretty=tformat:%s HEAD >msg.act &&
 		test_cmp "$3" msg.act
 	fi
 }
@@ -620,10 +619,10 @@ test_expect_success 'merge early part of c2' '
 	git tag c6 &&
 	git branch -f c5-branch c5 &&
 	git merge c5-branch~1 &&
-	git show -s --pretty=format:%s HEAD >actual.branch &&
+	git show -s --pretty=tformat:%s HEAD >actual.branch &&
 	git reset --keep HEAD^ &&
 	git merge c5~1 &&
-	git show -s --pretty=format:%s HEAD >actual.tag &&
+	git show -s --pretty=tformat:%s HEAD >actual.tag &&
 	test_cmp expected.branch actual.branch &&
 	test_cmp expected.tag actual.tag
 '
-- 
1.8.5.2.421.g4cdf8d0

Re: [PATCH v2 1/2] git-show: fix 'git show -s' to not add extra terminator after merge commit

[Johannes Sixt]

Am 5/13/2014 1:10, schrieb Max Kirillov:
> --- 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
>  '

Broken &&-chain.

-- Hannes

Re: [PATCH v2 2/2] t: git-show: adapt tests to fixed 'git show'

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> 'git show' used to print extra newline after merge commit, and it was
> recorded so into the test reference data. Now when the behavior is
> fixed, the tests should be updated.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---

Hmph.  Having these as two separate commits would mean that 1/2
alone will break the test, hurting bisectability a little bit.  The
necessary adjustments in this patch is small enough that we may be
better off squashing them into one.

>  t/t1507-rev-parse-upstream.sh |  2 +-
>  t/t7600-merge.sh              | 11 +++++------
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
> index 2a19e79..672280b 100755
> --- a/t/t1507-rev-parse-upstream.sh
> +++ b/t/t1507-rev-parse-upstream.sh
> @@ -100,7 +100,7 @@ test_expect_success 'merge my-side@{u} records the correct name' '
>  	git branch -D new ;# can fail but is ok
>  	git branch -t new my-side@{u} &&
>  	git merge -s ours new@{u} &&
> -	git show -s --pretty=format:%s >actual &&
> +	git show -s --pretty=tformat:%s >actual &&
>  	echo "Merge remote-tracking branch ${sq}origin/side${sq}" >expect &&
>  	test_cmp expect actual
>  )

This seems to me that it is updating how the output is produced, not
updating the expected outputs from commands with arguments we have
been testing.  I have been expecting to see changes to the pieces of
the code that prepare the expected output, so that we can compare
old expectations, which would have been expecting something strange,
with new expectations from the updated code, which would show that
the new behaviour is a welcome change because it would produce more
sensible output.

Having said all that, for this particular test piece, I agree with
the patch that using --pretty=tformat:%s is a lot more sensible and
using --pretty=format:%s and expecting its output to be terminated
with the newline was an unrealistic test.  After all, "tformat" is
the version with "line terminator" semantics, as opposed to "item
separator" semantics offered by "--pretty=format:", and the test
merely was depending on the bug to expect a complete line output
(i.e. "echo" without "-n"), which you fixed.  The new test makes a
lot more sense and is relevant to the real world use, and I would
have preferred to see it explained as such in the log message.

> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 10aa028..b164621 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -57,11 +57,10 @@ create_merge_msgs () {
>  		git log --no-merges ^HEAD c2 c3
>  	} >squash.1-5-9 &&
>  	: >msg.nologff &&
> -	echo >msg.nolognoff &&
> +	: >msg.nolognoff &&
>  	{
>  		echo "* tag 'c3':" &&
> -		echo "  commit 3" &&
> -		echo
> +		echo "  commit 3"
>  	} >msg.log
>  }

These are updating the expectation.  We used to expect an
unnecessary trailing blank line, and now we do not, which clearly
shows that the previous fix is a welcome change.

> @@ -71,7 +70,7 @@ verify_merge () {
>  	git diff --exit-code &&
>  	if test -n "$3"
>  	then
> -		git show -s --pretty=format:%s HEAD >msg.act &&
> +		git show -s --pretty=tformat:%s HEAD >msg.act &&
>  		test_cmp "$3" msg.act
>  	fi
>  }

It is hard to judge what is fed as "$3" here without context, but
this is in line with the "--pretty=tformat aka --format is the
normal thing to use" we saw in 1507.  The same for the other hunk.

> @@ -620,10 +619,10 @@ test_expect_success 'merge early part of c2' '
>  	git tag c6 &&
>  	git branch -f c5-branch c5 &&
>  	git merge c5-branch~1 &&
> -	git show -s --pretty=format:%s HEAD >actual.branch &&
> +	git show -s --pretty=tformat:%s HEAD >actual.branch &&
>  	git reset --keep HEAD^ &&
>  	git merge c5~1 &&
> -	git show -s --pretty=format:%s HEAD >actual.tag &&
> +	git show -s --pretty=tformat:%s HEAD >actual.tag &&
>  	test_cmp expected.branch actual.branch &&
>  	test_cmp expected.tag actual.tag
>  '

How about squashing these two into a single patch, and at the end of
the log message for 1/2, add this to explain the changes to the
test:

    Also existing tests are updated to demonstrate the new
    behaviour.  Earlier, the tests that used "git show -s
    --pretty=format:%s", even though "--pretty=format:%s" calls for
    item separator semantics and does not ask for the terminating
    newline after the last item, expected the output to end with
    such a newline.  They were relying on the buggy behaviour.  Use
    of "--format=%s", which is equivalent to "--pretty=tformat:%s"
    that asks for a terminating newline after each item, is a more
    realistic way to use the command.

    The update to verify_merge in t7600 adjusts the the merge
    message that used to expect an extra blank line in the output,
    which has been eliminated with this fix.

or something like that?

Re: [PATCH v2 2/2] t: git-show: adapt tests to fixed 'git show'

[Max Kirillov]

On Tue, May 13, 2014 at 12:26:42PM -0700, Junio C Hamano wrote:
> Hmph.  Having these as two separate commits would mean that 1/2
> alone will break the test, hurting bisectability a little bit.  The
> necessary adjustments in this patch is small enough that we may be
> better off squashing them into one.

Ok, will squash them.

>>  t/t1507-rev-parse-upstream.sh |  2 +-
>>  t/t7600-merge.sh              | 11 +++++------
>>  2 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
>> index 2a19e79..672280b 100755
>> --- a/t/t1507-rev-parse-upstream.sh
>> +++ b/t/t1507-rev-parse-upstream.sh
>> @@ -100,7 +100,7 @@ test_expect_success 'merge my-side@{u} records the correct name' '
>>  	git branch -D new ;# can fail but is ok
>>  	git branch -t new my-side@{u} &&
>>  	git merge -s ours new@{u} &&
>> -	git show -s --pretty=format:%s >actual &&
>> +	git show -s --pretty=tformat:%s >actual &&
>>  	echo "Merge remote-tracking branch ${sq}origin/side${sq}" >expect &&
>>  	test_cmp expect actual
>>  )
> 
> This seems to me that it is updating how the output is produced, not
> updating the expected outputs from commands with arguments we have
> been testing.  I have been expecting to see changes to the pieces of
> the code that prepare the expected output, so that we can compare
> old expectations, which would have been expecting something strange,
> with new expectations from the updated code, which would show that
> the new behaviour is a welcome change because it would produce more
> sensible output.
> 
> Having said all that, for this particular test piece, I agree with
> the patch that using --pretty=tformat:%s is a lot more sensible and
> using --pretty=format:%s and expecting its output to be terminated
> with the newline was an unrealistic test.  After all, "tformat" is
> the version with "line terminator" semantics, as opposed to "item
> separator" semantics offered by "--pretty=format:", and the test
> merely was depending on the bug to expect a complete line output
> (i.e. "echo" without "-n"), which you fixed.  The new test makes a
> lot more sense and is relevant to the real world use, and I would
> have preferred to see it explained as such in the log message.

In analogous cases with non-merge commits I have found the
form "--format=...", in t3505-cherry-pick-empty.sh for
example, so I have decided that merges should also use it. The
form "--pretty=tformat:..." seems more explicit to me, so I
have chosen this one.

Will add the message as you have suggested.

>> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
>> index 10aa028..b164621 100755
>> --- a/t/t7600-merge.sh
>> +++ b/t/t7600-merge.sh
>> @@ -57,11 +57,10 @@ create_merge_msgs () {
>>  		git log --no-merges ^HEAD c2 c3
>>  	} >squash.1-5-9 &&
>>  	: >msg.nologff &&
>> -	echo >msg.nolognoff &&
>> +	: >msg.nolognoff &&
>>  	{
>>  		echo "* tag 'c3':" &&
>> -		echo "  commit 3" &&
>> -		echo
>> +		echo "  commit 3"
>>  	} >msg.log
>>  }
> 
> These are updating the expectation.  We used to expect an
> unnecessary trailing blank line, and now we do not, which clearly
> shows that the previous fix is a welcome change.
> 
>> @@ -71,7 +70,7 @@ verify_merge () {
>>  	git diff --exit-code &&
>>  	if test -n "$3"
>>  	then
>> -		git show -s --pretty=format:%s HEAD >msg.act &&
>> +		git show -s --pretty=tformat:%s HEAD >msg.act &&
>>  		test_cmp "$3" msg.act
>>  	fi
>>  }
> 
> It is hard to judge what is fed as "$3" here without context, but
> this is in line with the "--pretty=tformat aka --format is the
> normal thing to use" we saw in 1507.  The same for the other hunk.
> 
>> @@ -620,10 +619,10 @@ test_expect_success 'merge early part of c2' '
>>  	git tag c6 &&
>>  	git branch -f c5-branch c5 &&
>>  	git merge c5-branch~1 &&
>> -	git show -s --pretty=format:%s HEAD >actual.branch &&
>> +	git show -s --pretty=tformat:%s HEAD >actual.branch &&
>>  	git reset --keep HEAD^ &&
>>  	git merge c5~1 &&
>> -	git show -s --pretty=format:%s HEAD >actual.tag &&
>> +	git show -s --pretty=tformat:%s HEAD >actual.tag &&
>>  	test_cmp expected.branch actual.branch &&
>>  	test_cmp expected.tag actual.tag
>>  '
> 
> How about squashing these two into a single patch, and at the end of
> the log message for 1/2, add this to explain the changes to the
> test:
> 
>     Also existing tests are updated to demonstrate the new
>     behaviour.  Earlier, the tests that used "git show -s
>     --pretty=format:%s", even though "--pretty=format:%s" calls for
>     item separator semantics and does not ask for the terminating
>     newline after the last item, expected the output to end with
>     such a newline.  They were relying on the buggy behaviour.  Use
>     of "--format=%s", which is equivalent to "--pretty=tformat:%s"
>     that asks for a terminating newline after each item, is a more
>     realistic way to use the command.
> 
>     The update to verify_merge in t7600 adjusts the the merge
>     message that used to expect an extra blank line in the output,
>     which has been eliminated with this fix.
> 
> or something like that?
> 
> 

Re: [PATCH v2 1/2] git-show: fix 'git show -s' to not add extra terminator after merge commit

[Max Kirillov]

On Tue, May 13, 2014 at 07:57:08AM +0200, Johannes Sixt wrote:
> Am 5/13/2014 1:10, schrieb Max Kirillov:
>> --- 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
>>  '
> 
> Broken &&-chain.

Thank you, will fix it.

-- 
Max