Re: [PATCH] git-send-email: Cc more people

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 18, 2018, 19:58 UTC
Message-ID: <87tvs8e174.fsf@evledraar.gmail.com>
In-Reply-To: <20180418140503.GD27475@bombadil.infradead.org>

On Wed, Apr 18 2018, Matthew Wilcox wrote:

Show 23 quoted lines

> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> Several of my colleagues (and myself) have expressed surprise and
> annoyance that git-send-email doesn't automatically pick up people who
> are listed in patches as Reported-by: or Reviewed-by: or ... many other
> tags that would seem (to us) to indicate that person might be interested.
> This patch to git-send-email tries to pick up all Foo-by: tags.
>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2fa7818ca..926815329 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {
>  	# Now parse the message body
>  	while(<$fh>) {
>  		$message .=  $_;
> -		if (/^(Signed-off-by|Cc): (.*)/i) {
> +		if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
>  			chomp;
>  			my ($what, $c) = ($1, $2);
>  			# strip garbage for the address we'll use:

I like this direction, I've actually been meaning to take this further and try to parse out SHA1s in the commit message, look those up, and add their authors to CC one of these days.

But IMO this patch is really lacking a few things before being ready:

1. You have no tests for this. See t/t9001-send-email.sh for examples,
   i.e. stuff like

       (body) Adding cc: C O Mitter <committer@example.com> from line
       'Signed-off-by: C O Mitter <committer@example.com>'

   Should have corresponding tests for "Reviewed-by" "Seen-by"
   etc. These are easy to add, just edit the raw messages and test that
   for the output about adding CCs.

2. Just a few lines down from your quoted hunk we have this:

	# strip garbage for the address we'll use:
	$c = strip_garbage_one_address($c);
	# sanitize a bit more to decide whether to suppress the address:
	my $sc = sanitize_address($c);
	if ($sc eq $sender) {
		next if ($suppress_cc{'self'});
	} else {
		next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
		next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
	}
	push @cc, $c;
	printf(__("(body) Adding cc: %s from line '%s'\n"),
		$c, $_) unless $quiet;

   So before we just supported Signed-off-by as a special case, but now
   your patch adds WHAT-EVER-by without updating the the corresponding
   --[no-]signed-off-by-cc command-line options.

   Your change should at least describe why those aren't being updated,
   but probably we should add some other command-line option for
   ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed
   --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by
   a historical alias for --[no-]wildcard-by-cc=signed-off.

3. Ditto all the documentation in "man git-send-email" about
   "signed-off-by", "sob" etc, and the "signedoffbycc" variable
   documented both there and in "man git-config".

Style comment: First time I've seen someone write a charclass as [A-Z-a-z] and mean it, usually it's usually it's [A-Za-z-] to clarify that the "-" isn't a range. Makes sense (to me) to have ranges first & stray chars last.