Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers

From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Date: Oct 16, 2018, 07:17 UTC
Message-ID: <ef80210d-f440-06c5-a1eb-7b3737f73adb@rasmusvillemoes.dk>
In-Reply-To: <xmqqo9buzb67.fsf@gitster-ct.c.googlers.com>

On 2018-10-16 07:57, Junio C Hamano wrote:

Show 20 quoted lines

> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
> 
>>> I wonder if it would make it easier to grok if we made the logic
>>> inside out, i.e.
>>>
>>> 	if ($sc eq $sender) {
>>> 		...
>>> 	} else {
>>> 		if ($what =~ /^Signed-off-by$/i) {
>>> 			next if $suppress_cc{'sob'};
>>> 		} elsif ($what =~ /-by$/i) {
>>> 			next if $suppress_cc{'misc'};
>>> 		} elsif ($what =~ /^Cc$/i) {
>>> 			next if $suppress_cc{'bodycc'};> 		}
>>
>> ...yes, that's probably more readable.
> 
> OK, unless there is more comments and suggestions for improvements,
> can you send in a final version sometime not in so distant future so
> that we won't forget?

Will do, I was just waiting for more comments to come in.

 It may be surprising to existing users that

> the command now suddenly adds more addresses the user did not think
> would be added, but it would probably be easy enough for them to
> work around.

Yeah, I thought about that, but unfortunately the whole auto-cc business is not built around some config options where we can add a new and default false. Also note that there are also cases currently where the user sends off a patch series and is surprised that lots of intended recipients were not cc'ed (that's how I picked this subject up again; I had a long series where I had put specific Cc's in each patch, at v2, some of those had given a Reviewed-by, so I changed the tags, and a --dry-run told me they wouldn't get the new version).

I suppose one could make use of -by addresses dependent on a new opt-in config option, but that's not very elegant. Another option is, for a release or two, to make a little (more) noise when picking up a -by address - something like setting a flag in the ($what =~ /-by/) branch, and testing that flag somewhere in send_message(). But I suppose the message printed when needs_confirm eq "inform" is generic enough.

Rasmus