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

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

[Remi Lespinet]

Hello,

I've corrected an old patch from an Ensimag student.
(http://thread.gmane.org/gmane.comp.version-control.git/228182). This
patch allows multiple email addresses for options --cc, --to and
--bcc. As said in the commit message, this patch doesn't handle commas
in name, and the only possibility for using commas in name is to use the
rfc2047 syntax:

To: =?ISO-8859-1?Q?Ex=2C_ample?= <to@a.com>

I would like to add the possibility to use the following command lines:

    git send-email --to '"Ex, am ple" <to@a.com>'

    git send-email --to '"Ex, am" "ple" <to@a.com>'

    git send-email --to "\"Ex, am ple\" <to@a.com>"

    git send-email --to "\"Ex, am\" \"ple\" <to@a.com>"


Here are my questions :
Is this a good idea to handle commas in name ?
Do you have any suggestion about proposed syntaxes ?

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

[Remi Lespinet]

From: Jorge Juan Garcia Garcia <Jorge-Juan.Garcia-Garcia@ensimag.imag.fr>

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
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>
---
 Documentation/git-send-email.txt | 23 ++++++++++++++--------
 git-send-email.perl              | 21 ++++++--------------
 t/t9001-send-email.sh            | 41 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 58 insertions(+), 27 deletions(-)

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>,...]"::
 	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.
+	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>".
 
---cc=<address>::
+--cc="[<address>,...]"::
 	Specify a starting "Cc:" value for each email.
 	Default is the value of 'sendemail.cc'.
-+
-The --cc option must be repeated for each user you want on the cc list.
+	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>".
 
 --compose::
 	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
@@ -111,12 +115,15 @@ is not set, this will be prompted for.
 	is not set, this will be prompted for.
 
 --to=<address>::
+--to="[<address>,...]"::
 	Specify the primary recipient of the emails generated. Generally, this
 	will be the upstream maintainer of the project involved. Default is the
 	value of the 'sendemail.to' configuration value; if that is unspecified,
 	and --to-cmd is not specified, this will be prompted for.
-+
-The --to option must be repeated for each user you want on the to list.
+	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>".
 
 --8bit-encoding=<encoding>::
 	When encountering a non-ASCII message or subject that does not
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
@@ -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/,/;
-}
-
 sub parse_address_line {
 	if ($have_mail_address) {
 		return map { $_->format } Mail::Address->parse($_[0]);
@@ -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);
 }
 
 # 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*/, $_ } @_);
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index a3663da..4245c06 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -519,6 +519,12 @@ Result: OK
 EOF
 "
 
+replace_variable_fields () {
+	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
+		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
+		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/"
+}
+
 test_suppression () {
 	git send-email \
 		--dry-run \
@@ -526,10 +532,7 @@ test_suppression () {
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--smtp-server relay.example.com \
-		$patches |
-	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
-		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
-		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
+		$patches | replace_variable_fields \
 		>actual-suppress-$1${2+"-$2"} &&
 	test_cmp expected-suppress-$1${2+"-$2"} actual-suppress-$1${2+"-$2"}
 }
@@ -1609,4 +1612,34 @@ test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
 	do_xmailer_test 1 "--xmailer"
 '
 
+test_expect_success $PREREQ 'setup expected-list' '
+	git send-email \
+	--dry-run \
+	--from="Example <from@example.com>" \
+	--to="to1@example.com" \
+	--to="to2@example.com" \
+	--to="to3@example.com" \
+	--cc="cc1@example.com" \
+	--cc="Cc 1 <cc1@example.com>" \
+	--cc="Cc 2 <cc2@example.com>" \
+	--bcc="bcc1@example.com" \
+	--bcc="bcc2@example.com" \
+	0001-add-master.patch |	replace_variable_fields \
+	>expected-list
+'
+
+test_expect_success $PREREQ 'use email list in --cc --to and --bcc' '
+	git send-email \
+	--dry-run \
+	--from="Example <from@example.com>" \
+	--to="to1@example.com, to2@example.com" \
+	--to="to3@example.com" \
+	--cc="cc1@example.com, Cc 1 <cc1@example.com>" \
+	--cc="Cc 2 <cc2@example.com>" \
+	--bcc="bcc1@example.com, bcc2@example.com" \
+	0001-add-master.patch |	replace_variable_fields \
+	 >actual-list &&
+	test_cmp expected-list actual-list
+'
+
 test_done
-- 
1.9.1

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

[Matthieu Moy]

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

> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -519,6 +519,12 @@ Result: OK
>  EOF
>  "
>  
> +replace_variable_fields () {
> +	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
> +		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
> +		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/"
> +}
> +
>  test_suppression () {
>  	git send-email \
>  		--dry-run \
> @@ -526,10 +532,7 @@ test_suppression () {
>  		--from="Example <from@example.com>" \
>  		--to=to@example.com \
>  		--smtp-server relay.example.com \
> -		$patches |
> -	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
> -		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
> -		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
> +		$patches | replace_variable_fields \

I wouldn't insist on that, but this change would be better done in a
separate, preparatory patch (that would be PATCH 1/2, and the actual
code would be PATCH 2/2).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Eric Sunshine]

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.

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

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

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

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

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

[Eric Sunshine]

On Thu, May 28, 2015 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, May 28, 2015 at 6:42 AM, Remi Lespinet
> <remi.lespinet@ensimag.grenoble-inp.fr> wrote:
>> +       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).

s/left-justified/unindented/

> 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

s/Mention/Mentioning/

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

Oops: s/sanitation/sanitization/

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

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

[Remi LESPINET]

Eric Sunshine <sunshine@sunshineco.com> writes:

> <remi.lespinet@ensimag.grenoble-inp.fr> wrote:
> 
>     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.

Agreed, If you have any suggestion for the name of this function, I'll
be happy to use it, I named it extract_address_list, but I'm not sure
that it's a good name.

Thanks again for the review!

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

[Junio C Hamano]

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

> From: Jorge Juan Garcia Garcia <Jorge-Juan.Garcia-Garcia@ensimag.imag.fr>
>
> 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
> 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.

s/Add the possibility to use/Accept/;

I am not sure "having to use" is a good characterization.

    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.

perhaps?

> 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

Separate displayed material from the body text with blank lines, i.e.

        The format of email list handled is basic for now:

                $ git send-email --to='Foo ...

        We thought it would be nice to have a "first-step" version which works

"We thought"?

> before handling more complex ones such as names with commas:
> 	$ git send-email --to='Foo, Bar <foobar@example.com>'

Shouldn't this example send to two users, i.e. a local user Foo and
the mailbox 'foobar' at example.com whose human-readable name is
Bar?  If so, that is a bad example to illustrate the aspiration for
the finished patch?

> This artificial limitation is imposed by 79ee555b (Check and document
> the options to prevent mistakes, 2006-06-21).

It is unclear from this sentence what you are doing with the
limitation (is it artificial?  do we have to call it artificial?).
The reader of the log message cannot tell if you are keeping it, or
if you are lifting it with this patch.

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

You touched the patch or sent it out, so you need to give your own
sign-off.

Thanks.

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

[Remi LESPINET]

Junio C Hamano <gitster@pobox.com> writes:

> 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.
>
> perhaps?
I've taken this description for the first part of the commit
message. Thanks!

> > before handling more complex ones such as names with commas:
> >         $ git send-email --to='Foo, Bar <foobar@example.com>'
> 
> Shouldn't this example send to two users, i.e. a local user Foo and
> the mailbox 'foobar' at example.com whose human-readable name is
> Bar?  If so, that is a bad example to illustrate the aspiration for
> the finished patch?

Yes, that works if Foo is in an alias file, so that's clearly a bad
example, I added quotes:

	git send-email --to='"Foo, Bar" <foobar@example.com>'

Thanks!

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

[Junio C Hamano]

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

> Yes, that works if Foo is in an alias file, so that's clearly a bad
> example, I added quotes:
>
> 	git send-email --to='"Foo, Bar" <foobar@example.com>'

I'd further suggest replacing ", Bar" with something a bit more
realistic that people use in real life, e.g. ", Esq." or ", PhD"
(e.g. "Jane Doe, Esq.")

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:
>
>> Yes, that works if Foo is in an alias file, so that's clearly a bad
>> example, I added quotes:
>>
>> 	git send-email --to='"Foo, Bar" <foobar@example.com>'
>
> I'd further suggest replacing ", Bar" with something a bit more
> realistic that people use in real life, e.g. ", Esq." or ", PhD"
> (e.g. "Jane Doe, Esq.")

Why not, but it's also not uncommon to see "Last, First", like
"Moy, Matthieu" <Matthieu.Moy@imag.fr>.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:
>>
>>> Yes, that works if Foo is in an alias file, so that's clearly a bad
>>> example, I added quotes:
>>>
>>> 	git send-email --to='"Foo, Bar" <foobar@example.com>'
>>
>> I'd further suggest replacing ", Bar" with something a bit more
>> realistic that people use in real life, e.g. ", Esq." or ", PhD"
>> (e.g. "Jane Doe, Esq.")
>
> Why not, but it's also not uncommon to see "Last, First", like
> "Moy, Matthieu" <Matthieu.Moy@imag.fr>.

", Bar" is not clear what its relationship is to "Foo", but with ",
Matthieu" it is clear what is going on, so that is fine, too.