Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2641 bytes --]

[adding git list]

On 9/5/19 7:25 AM, Christian Schoenebeck wrote:

>>>>> How are you sending patches ? With git send-email ? If so, maybe you
>>>>> can
>>>>> pass something like --from='"Christian Schoenebeck"
>>>>> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
>>>>> assume you're sending someone else's patch : it will automatically add
>>>>> an
>>>>> extra From: made out of the commit Author as recorded in the git tree.
>>>
>>> I think it is probably as simple as a 'git config' command to tell git
>>> to always put a 'From:' in the body of self-authored patches when using
>>> git format-patch; however, as I don't suffer from munged emails, I
>>> haven't actually tested what that setting would be.
> 
> Well, I tried that Eric. The expected solution would be enabling this git 
> setting:
> 
> git config [--global] format.from true
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
> 
> But as you can already read from the manual, the overall behaviour of git 
> regarding a separate "From:" line in the email body was intended solely for 
> the use case sender != author. So in practice (at least in my git version) git 
> always makes a raw string comparison between sender (name and email) string 
> and author string and only adds the separate From: line to the body if they 
> differ.
> 
> Hence also "git format-patch --from=" only works here if you use a different 
> author string (name and email) there, otherwise on a perfect string match it 
> is simply ignored and you end up with only one "From:" in the email header.

git folks:

How hard would it be to improve 'git format-patch'/'git send-email' to
have an option to ALWAYS output a From: line in the body, even when the
sender is the author, for the case of a mailing list that munges the
mail headers due to DMARC/DKIM reasons?

> 
> So eventually I added one extra character in my name for now and removed it 
> manually in the dumped emails subsequently (see today's
> "[PATCH v7 0/3] 9p: Fix file ID collisions").
> 
> Besides that direct string comparison restriction; git also seems to have a 
> bug here. Because even if you have sender != author, then git falsely uses 
> author as sender of the cover letter, whereas the emails of the individual 
> patches are encoded correctly.

At any rate, I'm glad that you have figured out a workaround, even if
painful, while we wait for git to provide what we really need.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Jeff King]

On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:

> > But as you can already read from the manual, the overall behaviour of git 
> > regarding a separate "From:" line in the email body was intended solely for 
> > the use case sender != author. So in practice (at least in my git version) git 
> > always makes a raw string comparison between sender (name and email) string 
> > and author string and only adds the separate From: line to the body if they 
> > differ.
> > 
> > Hence also "git format-patch --from=" only works here if you use a different 
> > author string (name and email) there, otherwise on a perfect string match it 
> > is simply ignored and you end up with only one "From:" in the email header.
> 
> git folks:
> 
> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?

It wouldn't be very hard to ask format-patch to just handle this
unconditionally. Something like:

diff --git a/pretty.c b/pretty.c
index e4ed14effe..9cf79d7874 100644
--- a/pretty.c
+++ b/pretty.c
@@ -451,7 +451,8 @@ 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->always_use_in_body_from ||
+		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
 			struct strbuf buf = STRBUF_INIT;
 
 			strbuf_addstr(&buf, "From: ");

but most of the work would be ferrying that option from the command line
down to the pretty-print code.

That would work in conjunction with "--from" to avoid a duplicate. It
might require send-email learning about the option to avoid doing its
own in-body-from management. If you only care about send-email, it might
be easier to just add the option there.

-Peff

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck]

On Montag, 9. September 2019 16:25:12 CEST Jeff King wrote:
> On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:
> > > But as you can already read from the manual, the overall behaviour of
> > > git
> > > regarding a separate "From:" line in the email body was intended solely
> > > for
> > > the use case sender != author. So in practice (at least in my git
> > > version) git always makes a raw string comparison between sender (name
> > > and email) string and author string and only adds the separate From:
> > > line to the body if they differ.
> > > 
> > > Hence also "git format-patch --from=" only works here if you use a
> > > different author string (name and email) there, otherwise on a perfect
> > > string match it is simply ignored and you end up with only one "From:"
> > > in the email header.> 
> > git folks:
> > 
> > How hard would it be to improve 'git format-patch'/'git send-email' to
> > have an option to ALWAYS output a From: line in the body, even when the
> > sender is the author, for the case of a mailing list that munges the
> > mail headers due to DMARC/DKIM reasons?
> 
> It wouldn't be very hard to ask format-patch to just handle this
> unconditionally. Something like:
> 
> diff --git a/pretty.c b/pretty.c
> index e4ed14effe..9cf79d7874 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -451,7 +451,8 @@ 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->always_use_in_body_from ||
> +		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
>  			struct strbuf buf = STRBUF_INIT;
> 
>  			strbuf_addstr(&buf, "From: ");
> 
> but most of the work would be ferrying that option from the command line
> down to the pretty-print code.
> 
> That would work in conjunction with "--from" to avoid a duplicate. It
> might require send-email learning about the option to avoid doing its
> own in-body-from management. If you only care about send-email, it might
> be easier to just add the option there.

Would it simplify the changes in git if that would be made a
"git config [--global]" setting only? That is, would that probably simplify 
that task to one simple function call there in pretty.c?

On the other hand, considering the already existing --from argument and 
"format.from" config option:
https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom

Wouldn't it make sense to just drop the currently existing sender != author 
string comparison in git and simply always add the "From:" line to the email's 
body if "format.from yes" is used, instead of introducing a suggested 2nd 
(e.g. "always-from") option? I mean sure automatically removing redundant 
information in the generated emails if sender == author sounds nice on first 
thought, but does it address anything useful in practice to justify 
introduction of a 2nd related option?

Best regards,
Christian Schoenebeck

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Jeff King]

On Mon, Sep 23, 2019 at 01:19:18PM +0200, Christian Schoenebeck wrote:

> >  	if (cmit_fmt_is_mail(pp->fmt)) {
> > -		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> > +		if (pp->always_use_in_body_from ||
> > +		    (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
> >  			struct strbuf buf = STRBUF_INIT;
> > 
> >  			strbuf_addstr(&buf, "From: ");
> > 
> > but most of the work would be ferrying that option from the command line
> > down to the pretty-print code.
> > 
> > That would work in conjunction with "--from" to avoid a duplicate. It
> > might require send-email learning about the option to avoid doing its
> > own in-body-from management. If you only care about send-email, it might
> > be easier to just add the option there.
> 
> Would it simplify the changes in git if that would be made a
> "git config [--global]" setting only? That is, would that probably simplify 
> that task to one simple function call there in pretty.c?

I think a config option would make sense, but we generally try to avoid
adding a config option that doesn't have a matching command-line option.

I also think saving implementation work there is orthogonal. You can as
easily make a global "always_use_in_body_from" as you can call a global
config_get_bool("format-patch.always_use_in_body_from"). :)

And anyway, it's not _that_ much work to pass it around. At least as
much would go into writing documentation and tests. One of the reasons I
left the patch above as a sketch is that I'm not 100% convinced this is
a useful feature. Somebody caring enough about it to make a real patch
would send a signal there.

> On the other hand, considering the already existing --from argument and 
> "format.from" config option:
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
> 
> Wouldn't it make sense to just drop the currently existing sender != author 
> string comparison in git and simply always add the "From:" line to the email's 
> body if "format.from yes" is used, instead of introducing a suggested 2nd 
> (e.g. "always-from") option? I mean sure automatically removing redundant 
> information in the generated emails if sender == author sounds nice on first 
> thought, but does it address anything useful in practice to justify 
> introduction of a 2nd related option?

Yes, the resulting mail would be correct, in the sense that it could be
applied just fine by git-am. But I think it would be uglier. IOW, I
consider the presence of the in-body From to be a clue that something
interesting is going on (like forwarding somebody else's patch). So from
my perspective, it would just be useless noise. Other communities may
have different opinions, though (I think I have seen some kernel folks
always including all of the possible in-body headers, including Date).
But it seems like it makes sense to keep both possibilities.

-Peff

git format.from (was: 9p: Fix file ID collisions)

[Christian Schoenebeck]

On Dienstag, 24. September 2019 00:24:15 CEST Jeff King wrote:
> > On the other hand, considering the already existing --from argument and
> > "format.from" config option:
> > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfro
> > m
> > 
> > Wouldn't it make sense to just drop the currently existing sender !=
> > author
> > string comparison in git and simply always add the "From:" line to the
> > email's body if "format.from yes" is used, instead of introducing a
> > suggested 2nd (e.g. "always-from") option? I mean sure automatically
> > removing redundant information in the generated emails if sender ==
> > author sounds nice on first thought, but does it address anything useful
> > in practice to justify introduction of a 2nd related option?
> 
> Yes, the resulting mail would be correct, in the sense that it could be
> applied just fine by git-am. But I think it would be uglier. IOW, I
> consider the presence of the in-body From to be a clue that something
> interesting is going on (like forwarding somebody else's patch). So from
> my perspective, it would just be useless noise. Other communities may
> have different opinions, though (I think I have seen some kernel folks
> always including all of the possible in-body headers, including Date).
> But it seems like it makes sense to keep both possibilities.

Exactly, current git behaviour is solely "prettier" (at first thought only 
though), but does not address anything useful in real life.

Current git behaviour does cause real life problems though: Many email lists 
are munging emails of patch senders whose domain is configured for requiring 
domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a 
DMARC). So original sender's From: header is then automatically replaced by an 
alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting

For instance the email header:

From: "Bob Bold" <bold@foo.com>

is automatically replaced by lists by something like

From: "Bob Bold via Somelist" <somelist@gnu.org>

And since git currently always drops the From: line from the email's body if
sender == author, as a consequence maintainers applying patches from such 
lists, always need to rewrite git history subsequently and have to replace 
patch author's identity manually for each commit to have their correct, real 
email address and real name in git history instead of something like
"Bob Bold via Somelist" <somelist@gnu.org>

So what do you find "uglier"? I prefer key info not being lost as default 
behaviour. :-)

Best regards,
Christian Schoenebeck

Re: git format.from (was: 9p: Fix file ID collisions)

[Jeff King]

On Tue, Sep 24, 2019 at 11:03:38AM +0200, Christian Schoenebeck wrote:

> > Yes, the resulting mail would be correct, in the sense that it could be
> > applied just fine by git-am. But I think it would be uglier. IOW, I
> > consider the presence of the in-body From to be a clue that something
> > interesting is going on (like forwarding somebody else's patch). So from
> > my perspective, it would just be useless noise. Other communities may
> > have different opinions, though (I think I have seen some kernel folks
> > always including all of the possible in-body headers, including Date).
> > But it seems like it makes sense to keep both possibilities.
> 
> Exactly, current git behaviour is solely "prettier" (at first thought only 
> though), but does not address anything useful in real life.

I wouldn't agree with that. By being pretty, it also is functionally
more useful (I can tell at a glance whether somebody is sending a patch
from another author).

> Current git behaviour does cause real life problems though: Many email lists 
> are munging emails of patch senders whose domain is configured for requiring 
> domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a 
> DMARC). So original sender's From: header is then automatically replaced by an 
> alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting
> 
> For instance the email header:
> 
> From: "Bob Bold" <bold@foo.com>
> 
> is automatically replaced by lists by something like
> 
> From: "Bob Bold via Somelist" <somelist@gnu.org>
> 
> And since git currently always drops the From: line from the email's body if
> sender == author, as a consequence maintainers applying patches from such 
> lists, always need to rewrite git history subsequently and have to replace 
> patch author's identity manually for each commit to have their correct, real 
> email address and real name in git history instead of something like
> "Bob Bold via Somelist" <somelist@gnu.org>
> 
> So what do you find "uglier"? I prefer key info not being lost as default 
> behaviour. :-)

Sure, for your list that munges From headers, always including an
in-body From is way better. But for those of us _not_ on such lists, I'd
much prefer not to force the in-body version on them.

-Peff

Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Junio C Hamano]

Eric Blake <eblake@redhat.com> writes:

> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?

I'd say that it shouldn't be so hard to implement than realizing
what ahd why it is needed, designing what the end-user interaction
would be (i.e.  command line options?  configuration variables?
should it be per send-email destination?) and stating all of the
above clearly in the documentation and the proposed commit log
message.

The reason you are asking is...?  Am I smelling a volunteer?