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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 16, 2021, 16:36 UTC
Message-ID: <xmqq8s11s5wk.fsf@gitster.g>
In-Reply-To: <043edd52-c666-0939-bf6d-51b0d225157e@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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

To be quite honest, I am pretty happy with the tests as posted, and the choice among various ways we are discussing depends on what the likely mode of breakage we expect.

The breakage we are expecting to catch in the second hunk of your patch is somehow "rebase -i" fails to keep the authorship of amended-goodbye and ends up making the commit at HEAD under different authorship.

 - A likely source of a different authorship information that would
   be recorded, when such a bug gets introduced, is from the
   environment (i.e. GIT_AUTHOR_NAME etc. that test-lib.sh sets up).
   This can happen by a new bug in the test added before the second
   hunk of your patch we see below, and with or without this patch,
   such a bug in the test will not be caught.

 - Or amended-goodbye may by a test bug have been recorded under a
   wrong authorship information to begin with, and if it were done
   as the default author we use in our tests (i.e. the bug you fixed
   in your patch that started this thread).  If we reintroduced such
   a bug, the second hunk of this patch will help.

 - Or the variable $another_author gets clobbered in a future change
   between the two hunks of this patch, and the check in the second
   hunk would be broken.

Overall, I do not think any of the above breakage is so likely to happen, and that is why I am happy with the tests as posted. The necessity for a plain shell variable to retain its value for such a long haul across tests the patch below introduces may be making the test more brittle than safer.

So...

Show 31 quoted lines

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