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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 11/06/25 21:23, Junio C Hamano wrote:
> 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.

Thanks for the detailed review and suggestions!

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

You are absolutely right! I should have included this historical context
in the commit message. The bug was indeed introduced during that rewrite
in e9c1b0e3. The original implementation processed author and committer
separately, but the rewrite introduced the loop-based approach that failed
to properly handle the transition between identity lines.

>
>> 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).

Good point about portability and readability. The `-e` flag approach
is indeed cleaner and more maintainable. I'll update both test cases
to use this format.

I will send a v2 with:
1. Updated commit message explaining the historical context from e9c1b0e3
2. Improved sed commands using the `-e` flag format

Thanks

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