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

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

[siddharthasthana31]

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.

When rewrite_ident_line() processes an identity, it stops at the end
of the identity data (e.g., "Author Name <email> timestamp"), but
doesn't account for the trailing newline. The current code adds the
identity length to buf_offset but fails to advance past the newline
character. This causes the next iteration to start parsing from the
newline instead of the beginning of the next header line, making it
impossible to match subsequent headers like "committer".

Additionally, rewrite_ident_line() may reallocate the buffer during
its operation. Any code using pointers into the old buffer would be
using invalid memory after such a reallocation.

Let's fix this by addressing both issues:
1. After processing an identity line, we now check if we're at a
   newline and advance past it, ensuring the next header line is
   parsed correctly.
2. We recompute the buffer position after rewrite_ident_line() to
   handle potential buffer reallocation.

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.

Reported-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Reviewed-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
---
 ident.c            |  4 ++++
 t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/ident.c b/ident.c
index 967895d885..281e830573 100644
--- a/ident.c
+++ b/ident.c
@@ -412,6 +412,10 @@ void apply_mailmap_to_header(struct strbuf *buf, const char **header,
 				found_header = 1;
 				buf_offset += endp - line;
 				buf_offset += rewrite_ident_line(person, endp - person, buf, mailmap);
+				/* Recompute endp after potential buffer reallocation */
+				endp = buf->buf + buf_offset;
+				if (*endp == '\n')
+					buf_offset++;
 				break;
 			}
 
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 &&
+	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
-- 
2.49.0

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

[Christian Couder]

On Wed, Jun 11, 2025 at 8:27 AM <siddharthasthana31@gmail.com> wrote:

> Reported-by: Vasilii Iakliushin <viakliushin@gitlab.com>
> Reviewed-by: Christian Couder <christian.couder@gmail.com>

Nit: I reviewed it when you suggested it on a GitLab MR (Merge
Request), but I am not sure it counts unless I also review it here. I
think the "Reviewed-by: ..." trailer is for patches reviewed on the
regular Git mailing list (or maybe on the private Git security list).
So maybe "Helped-by: ..." would have been better in this case.

Anyway I have now reviewed it again and I found it great.

Thanks for working on this!

> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> ---
>  ident.c            |  4 ++++
>  t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/ident.c b/ident.c
> index 967895d885..281e830573 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -412,6 +412,10 @@ void apply_mailmap_to_header(struct strbuf *buf, const char **header,
>                                 found_header = 1;
>                                 buf_offset += endp - line;
>                                 buf_offset += rewrite_ident_line(person, endp - person, buf, mailmap);
> +                               /* Recompute endp after potential buffer reallocation */
> +                               endp = buf->buf + buf_offset;
> +                               if (*endp == '\n')
> +                                       buf_offset++;

Yeah, without this, in the next iteration of the `for (;;) { ... }`
loop after the "author" header has been found, we have:

        line = buf->buf + buf_offset;

which sets `line` to something like "\ncommitter C O Mitter
<committer@example.com> ...", and then:

        if (!*line || *line == '\n')
            return; /* End of headers */

which just returns as `*line` is indeed '\n'.

>                                 break;
>                         }

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Wed, Jun 11, 2025 at 8:27 AM <siddharthasthana31@gmail.com> wrote:
>
>> Reported-by: Vasilii Iakliushin <viakliushin@gitlab.com>
>> Reviewed-by: Christian Couder <christian.couder@gmail.com>
>
> Nit: I reviewed it when you suggested it on a GitLab MR (Merge
> Request), but I am not sure it counts unless I also review it here. I
> think the "Reviewed-by: ..." trailer is for patches reviewed on the
> regular Git mailing list (or maybe on the private Git security list).
> So maybe "Helped-by: ..." would have been better in this case.

If somebody (including me) sees your reviewed-by on a patch and do
not remember they saw your review here, they might ask, but as long
as you are OK to have your name on the reviewed-by trailer, meaning
you have carefully inspected exactly the same version of the patch
and are willing to stand behind the change, it is perfectly fine.

On the other hand, if you see somebody attach your reviewed-by to a
patch that you didn't review, or is substantially different from the
one you reviewed, please raise a stink about it.  I do not think
this case is such a case.

> Anyway I have now reviewed it again and I found it great.
>
> Thanks for working on this!
>
>> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
>> ---

Thanks.

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

[Junio C Hamano]

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

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

[Eric Sunshine]

On Wed, Jun 11, 2025 at 11:55 AM Junio C Hamano <gitster@pobox.com> wrote:
> From: Siddharth Asthana <siddharthasthana31@gmail.com>
> > +     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).

For what it's worth, Git test scripts already contain a fair number of
uses of semicolon-separated `sed` commands, and we haven't heard of
any problems with them; not even from the very old and quite picky
Solaris `sed` (or was it the ancient SunOS `sed`?).

The only case I can think of in which there was a semicolon-related
problem (and perhaps what you're thinking of) was when a recent
patch[*] neglected to insert a semicolon where it was expected. That
particular case involved a missing semicolon before a closing brace:

    sed -n '/ version /{p;q}'

which should have been:

    sed -n '/ version /{p;q;}'

To summarize: Using semicolon-separated commands is safe and portable;
I don't think there is any evidence that doing so would be
problematic. Whether to use semicolon-separated commands or multiple
`-e` arguments is subjective, and I don't believe the project has
expressed a preference for one for or the other.

[*]: https://lore.kernel.org/git/CAPig+cR+ESNg4tV1G6jbKKeRKABD053qZcG0BoFuQ7aC+1tGYw@mail.gmail.com/

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> may be easier to read and more portable (as some implementation of
>> sed is picky about semicolon concatenated multiple commands).
>
> For what it's worth, Git test scripts already contain a fair number of
> uses of semicolon-separated `sed` commands, and we haven't heard of
> any problems with them; not even from the very old and quite picky
> Solaris `sed` (or was it the ancient SunOS `sed`?).

I think it was of BSD lineage, but I phrased it poorly.

>     sed -n '/ version /{p;q}'

This pattern did cause issues in the past.  I was hoping that we can
avoid it by training our developers to avoid concatenation with
semicolons in general, but {grouped} commands cannot be fed without
properly using semicolons anyway, so it would not help to just
generally avoid use of semicolons.

On the other hand, the suggestion that was given in the message ...

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

... is much shorter, simpler and easier to understand.  With the
added benefit that you can even line-wrap sensibly

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

there really isn't a good reason not to adopt the style, compared to
the way the patch was originally written.

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

[Siddharth Asthana]

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

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

[siddharthasthana31]

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.

When rewrite_ident_line() processes an identity, it stops at the end
of the identity data (e.g., "Author Name <email> timestamp"), but
doesn't account for the trailing newline. The current code adds the
identity length to buf_offset but fails to advance past the newline
character. This causes the next iteration to start parsing from the
newline instead of the beginning of the next header line, making it
impossible to match subsequent headers like "committer".

Additionally, rewrite_ident_line() may reallocate the buffer during
its operation. Any code using pointers into the old buffer would be
using invalid memory after such a reallocation.

This bug was introduced in e9c1b0e3 (revision: improve
commit_rewrite_person(), 2022-07-19) when the much simpler version of
commit_rewrite_person() that worked on one "person header" at a time
was rewritten to use the current apply_mailmap_to_header() function.
The original implementation processed author and committer separately,
but the rewrite introduced this loop-based approach that failed to
properly handle the transition between identity lines.

Let's fix this by addressing both issues:
1. After processing an identity line, we now check if we're at a
   newline and advance past it, ensuring the next header line is
   parsed correctly.
2. We recompute the buffer position after rewrite_ident_line() to
   handle potential buffer reallocation.

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.

Reported-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Reviewed-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
---
 ident.c            |  4 ++++
 t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/ident.c b/ident.c
index 967895d885..281e830573 100644
--- a/ident.c
+++ b/ident.c
@@ -412,6 +412,10 @@ void apply_mailmap_to_header(struct strbuf *buf, const char **header,
 				found_header = 1;
 				buf_offset += endp - line;
 				buf_offset += rewrite_ident_line(person, endp - person, buf, mailmap);
+				/* Recompute endp after potential buffer reallocation */
+				endp = buf->buf + buf_offset;
+				if (*endp == '\n')
+					buf_offset++;
 				break;
 			}
 
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 4a6242ff99..74b7ddccb2 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 -e "/^author /s/>.*/>/p" -e "/^committer /s/>.*/>/p" log >actual &&
+	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 -e "/^author /s/>.*/>/p" -e "/^committer /s/>.*/>/p" log >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.49.0

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

[Christian Couder]

On Fri, Jun 13, 2025 at 1:58 PM <siddharthasthana31@gmail.com> wrote:

[...]

> This bug was introduced in e9c1b0e3 (revision: improve
> commit_rewrite_person(), 2022-07-19) when the much simpler version of
> commit_rewrite_person() that worked on one "person header" at a time
> was rewritten to use the current apply_mailmap_to_header() function.
> The original implementation processed author and committer separately,
> but the rewrite introduced this loop-based approach that failed to
> properly handle the transition between identity lines.

Thanks for adding this context and improving the `sed` invocation in
the tests! Happy to stand behind this change too :-)

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Jun 13, 2025 at 1:58 PM <siddharthasthana31@gmail.com> wrote:
>
> [...]
>
>> This bug was introduced in e9c1b0e3 (revision: improve
>> commit_rewrite_person(), 2022-07-19) when the much simpler version of
>> commit_rewrite_person() that worked on one "person header" at a time
>> was rewritten to use the current apply_mailmap_to_header() function.
>> The original implementation processed author and committer separately,
>> but the rewrite introduced this loop-based approach that failed to
>> properly handle the transition between identity lines.
>
> Thanks for adding this context and improving the `sed` invocation in
> the tests! Happy to stand behind this change too :-)

Thanks, all.  Queued.  Let's mark it for 'next', hoping that the fix
will be in the first batch in the post 2.50 cycle.