On 11/06/25 21:23, Junio C Hamano wrote:
siddharthasthana31@xxxxxxxxx writes:
From: Siddharth Asthana <siddharthasthana31@xxxxxxxxx>
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@xxxxxxxxxx> C O Mitter <committer@xxxxxxxxxxx>
+ EOF
+ git commit --allow-empty -m "different author/committer" \
+ --author="Different Author <different@xxxxxxxxxxx>" &&
+ cat >expect <<-\EOF &&
+ author Different Author <different@xxxxxxxxxxx>
+ committer Mailmapped User <mailmapped-user@xxxxxxxxxx>
+ 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@xxxxxxxxxxx> <different@xxxxxxxxxxx>
+ Mapped Committer <mapped-committer@xxxxxxxxxxx> C O Mitter <committer@xxxxxxxxxxx>
+ EOF
+ git commit --allow-empty -m "both author and committer mapped" \
+ --author="Different Author <different@xxxxxxxxxxx>" &&
+ cat >expect <<-\EOF &&
+ author Mapped Author <mapped-author@xxxxxxxxxxx>
+ committer Mapped Committer <mapped-committer@xxxxxxxxxxx>
+ 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
