Re: [PATCH V3] git-send-email: Add auto-cc to all body signatures

[Joe Perches <joe@perches.com>]

On Wed, 2015-12-02 at 09:58 -0800, Junio C Hamano wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > Many types of signatures are used by various projects.
> > 
> > The most common type is formatted:
> > 	"[some_signature_type]-by: First Last <email <at> domain.tld>"
> > e.g:
> > 	"Reported-by: First Last <email <at> domain.tld>" (no quotes are used)
> > 
> > Make git-send-email use these signatures as "CC:" entries.
> > 
> > Add command line option --suppress-cc=signatures to avoid
> > adding these entries to the cc.
> > 
> > Signed-off-by: Joe Perches  perches.com>
> > Acked-by: Jeff Kirsher  intel.com>
> 
> I wonder what send-email with this patch does to the above two lines
> with "" not "@" ;-)  How was this patch sent?

gnome evolution v3.18.2 email client.

And it seems all newer versions of evolution beyond 3.12
are really, really poor at sending inline patches. <grumble>

I'll update and resend using git-send-email eventually

> In any case, did you mean "Helped-by:" not "Acked-by:"?  "git
> shortlog git-send-email.perl" does not show that name as one of the
> major stakeholders who would be capable of giving an Ack on it.

At least for linux-kernel, "Acked-by:" doesn't mean a maintainer
or a contributor to a particular module/file, just someone that
has looked at the patch, tried it, and approved of the concept.

I don't know what process git uses for approval/signatures.

> > ---
> > > It's been four years, but I recently ran into this. I mistakenly thought
> > > that git would actually pick up cc addresses also from Reported-by, so
> > > the reporter ended up not being cc'ed. Is there any chance this could be
> > > revisited,
> > 
> > Here's a refresh if desired.  I still think it's sensible.
> 
> What the patch tries to achieve may make a lot of sense.  I however
> do not necessarily think this particular implementation does,
> unfortunately.
> 
> These "Random-by:", especially the ones that the author adds on his
> own initiative like "Reported-by:", are often followed by just a
> name but not an addresses.  A "Signed-off-by:" and "Cc:" that is not
> followed by a valid e-mail address may deserve to get an error (or
> perhaps an end-user interaction "This is not a valid address. What
> do you want to do about it?") so "/^(Signed-off-by|Cc): (.*)$/i"
> does not need its own sanity check on $2, because a later call to
> extract-valid-address or extract-valid-address-or-die will take care
> of it.

> It would however be wrong to cause the program to error out or even
> bother the user upon seeing such random trailer lines that the
> author did not mean to have an e-mail address on it in the first
> place.  If you have a trailer line
> 
>     Random-by: Joe Perches
> 
> without an address, I suspect you will end up adding "Joe" and
> "Perches" as two addresses on the Cc: line, which is most likely not
> what the user intended [*1*].

At least with new versions of git-send-email.perl
that's true so the patch will need to validate that
there is an email address following.

> As to the lingo, these are still not signatures, but during the past
> years, it seems that we settled on using the term "trailers" for
> these e-mail header-like things at the end of the log message.
> "Trailers" are not limited to "*-by:" so this patch is not about
> adding auto-cc to all trailers--a retitle would be
> 
>     send-email: add auto-cc to addresses that appear on *-by: trailers
> 
> or something (and the option and variable names may need to be
> updated to match).
> 
> 
> [Footnote]
> 
> *1* I further suspect that the existing code shares a similar issue.
> Don't Cc: and Signed-off-by: expect a single address on each line in
> the usual fashion?  Perhaps a two-patch series whose first part does
> 
> -		if (/^(Signed-off-by|Cc): (.*)$/i) {
> +		if (/^(Signed-off-by|Cc): (.*<[^>]*>)\s*$/i) {
> 
> to tighten it (so that "Cc: Joe Perches" would not result in two
> pieces of mail sent to Joe and Perches), with your patch as a follow
> up, may be a good way forward.
> 
> I dunno.

I believe the old git-send-email code required addresses
and validated the form after Signed-off-by:'s.

I haven't looked at the code for several years and just
refreshed it without much thinking or testing.

I'll do a bit more and resend.