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

[Junio C Hamano <gitster@pobox.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...

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