From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 01 Jun 2015 15:19:16 GMT
Subject: Re: [PATCH 2/2] send-email: allow multiple emails using --cc, --to and --bcc
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:

> 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.

> 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?


>  @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.