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

[Max Kirillov <max@max630.net>]

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