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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2015, 15:19 UTC
Message-ID: <xmqqiob78nij.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433168042-28269-2-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr>

Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr> writes:

Show 14 quoted lines

> Accept a list of emails separated by commas in flags --cc, --to and
> --bcc.  Multiple addresses can already be given by using these options
> multiple times, but it is more convenient to allow cutting-and-pasting
> a list of addresses from the header of an existing e-mail message,
> which already lists them as comma-separated list, as a value to a
> single parameter.
>
> The following format can now be used:
>
>     $ git send-email --to='Jane <jdoe@example.com>, mike@example.com'
>
> However format using commas in names doesn't work:
>
>     $ git send-email --to='"Jane, Doe" <jdoe@example.com>'

That looks as if you are doing "Remi, Lespinet", which is not a good example. I think you want "Doe, Jane", the use of comma is when a name is spelled in the "LastName, FirstName" order.

Show 21 quoted lines

> diff --git a/git-send-email.perl b/git-send-email.perl
> index ffea500..389f19c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -460,20 +460,6 @@ my ($repoauthor, $repocommitter);
>  ($repoauthor) = Git::ident_person(@repo, 'author');
>  ($repocommitter) = Git::ident_person(@repo, 'committer');
>  
> -# Verify the user input
> -
> -foreach my $entry (@initial_to) {
> -	die "Comma in --to entry: $entry'\n" unless $entry !~ m/,/;
> -}
> -
> -foreach my $entry (@initial_cc) {
> -	die "Comma in --cc entry: $entry'\n" unless $entry !~ m/,/;
> -}
> -
> -foreach my $entry (@bcclist) {
> -	die "Comma in --bcclist entry: $entry'\n" unless $entry !~ m/,/;
> -}

So at this point, each element in @initial_to and friends is one item we got from the user, e.g. "--to=<arg>", where <arg> may want to include multiple addresses, e.g. "me, you".

And then later, in the more interesting part of the patch, I find a puzzling code.

>  @initial_to = expand_aliases(@initial_to);
> -@initial_to = validate_address_list(sanitize_address_list(@initial_to));
> +@initial_to = extract_address_list(@initial_to);

A question: what is in @initial_to at this point in the code, immediately before you call expand_aliases() on it? Isn't one of the elements be "me, you" when I said --to="me, you"?

Shouldn't you be splitting that into "me" and "you" and expand these two as potential aliases separately?

In other words, I wonder why the patch needs to be any more complex than this instead:

	+@initial_to = split_at_comma(@initial_to);
	 @initial_to = expand_aliases(@initial_to);
	 @initial_to = validate_address_list(sanitize_address_list(@initial_to));

As your goal is to treat --to="<address 1>, <address 2>" as if the user said --to="<address 1>" --to="<address 2>", that would be a more straight-forward way to enhance the system, and you shouldn't have to change anything else, no?

Show 6 quoted lines

>  @initial_cc = expand_aliases(@initial_cc);
> -@initial_cc = validate_address_list(sanitize_address_list(@initial_cc));
> +@initial_cc = extract_address_list(@initial_cc);
>  @bcclist = expand_aliases(@bcclist);
> -@bcclist = validate_address_list(sanitize_address_list(@bcclist));
> +@bcclist = extract_address_list(@bcclist);

Ditto.