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

[Junio C Hamano <gitster@pobox.com>]

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

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