[PATCH b4 v2] send: generate patches with --no-mailmap

[PATCH b4 v2] send: generate patches with --no-mailmap

[Matthieu Baerts (NGI0)]

b4 uses 'git show' to generate patches. By default, this command will
use the mailmap file to convert author and committer names and email
addresses if there is a match.

That's good to display the last identity when looking at old commits in
the history, but it doesn't sound like a good idea when sending patches.
'git format-patch' doesn't use the mailmap file, it currently doesn't
even support it, and it is not planned to support it apparently [1].
Note that 'git send-email' might support a new '--mailmap' option, but
it looks like this option will not be enabled by default [2].

In my case, I had to send some patches, including one from someone else
who has to use the email address from his company, but has an entry in
the mailmap file to his @kernel.org email address. When I sent his
patch, b4 seamlessly converted his email address, and caused checkpatch
to complain, because the From and the SoB entries were different.

Here, we ensure the conversion is not done, to avoid any surprise once
the emails are sent.

Link: https://lore.kernel.org/all/20240813-jk-support-mailmap-git-format-patch-v1-1-1aea690ea5dd@gmail.com/ [1]
Link: https://lore.kernel.org/all/20240827-jk-send-email-mailmap-support-v3-0-bec5ba9be391@gmail.com/ [2]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v2:
- Rebased and resolved conflicts.
- Link to v1: https://patch.msgid.link/20240820-send-mailmap-v1-1-7f35789be4b1@kernel.org
---
 src/b4/__init__.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/b4/__init__.py b/src/b4/__init__.py
index 811bbcaf028d78224242433eac9fdbeb03a306b3..a650d114bcc1d0fb534fda3735101458062a8386 100644
--- a/src/b4/__init__.py
+++ b/src/b4/__init__.py
@@ -3463,6 +3463,7 @@ def git_range_to_patches(gitdir: Optional[str], start: str, end: str,
                 '--binary',
                 '--patch-with-stat',
                 '--encoding=utf-8',
+                '--no-mailmap',
                 commit,
             ],
             decode=False,

---
base-commit: dedf88cb947bab87c418b49d975df11f83621692
change-id: 20240820-send-mailmap-ef8ba398450f

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Konstantin Ryabitsev]

On Mon, Sep 23, 2024 at 06:04:31PM +0200, Matthieu Baerts (NGI0) wrote:
> b4 uses 'git show' to generate patches. By default, this command will
> use the mailmap file to convert author and committer names and email
> addresses if there is a match.
> 
> That's good to display the last identity when looking at old commits in
> the history, but it doesn't sound like a good idea when sending patches.
> 'git format-patch' doesn't use the mailmap file, it currently doesn't
> even support it, and it is not planned to support it apparently [1].
> Note that 'git send-email' might support a new '--mailmap' option, but
> it looks like this option will not be enabled by default [2].
> 
> In my case, I had to send some patches, including one from someone else
> who has to use the email address from his company, but has an entry in
> the mailmap file to his @kernel.org email address. When I sent his
> patch, b4 seamlessly converted his email address, and caused checkpatch
> to complain, because the From and the SoB entries were different.

I've been sitting on this for a bit, simply because I'm 50/50 on whether this
is the right thing to do. On the one hand, I see your point, but on the other
it seems that if there is a mailmap entry, then the person would probably like
to prefer to receive mail at their new address, not at their old one.

What does everyone else think?

-K

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Matthieu Baerts]

Hi Konstantin,

On 22/01/2025 17:10, Konstantin Ryabitsev wrote:
> On Mon, Sep 23, 2024 at 06:04:31PM +0200, Matthieu Baerts (NGI0) wrote:
>> b4 uses 'git show' to generate patches. By default, this command will
>> use the mailmap file to convert author and committer names and email
>> addresses if there is a match.
>>
>> That's good to display the last identity when looking at old commits in
>> the history, but it doesn't sound like a good idea when sending patches.
>> 'git format-patch' doesn't use the mailmap file, it currently doesn't
>> even support it, and it is not planned to support it apparently [1].
>> Note that 'git send-email' might support a new '--mailmap' option, but
>> it looks like this option will not be enabled by default [2].
>>
>> In my case, I had to send some patches, including one from someone else
>> who has to use the email address from his company, but has an entry in
>> the mailmap file to his @kernel.org email address. When I sent his
>> patch, b4 seamlessly converted his email address, and caused checkpatch
>> to complain, because the From and the SoB entries were different.
> 
> I've been sitting on this for a bit, simply because I'm 50/50 on whether this
> is the right thing to do. On the one hand, I see your point, but on the other
> it seems that if there is a mailmap entry, then the person would probably like
> to prefer to receive mail at their new address, not at their old one.

Thank you for having looked at this patch!

To me, most of the time, it doesn't make sense to change the 'From'
field when sending a patch: it might remove attributions, create
differences when sending patches for stable, incoherences between the
From and the Signed-off-by, etc.

(In my case, it was not an "old" address, but the owner is not able to
use it to send emails on the mailing list. Still, his employer asked him
to keep his email address in the patches he sends. That's why his work
email is in the mailmap file, even if it is still used in patches, but
not in the mailing lists.)

> What does everyone else think?

I guess that's a particular case, because I happened to send series
including such email address :)

Please note that since then, a new "sendemail.mailmap" option has been
added, set to False by default:

https://git-scm.com/docs/git-config#Documentation/git-config.txt-sendemailmailmap

Maybe this could be read before adding '--no-mailmap'?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Krzysztof Kozlowski]

On 23/01/2025 12:04, Matthieu Baerts wrote:
> Hi Konstantin,
> 
> On 22/01/2025 17:10, Konstantin Ryabitsev wrote:
>> On Mon, Sep 23, 2024 at 06:04:31PM +0200, Matthieu Baerts (NGI0) wrote:
>>> b4 uses 'git show' to generate patches. By default, this command will
>>> use the mailmap file to convert author and committer names and email
>>> addresses if there is a match.
>>>
>>> That's good to display the last identity when looking at old commits in
>>> the history, but it doesn't sound like a good idea when sending patches.
>>> 'git format-patch' doesn't use the mailmap file, it currently doesn't
>>> even support it, and it is not planned to support it apparently [1].
>>> Note that 'git send-email' might support a new '--mailmap' option, but
>>> it looks like this option will not be enabled by default [2].
>>>
>>> In my case, I had to send some patches, including one from someone else
>>> who has to use the email address from his company, but has an entry in
>>> the mailmap file to his @kernel.org email address. When I sent his
>>> patch, b4 seamlessly converted his email address, and caused checkpatch
>>> to complain, because the From and the SoB entries were different.
>>
>> I've been sitting on this for a bit, simply because I'm 50/50 on whether this
>> is the right thing to do. On the one hand, I see your point, but on the other
>> it seems that if there is a mailmap entry, then the person would probably like
>> to prefer to receive mail at their new address, not at their old one.
> 
> Thank you for having looked at this patch!
> 
> To me, most of the time, it doesn't make sense to change the 'From'
> field when sending a patch: it might remove attributions, create
> differences when sending patches for stable, incoherences between the
> From and the Signed-off-by, etc.
> 
> (In my case, it was not an "old" address, but the owner is not able to
> use it to send emails on the mailing list. Still, his employer asked him
> to keep his email address in the patches he sends. That's why his work
> email is in the mailmap file, even if it is still used in patches, but
> not in the mailing lists.)
> 
>> What does everyone else think?
> 
> I guess that's a particular case, because I happened to send series
> including such email address :)

I would expect b4 working here exactly like git format-patch, where you
have commit with old identity (mailmapped or not, does not matter) and
resulting git format-patch (with mailmap entry and default arguments)
produces result as:

   From: old me
   Signed-off-by: old me
   Signed-off-by: new me

(with or without final SoB, discussion is not about DCO)

And above is necessary for all cases of changing employers.

So if b4 does not work like this, then this should be either fixed or +1
for this approach.

Best regards,
Krzysztof

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Konstantin Ryabitsev]

On Thu, Jan 23, 2025 at 12:04:37PM +0100, Matthieu Baerts wrote:
> > What does everyone else think?
> 
> I guess that's a particular case, because I happened to send series
> including such email address :)
> 
> Please note that since then, a new "sendemail.mailmap" option has been
> added, set to False by default:
> 
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-sendemailmailmap
> 
> Maybe this could be read before adding '--no-mailmap'?

Ah, that sounds like the best course of action. We already parse sendemail
configuration options, so looking up if sendemail.mailmap.* is set is the
right way to go.

Thanks for the suggestion! I won't take the patch, but I'll work this in and
give credit.

-K

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Matthieu Baerts]

Hi Konstantin,

On 23/01/2025 19:04, Konstantin Ryabitsev wrote:
> On Thu, Jan 23, 2025 at 12:04:37PM +0100, Matthieu Baerts wrote:
>>> What does everyone else think?
>>
>> I guess that's a particular case, because I happened to send series
>> including such email address :)
>>
>> Please note that since then, a new "sendemail.mailmap" option has been
>> added, set to False by default:
>>
>> https://git-scm.com/docs/git-config#Documentation/git-config.txt-sendemailmailmap
>>
>> Maybe this could be read before adding '--no-mailmap'?
> 
> Ah, that sounds like the best course of action. We already parse sendemail
> configuration options, so looking up if sendemail.mailmap.* is set is the
> right way to go.
> 
> Thanks for the suggestion! I won't take the patch, but I'll work this in and
> give credit.

Thank you for having applied my suggestion [1].

However, there was maybe a small confusion: I think it is important no
to use mailmap for the equivalent of 'git format-patch', but it is still
interesting to continue using it for the equivalent of 'git send-email'.

My issue was that the author's 'From' tag was modified: I wanted to send
a patch from someone else using an address having an entry in the
mailmap file, and b4 changed the 'From' tag, no longer corresponding to
the SoB.

In other words, instead of:

  From: Me
  Subject: Foo

  From: Dev <Original>

  (...)

  Signed-off-by: Dev <Original>

b4 prepared this patch:

  From: Me
  Subject: Foo

  From: Dev <MailMap>  <== Modified :(

  (...)

  Signed-off-by: Dev <Original>  <== Unmodified (good)

=> I don't think b4 should modify the From (due to "git show --mailmap")
by default. This can also happen when sending an "old" patch to stable
for example. Your recent modification fixes that in
__init__.py:git_range_to_patches() by using "git show --no-mailmap" by
default, thank you!

Now there is a second part in your patch: the modifications of the To/Cc
fields. For me, it is OK and maybe better to send the patch to "Dev
<MailMap>" instead of / and "Dev <Original>", because that's what "Dev"
wants according to the mailmap file. This behaviour has been changed
with your modifications in ez.py. Maybe the emails should continue to be
updated to use the mailmap version by default, if sendemail.mailmap is
not set?

So doing this in cmd_send():

 -    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'false'))
 +    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'true'))

WDYT?


[1] https://git.kernel.org/pub/scm/utils/b4/b4.git/commit/?id=8438e306

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Konstantin Ryabitsev]

On Wed, Feb 05, 2025 at 04:46:16PM +0100, Matthieu Baerts wrote:
> b4 prepared this patch:
> 
>   From: Me
>   Subject: Foo
> 
>   From: Dev <MailMap>  <== Modified :(
> 
>   (...)
> 
>   Signed-off-by: Dev <Original>  <== Unmodified (good)

Thank you for explaining that -- I wasn't actually fully understanding the
problem. I thought it would only affect the to/cc entries, therefore my
confusion why would anyone *not* want that. :)

> => I don't think b4 should modify the From (due to "git show --mailmap")
> by default. This can also happen when sending an "old" patch to stable
> for example. Your recent modification fixes that in
> __init__.py:git_range_to_patches() by using "git show --no-mailmap" by
> default, thank you!
> 
> Now there is a second part in your patch: the modifications of the To/Cc
> fields. For me, it is OK and maybe better to send the patch to "Dev
> <MailMap>" instead of / and "Dev <Original>", because that's what "Dev"
> wants according to the mailmap file. This behaviour has been changed
> with your modifications in ez.py. Maybe the emails should continue to be
> updated to use the mailmap version by default, if sendemail.mailmap is
> not set?
> 
> So doing this in cmd_send():
> 
>  -    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'false'))
>  +    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'true'))
> 
> WDYT?

I'll just roll back that part and simplify things back down to just pass
--no-mailmap to the git-show command.

Thanks for the clarification!

-K

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Matthieu Baerts]

On 05/02/2025 17:13, Konstantin Ryabitsev wrote:
> On Wed, Feb 05, 2025 at 04:46:16PM +0100, Matthieu Baerts wrote:

(...)

>> So doing this in cmd_send():
>>
>>  -    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'false'))
>>  +    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'true'))
>>
>> WDYT?
> 
> I'll just roll back that part and simplify things back down to just pass
> --no-mailmap to the git-show command.

Thank you, looks good to me!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Konstantin Ryabitsev]

On Wed, Feb 05, 2025 at 05:29:45PM +0100, Matthieu Baerts wrote:
> >> So doing this in cmd_send():
> >>
> >>  -    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'false'))
> >>  +    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'true'))
> >>
> >> WDYT?
> > 
> > I'll just roll back that part and simplify things back down to just pass
> > --no-mailmap to the git-show command.
> 
> Thank you, looks good to me!

The changes are in place in the latest master. Please follow up if there's
more work that is required.

Thank you,
-K

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Matthieu Baerts]

Hi Konstantin,

On 05/02/2025 21:53, Konstantin Ryabitsev wrote:
> On Wed, Feb 05, 2025 at 05:29:45PM +0100, Matthieu Baerts wrote:
>>>> So doing this in cmd_send():
>>>>
>>>>  -    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'false'))
>>>>  +    mailmap_replace = b4.get_git_bool(sconfig.get('mailmap', 'true'))
>>>>
>>>> WDYT?
>>>
>>> I'll just roll back that part and simplify things back down to just pass
>>> --no-mailmap to the git-show command.
>>
>> Thank you, looks good to me!
> 
> The changes are in place in the latest master.

Great, thanks!

> Please follow up if there's more work that is required.

Sorry, I have one last question about this. When reading this part of
your commit message ...

> The consensus is that we *do* want to apply mailmap updates to the
> actual addresses going into to/cc, just not to the in-body From:
> addresses generated during the "git show" stage.

... I'm wondering if there is still a need to look at the
sendemail.mailmap config: it is now only used to modify the behaviour of
patches generation, and not how/to who they are going to be sent. In
Git, this config option is supposed to act only on the second part.

Don't get me wrong, the current version fixes my issue, and I'm happy
with that! Plus I know I suggested looking at sendemail.mailmap as a
"workaround". But now that the confusion around my issue has been
cleared up, maybe all we need here is to always use "git show
--no-mailmap" and forget about sendemail.mailmap? :)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Konstantin Ryabitsev]

On Thu, Feb 06, 2025 at 06:23:58PM +0100, Matthieu Baerts wrote:
> Sorry, I have one last question about this. When reading this part of
> your commit message ...
> 
> > The consensus is that we *do* want to apply mailmap updates to the
> > actual addresses going into to/cc, just not to the in-body From:
> > addresses generated during the "git show" stage.
> 
> ... I'm wondering if there is still a need to look at the
> sendemail.mailmap config: it is now only used to modify the behaviour of
> patches generation, and not how/to who they are going to be sent. In
> Git, this config option is supposed to act only on the second part.
> 
> Don't get me wrong, the current version fixes my issue, and I'm happy
> with that! Plus I know I suggested looking at sendemail.mailmap as a
> "workaround". But now that the confusion around my issue has been
> cleared up, maybe all we need here is to always use "git show
> --no-mailmap" and forget about sendemail.mailmap? :)

I'm okay if we keep it as-is right now, but we'll keep this in mind for later,
should this come up again.

-K

Re: [PATCH b4 v2] send: generate patches with --no-mailmap

[Matthieu Baerts]

On 06/02/2025 18:47, Konstantin Ryabitsev wrote:
> On Thu, Feb 06, 2025 at 06:23:58PM +0100, Matthieu Baerts wrote:
>> Sorry, I have one last question about this. When reading this part of
>> your commit message ...
>>
>>> The consensus is that we *do* want to apply mailmap updates to the
>>> actual addresses going into to/cc, just not to the in-body From:
>>> addresses generated during the "git show" stage.
>>
>> ... I'm wondering if there is still a need to look at the
>> sendemail.mailmap config: it is now only used to modify the behaviour of
>> patches generation, and not how/to who they are going to be sent. In
>> Git, this config option is supposed to act only on the second part.
>>
>> Don't get me wrong, the current version fixes my issue, and I'm happy
>> with that! Plus I know I suggested looking at sendemail.mailmap as a
>> "workaround". But now that the confusion around my issue has been
>> cleared up, maybe all we need here is to always use "git show
>> --no-mailmap" and forget about sendemail.mailmap? :)
> 
> I'm okay if we keep it as-is right now, but we'll keep this in mind for later,
> should this come up again.

Works for me, thank you!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.