Re: [PATCH] cat-file: fix mailmap application for different author and committer

[Junio C Hamano <gitster@pobox.com>]

siddharthasthana31@gmail.com writes:

> From: Siddharth Asthana <siddharthasthana31@gmail.com>
>
> The git cat-file command with --mailmap option fails to apply mailmap
> transformations to the committer field when the author and committer
> identities are different. This occurs due to a missing newline handling
> in apply_mailmap_to_header() after processing each identity line.
> ...
> This ensures that all identity headers in commit and tag objects are
> consistently processed regardless of whether the author and committer
> are the same person.

Nicely described.

While the above explains what is wrong in the current code, it does
not tell if that was buggy from the beginning or we unintentionally
broke it.  It seems that this logic came from e9c1b0e3 (revision:
improve commit_rewrite_person(), 2022-07-19) when a much simpler
version of commit_rewrite_person() that worked on one "person
header" at a time (as there are only two, author and committer,
anyway) was rewritten to use this function, and it was broken during
the rewrite?

> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 4a6242ff99..98dd0ae12f 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -1133,4 +1133,37 @@ test_expect_success 'git cat-file --batch-command returns correct size with --us
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'git cat-file --mailmap works with different author and committer' '
> +	test_when_finished "rm .mailmap" &&
> +	cat >.mailmap <<-\EOF &&
> +	Mailmapped User <mailmapped-user@gitlab.com> C O Mitter <committer@example.com>
> +	EOF
> +	git commit --allow-empty -m "different author/committer" \
> +		--author="Different Author <different@example.com>" &&
> +	cat >expect <<-\EOF &&
> +	author Different Author <different@example.com>
> +	committer Mailmapped User <mailmapped-user@gitlab.com>
> +	EOF
> +	git cat-file --mailmap commit HEAD >log &&
> +	sed -n "/^author /s/\([^>]*>\).*/\1/p; /^committer /s/\([^>]*>\).*/\1/p" log >actual &&

Perhaps just a  matter of taste, but

	sed -n -e "/^author /s/>.*/>/p" -e "/^committer /s/>.*/>/p"

may be easier to read and more portable (as some implementation of
sed is picky about semicolon concatenated multiple commands).

> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git cat-file --mailmap maps both author and committer when both need mapping' '
> +	test_when_finished "rm .mailmap" &&
> +	cat >.mailmap <<-\EOF &&
> +	Mapped Author <mapped-author@example.com> <different@example.com>
> +	Mapped Committer <mapped-committer@example.com> C O Mitter <committer@example.com>
> +	EOF
> +	git commit --allow-empty -m "both author and committer mapped" \
> +		--author="Different Author <different@example.com>" &&
> +	cat >expect <<-\EOF &&
> +	author Mapped Author <mapped-author@example.com>
> +	committer Mapped Committer <mapped-committer@example.com>
> +	EOF
> +	git cat-file --mailmap commit HEAD >log &&
> +	sed -n "/^author /s/\([^>]*>\).*/\1/p; /^committer /s/\([^>]*>\).*/\1/p" log >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done