Re: [PATCH/RFC] send-email: allow multiple emails using --cc --to and --bcc

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 28, 2015, 15:26 UTC
Message-ID: <CAPig+cRb0GstQThkprzBA=VwHSFxUeX6KDdSA7Xjdb_B2mH6Uw@mail.gmail.com>
In-Reply-To: <1432809733-4321-2-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr>

On Thu, May 28, 2015 at 6:42 AM, Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr> wrote:

> Add the possibility to use a list of emails separated by commas
> in flags --cc --to and --bcc instead of having to use one flag

s/--cc --to/--cc, --to/

Ditto in subject.

Show 18 quoted lines

> per email address.
>
> The use-case is to copy-paste a list of addresses from an email.
> This change makes it so that we no longer need to cut the list.
>
> The format of email list handled is basic for now:
>         $ git send-email --to='Foo <foo@example.com>, bar@example.com'
> We thought it would be nice to have a "first-step" version which works
> before handling more complex ones such as names with commas:
>         $ git send-email --to='Foo, Bar <foobar@example.com>'
>
> This artificial limitation is imposed by 79ee555b (Check and document
> the options to prevent mistakes, 2006-06-21).
>
> Signed-off-by: Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr>
> Signed-off-by: Jorge Juan Garcia Garcia <Jorge-Juan.Garcia-Garcia@ensimag.imag.fr>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Contributions-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>

Helped-by: may be more appropriate than Contributions-by: on this
project. Also, move it above the sign-offs.

Show 11 quoted lines

> ---
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 043f345..0aeddcb 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -49,17 +49,21 @@ Composing
>         of 'sendemail.annotate'. See the CONFIGURATION section for
>         'sendemail.multiEdit'.
>
> ---bcc=<address>::
> +--bcc="[<address>,...]"::

Adding square brackets indicates that the addresses following '=' are optional, which is incorrect. It should be sufficient merely to add the comma and ellipsis. Also, the need to quote strings containing whitespace is a shell issue not specifically related to this option. That is, people have to understand quoting issues in general, so it doesn't make sense to mention them literally here. Thus:

    --bcc=<address>,...::

should be sufficient.

>         Specify a "Bcc:" value for each email. Default is the value of
>         'sendemail.bcc'.
> -+
> -The --bcc option must be repeated for each user you want on the bcc list.

I think it's harmful to remove this line. Although the "must" is no longer true following this change, it's still important for people to know that they can use --bcc multiple times. So, perhaps just reword it:

    This option may be specified multiple times.

> +       The format supported for email list is the following:
> +       "Foo <foo@example.com>, bar@example.com".
> +       Please notice that the email list does not handle commas in
> +       email names such as "Foo, Bar <foobar@example.com>".

A few issues:

In order for this to format correctly in Asciidoc, the text needs to be left-justified (just as was the line which you removed).

The sentence "The format supported..." seems superfluous. It's merely repeating what is already clearly indicated by "--bcc=<address>,...", thus it could easily be dropped without harm.

Mention that commas in names are not currently handled is indeed a good idea, however, "please" tends to be an overused and wasted word in documentation. More tersely:

    Note: Addresses containing commas ("Foo, Bar" <...>)
    are not currently supported.

The above comments also apply to --cc and --to.

Show 12 quoted lines

> diff --git a/git-send-email.perl b/git-send-email.perl
> index ffea500..409ff45 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1052,7 +1038,8 @@ sub sanitize_address {
>  }
>
>  sub sanitize_address_list {
> -       return (map { sanitize_address($_) } @_);
> +       my @addr_list = split_address_list(@_);
> +       return (map { sanitize_address($_) } @addr_list);
>  }

Although, it was convenient from an implementation perspective to plop the split_address_list() invocation into sanitize_address_list(), it pollutes sanitize_address_list() by making it do something unrelated to its purpose.

If you examine places where sanitize_address_list() is called, you will see:

    validate_address_list(sanitize_address_list(@xx))

which clearly shows that sanitation and validation are distinct operations (and each function does only what its name implies). To continue this idea, the splitting of addresses should be performed distinctly from the other operations, in this order:

   split -> sanitize -> validate

or:

    validate_address_list(sanitize_address_list(
        split_address_list(@xx))

That's pretty verbose, so introducing a new function to encapsulates that behavior might be reasonable.

Show 8 quoted lines

>  # Returns the local Fully Qualified Domain Name (FQDN) if available.
> @@ -1193,6 +1180,10 @@ sub file_name_is_absolute {
>         return File::Spec::Functions::file_name_is_absolute($path);
>  }
>
> +sub split_address_list {
> +       return (map { split /\s*,\s*/, $_ } @_);
> +}

This is somewhat misnamed. It's not splitting the address list in the sense that sanitize_address_list() and validate_address_list() operate on the list of addresses. The name implies that it's somehow partitioning the list of addresses, but in fact it's iterating over the incoming list and (possibly) splitting each item into multiple pieces. Perhaps a better name would be split_address_list_items() or something.

>  # Returns 1 if the message was sent, and 0 otherwise.
>  # In actuality, the whole program dies when there
>  # is an error sending a message.