forcing an in-body From header

Commit Message

Rasmus Villemoes Aug. 26, 2022, 1:17 p.m. UTC

Hi,

Some mailing lists mangle the real From: header, making it a little hard
for maintainers to apply patches directly using 'git am'. See e.g.
https://lists.openembedded.org/g/openembedded-core/message/166515 . One
way to work around that is by having an in-body From: "header".

However, merely setting sendemail.from or format.from is not enough to
get such a header, if the value happens to be identical to the patch
author (which it would usually be). So, could we get some config knob
and/or command line switch to force an in-body from header? Then one
could set that on a per-project basis, for projects with such mailing lists.

I looked into the code, and while this is obviously just a hacky patch
to see that I found the right spot, it doesn't seem to be too hard to
implement properly.


                        strbuf_addstr(&buf, "From: ");

Thanks,
Rasmus

Comments

Konstantin Ryabitsev Aug. 26, 2022, 4:48 p.m. UTC | #1

On Fri, Aug 26, 2022 at 03:17:34PM +0200, Rasmus Villemoes wrote:
> Hi,
> 
> Some mailing lists mangle the real From: header, making it a little hard
> for maintainers to apply patches directly using 'git am'. See e.g.
> https://lists.openembedded.org/g/openembedded-core/message/166515 . One
> way to work around that is by having an in-body From: "header".

This is only tangentially related to the problem at hand, but the list in
question is also mirrored on lore.kernel.org:

https://lore.kernel.org/openembedded-core/[e-mail]/

The mirroring is done with a direct push hook from groups.io, which is why we
get messages without all the groups.io mangling.

> However, merely setting sendemail.from or format.from is not enough to
> get such a header, if the value happens to be identical to the patch
> author (which it would usually be). So, could we get some config knob
> and/or command line switch to force an in-body from header? Then one
> could set that on a per-project basis, for projects with such mailing lists.

I agree that it's a nice feature to have, though I would put this into the
sendemail config instead of using an env variable, something like:

    [sendemail]
      in-body-headers = From

-K

Junio C Hamano Aug. 26, 2022, 5:11 p.m. UTC | #2

Rasmus Villemoes <[e-mail]> writes:

> I looked into the code, and while this is obviously just a hacky patch
> to see that I found the right spot, it doesn't seem to be too hard to
> implement properly.

I agree with you on all points in the above sentence ;-) I do not
think anybody minds if a command line option to "git format-patch"
added in builtin/log.c::cmd_format_patch() and a new configuration
variable taught to builtin/log.c::git_format_config() that gives
the default value for the new "force_in_body_from" member that will
be added to "struct rev_info" and relayed to "pretty_print_context"
when log-tree.c::fmt_output_commit() calls format_commit_messages().

Or something like that.

Junio C Hamano Aug. 26, 2022, 5:20 p.m. UTC | #3

Konstantin Ryabitsev <[e-mail]> writes:

> I agree that it's a nice feature to have, though I would put this into the
> sendemail config instead of using an env variable, something like:
>
>     [sendemail]
>       in-body-headers = From

A change only for send-email does sound quite attractive.

I encourage folks to run format-patch first, make amends as needed
in the output files, proofread the result once again, all before
finally handing it to send-email for sending out.  If a "force
in-body headers" command line option plus a configuration variable
added to "git send-email" would work for them, I would be OK with
such a change.

There may be folks who do not use "git send-email" to send out their
patches, and a change to "git format-patch" may help them, though.

Thanks.

Rasmus Villemoes Aug. 29, 2022, 11:56 a.m. UTC | #4

On 26/08/2022 19.20, Junio C Hamano wrote:
> Konstantin Ryabitsev <[e-mail]> writes:
> 
>> I agree that it's a nice feature to have, though I would put this into the
>> sendemail config instead of using an env variable, something like:
>>
>>     [sendemail]
>>       in-body-headers = From
> 
> A change only for send-email does sound quite attractive.
> 
> I encourage folks to run format-patch first, make amends as needed
> in the output files, proofread the result once again, all before
> finally handing it to send-email for sending out. 

That's at least my usual workflow, yes.

 If a "force
> in-body headers" command line option plus a configuration variable
> added to "git send-email" would work for them, I would be OK with
> such a change.

It would work for me, except that if I am to rely on git send-email to
munge the body of the file(s) I pass to it, I'd really like a way for
--dry-run to reassure me that that will actually happen. Currently that
only prints the headers, which is quite useful to check that the To and
Cc are as expected (especially when one has some to-cmd or cc-cmd
configured).

> There may be folks who do not use "git send-email" to send out their
> patches, and a change to "git format-patch" may help them, though.

Maybe, yes. I also stumbled on this paragraph in git format-patch --help
which would probably need some adjustment

           Note that this option is only useful if you are actually
sending the
           emails and want to identify yourself as the sender, but
retain the
           original author (and git am will correctly pick up the in-body
           header). Note also that git send-email already handles this
           transformation for you, and this option should not be used if
you are
           feeding the result to git send-email.

I don't know how git send-email behaves if there already is an in-body
From, or if it is different than the one that send-email was about to
add itself.

Somehow I got the cover letter of the mini-series, but neither of the
two actual patches. I can't answer the big-picture question of whether
this belongs in format-patch, send-email or both, but:

> * Should it be "inbody" or "in-body"?

in-body, I think.

> * Should it have a corresponding configuration variable?

Most definitely yes, it's something I'd want to set for certain projects
in the local .git/config file, and not a command line option I want to
have to remember when doing the occasional contribution for those projects.

Thanks for the quick responses and initial draft work on this.

Rasmus

Patch

diff --git a/pretty.c b/pretty.c
index ee6114e3f0..8b9ef6f644 100644
--- a/pretty.c
+++ b/pretty.c
@@ -503,7 +503,7 @@  void pp_user_info(struct pretty_print_context *pp,
                map_user(pp->mailmap, &mailbuf, &maillen, &namebuf,
&namelen);

        if (cmit_fmt_is_mail(pp->fmt)) {
-               if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
+               if (pp->from_ident && (ident_cmp(pp->from_ident, &ident)
|| getenv("GIT_FORCE_BODY_FROM"))) {
                        struct strbuf buf = STRBUF_INIT;