Max Kirillov <max@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html