Re: [PATCH] git-send-email: Add --suppress-all-from option.

David Brown <git@davidb.org> writes:

I would agree with the cause, but not necessarily with the execution.

The option name feels as if it is somehow affecting From: but this is all about recipients. It needs to be named better.

Even more importantly, git-send-email has too many places that pick up additional recipients. I doubt --suppress-foo to suppress one such source "foo" is sustainable. We should try to clean up the mess, not adding to it.

So let's analyze the current situation first. It seems that we currently pick up the list of recipients from the following places:

 * obviously, --to command line;
 * mbox Cc: header lines;
 * mbox From: header lines;
 * lots-of-email first line
 * $cc_cmd output;
 * Signed-off-by: lines in body;
 * Cc: lines in body;

The --no-signed-off-cc option is about omitting the last two from the recipients. We do not have a way to squelch other sources of extra recipients, hence the need for your patch.

The --suppress-from option is about not giving an extra copy to the sender. It is "suppress from-address from the recipient list", so the option name makes sense.

Your --suppress-all-from, from a cursory read of your patch, omits only mbox Cc: and From: line recipients -- it is far from "all", isn't it? --signed-off-cc defaults to true so you would need to suppress that at least to call it "all".

A cleaner approach might be:

 - introduce a helper function add_to_recipients that take \@cc,
   $recipient and the "source class".  Make this function
   responsible for not adding the sender to the list
   (i.e. --suppress-from, which is currently checked
   everywhere), and for not adding recipients from specified
   classes of sources, like this:

        sub add_to_recipients {
                my ($cc, $source, $recipient) = @_;
                return 0 if ($suppress_from and $sender eq $recipient);
                return 0 if ($suppressed_recipient_source{$source});
                push @$cc, $recipient;
                return 1;
        }

   Instead of returning 1 unconditionally, it might make sense
   to omit pushing duplicate here and to return 0 when
   $recipient was already in @$cc.

 - adjust the places where "push @cc" happens to use the above
   helper; the existing suppress logic in the callers can and
   should be removed as the add_to_recipients will be
   responsible for it, like this (an example for cc-cmd part):

        if (defined $cc_cmd) {
        open(F, "$cc_cmd $t |")
                or die "(cc-cmd) Could not execute '$cc_cmd'";
        while(<F>) {
                my $c = $_;
                $c =~ s/^\s*//g;
                $c =~ s/\n$//g;
-               next if ($c eq $sender and $suppress_from);
-               push @cc, $c;
+               next if (!add_to_recipients(\@cc, 'cccmd', $c));
                printf("(cc-cmd) Adding cc: %s from: '%s'\n",
                        $c, $cc_cmd) unless $quiet;
        }

 - define a global %suppressed_recipient_source hash to be used
   in add_to_recipients().  The existing --no-signed-off-cc is
   about adding two sources to this hash.

 - make the %suppressed_recipient_source configurable from the
   command line and repository configuration.

As to the "recipient source" classes, I think they can be categorized as:

 * 'cc', to cover mbox Cc: header, lots-of-email first line, and
    Cc: lines in body;

 * 'sob', to cover Signed-off-by: lines in body;

 * 'cccmd', to cover $cc_cmd output;

Hmm?