Re: [PATCH 1/3] t3403: fix commit authorship

Johannes Schindelin [off-list ref] writes:

quoted

quoted

Good point.  The commit tagged with amended-goodbye is later used in
some tests that ensure the author ident does not change across a
rebase.  If this commit gets created without authorship customized
(i.e. before Phillip's fix), we would not catch a possible breakage
to make rebase discard the original authorship information.

But with this fix, we now can catch such a breakage.

I'll expand the commit message to make that clear

Maybe you could even add a `test another.author@example.com = $(git show
-s --format=%ae HEAD)`?

The version I have from Phillip has updated log message already, but
not with such a regression prevention.

The test that the patch under discussion corrects does this:

    test_expect_success 'correct authorship when committing empty pick' '
        test_when_finished "git rebase --abort" &&
        test_must_fail git rebase -i --onto goodbye \
                amended-goodbye^ amended-goodbye &&
        git commit --allow-empty &&
        git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
        git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
        test_cmp expect actual
    '

to ensure that the authorship is the same between the original
(i.e. amended-goodbye) and the rebased (i.e. HEAD), with the
expectation that a bug may lose the authorship and instead use the
default one used in the test suite.  What this test truly cares is
not that amended-goodbye was authored by another.author, but it was
not written by the default author.

We could test both, like the attached patch, for completeness.  The
first half makes sure amended-goodbye (the original) was written by
the another.author, and the other one makes sure that author is not
the one we use to prepare commits for the tests by default.

I do not think the latter is actually a good idea ("As long as the
command produces a result different from THIS, any random garbage is
accepted" does not make a good test), so perhaps the first half
would be good enough.

 t/t3403-rebase-skip.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git c/t/t3403-rebase-skip.sh w/t/t3403-rebase-skip.sh
index e26762d0b2..1405720767 100755
--- c/t/t3403-rebase-skip.sh
+++ w/t/t3403-rebase-skip.sh

@@ -40,6 +40,14 @@ test_expect_success setup '
 	test_tick &&
 	git tag amended-goodbye &&
 
+	# Make sure the authorship info is different from the default one
+	echo "Another Author <another.author@example.com>" >expect &&
+	git log --pretty=format:"%an <%ae>" -1 amended-goodbye >actual &&
+	test_cmp expect actual &&
+
+	git log --pretty=format:"%an <%ae>" -1 goodbye >unexpect &&
+	! test_cmp unexpect actual &&
+
 	git checkout -f skip-reference &&
 	echo moo > hello &&
 	git commit -a -m "we should skip this" &&

On 15/08/2021 18:36, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

quoted

quoted

Good point.  The commit tagged with amended-goodbye is later used in
some tests that ensure the author ident does not change across a
rebase.  If this commit gets created without authorship customized
(i.e. before Phillip's fix), we would not catch a possible breakage
to make rebase discard the original authorship information.

But with this fix, we now can catch such a breakage.

I'll expand the commit message to make that clear

Maybe you could even add a `test another.author@example.com = $(git show
-s --format=%ae HEAD)`?

The version I have from Phillip has updated log message already, but
not with such a regression prevention.

The test that the patch under discussion corrects does this:

     test_expect_success 'correct authorship when committing empty pick' '
         test_when_finished "git rebase --abort" &&
         test_must_fail git rebase -i --onto goodbye \
                 amended-goodbye^ amended-goodbye &&
         git commit --allow-empty &&
         git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
         git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
         test_cmp expect actual
     '

I wonder if it would be better to hard code the author in this test rather
than rather than relying on git log like this

diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index e26762d0b2..c90e32817f 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh

@@ -34,9 +34,10 @@ test_expect_success setup '
         git tag reverted-goodbye &&
         git checkout goodbye &&
         test_tick &&
-       GIT_AUTHOR_NAME="Another Author" \
-               GIT_AUTHOR_EMAIL="another.author@example.com" \
-               git commit --amend --no-edit -m amended-goodbye &&
+       another_author="Another Author <another.author@example.com>" &&
+       git commit --amend --no-edit -m amended-goodbye \
+               --author="$another_author" --date="$GIT_AUTHOR_DATE" &&
+       another_author="$another_author $GIT_AUTHOR_DATE" &&
         test_tick &&
         git tag amended-goodbye &&
  

@@ -110,8 +111,10 @@ test_expect_success 'correct authorship when committing empty pick' '
         test_must_fail git rebase -i --onto goodbye \
                 amended-goodbye^ amended-goodbye &&
         git commit --allow-empty &&
-       git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
-       git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
+       git log --pretty=format:"$another_author%n%B" -1 amended-goodbye \
+                >expect &&
+       git log --date=raw --pretty=format:"%an <%ae> %ad%n%B" -1 HEAD \
+               >actual &&
         test_cmp expect actual
  '