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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 2, 2015, 17:58 UTC
Message-ID: <xmqq8u5c68by.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1449075602.3716.27.camel@perches.com>

Joe Perches <joe@perches.com> writes:

Show 14 quoted lines

> 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 <joe <at> perches.com>
> Acked-by: Jeff Kirsher <jeffrey.t.kirsher <at> intel.com>

I wonder what send-email with this patch does to the above two lines with "<at>" not "@" ;-) How was this patch sent?

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.

Show 7 quoted lines

> ---
>> 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*].

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.