All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Christian Schoenebeck <qemu_oss@crudebyte.com>
Cc: qemu-devel@nongnu.org, Eric Blake <eblake@redhat.com>,
	berrange@redhat.com, stefanha@gmail.com,
	Greg Kurz <groug@kaod.org>,
	dgilbert@redhat.com, antonios.motakis@huawei.com,
	git@vger.kernel.org
Subject: Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
Date: Mon, 23 Sep 2019 18:24:15 -0400	[thread overview]
Message-ID: <20190923222415.GA22495@sigill.intra.peff.net> (raw)
In-Reply-To: <56046367.TiUlWITyhT@silver>

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

WARNING: multiple messages have this Message-ID (diff)
From: Jeff King <peff@peff.net>
To: Christian Schoenebeck <qemu_oss@crudebyte.com>
Cc: berrange@redhat.com, stefanha@gmail.com,
	Greg Kurz <groug@kaod.org>,
	qemu-devel@nongnu.org, git@vger.kernel.org,
	antonios.motakis@huawei.com, dgilbert@redhat.com
Subject: Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
Date: Mon, 23 Sep 2019 18:24:15 -0400	[thread overview]
Message-ID: <20190923222415.GA22495@sigill.intra.peff.net> (raw)
In-Reply-To: <56046367.TiUlWITyhT@silver>

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


  reply	other threads:[~2019-09-23 22:26 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-22 19:53 [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions Christian Schoenebeck via Qemu-devel
2019-08-22 19:28 ` [Qemu-devel] [PATCH v6 1/4] 9p: Treat multiple devices on one export as an error Christian Schoenebeck via Qemu-devel
2019-08-29 16:27   ` Greg Kurz
2019-09-01 17:38     ` Christian Schoenebeck via Qemu-devel
2019-08-22 19:33 ` [Qemu-devel] [PATCH v6 2/4] 9p: Added virtfs option 'multidevs=remap|forbid|warn' Christian Schoenebeck via Qemu-devel
2019-08-29 16:55   ` Greg Kurz
2019-09-01 18:40     ` Christian Schoenebeck via Qemu-devel
2019-09-02 10:16       ` Greg Kurz
2019-09-02 21:07         ` Christian Schoenebeck via Qemu-devel
2019-08-30 12:22   ` Greg Kurz
2019-09-01 18:56     ` Christian Schoenebeck via Qemu-devel
2019-09-02 11:49       ` Greg Kurz
2019-09-02 21:25         ` Christian Schoenebeck via Qemu-devel
2019-08-22 19:44 ` [Qemu-devel] [PATCH v6 3/4] 9p: stat_to_qid: implement slow path Christian Schoenebeck via Qemu-devel
2019-08-22 19:49 ` [Qemu-devel] [PATCH v6 4/4] 9p: Use variable length suffixes for inode remapping Christian Schoenebeck via Qemu-devel
2019-08-22 22:18 ` [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions no-reply
2019-08-29 17:02   ` Greg Kurz
2019-09-01 19:28     ` Christian Schoenebeck via Qemu-devel
2019-09-02 15:34       ` Greg Kurz
2019-09-02 22:29         ` Christian Schoenebeck via Qemu-devel
2019-09-03 19:11           ` [Qemu-devel] DMARC/DKIM and qemu-devel list settings Ian Kelling
2019-09-04  8:13             ` Daniel P. Berrangé
2019-09-04 14:19               ` Ian Kelling
2019-09-04 14:30             ` Peter Maydell
2019-09-09 11:47               ` Markus Armbruster
2019-09-10  7:23               ` Stefan Hajnoczi
2019-09-03 19:38           ` [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions Eric Blake
2019-09-04 13:02             ` Christian Schoenebeck via Qemu-devel
2019-09-05 12:25               ` Christian Schoenebeck via Qemu-devel
2019-09-05 12:59                 ` Greg Kurz
2019-09-23 11:27                   ` Christian Schoenebeck via
2019-09-09 14:05                 ` Eric Blake
2019-09-09 14:05                   ` Eric Blake
2019-09-09 14:25                   ` Jeff King
2019-09-09 14:25                     ` Jeff King
2019-09-23 11:19                     ` Christian Schoenebeck
2019-09-23 11:19                       ` Christian Schoenebeck via
2019-09-23 22:24                       ` Jeff King [this message]
2019-09-23 22:24                         ` Jeff King
2019-09-24  9:03                         ` git format.from (was: 9p: Fix file ID collisions) Christian Schoenebeck
2019-09-24  9:03                           ` Christian Schoenebeck via
2019-09-24 21:36                           ` Jeff King
2019-09-24 21:36                             ` Jeff King
2019-09-09 18:41                   ` [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions Junio C Hamano
2019-09-09 18:41                     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190923222415.GA22495@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=antonios.motakis@huawei.com \
    --cc=berrange@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=eblake@redhat.com \
    --cc=git@vger.kernel.org \
    --cc=groug@kaod.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu_oss@crudebyte.com \
    --cc=stefanha@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.