[PATCH 1/2] t9001-send-email: create a function replacing variable fields

[PATCH 1/2] t9001-send-email: create a function replacing variable fields

[Remi Lespinet]

Create a function which replaces Date, Message-Id and
X-Mailer lines generated by git-send-email by a specific string

Date:.*$       -> Date: DATE-STRING
Message-Id:.*$ -> Message-Id: MESSAGE-ID-STRING
X-Mailer:.*$   -> X-Mailer: X-MAILER-STRING

This is a preparatory for the next commit which uses this
function

Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
---
 t/t9001-send-email.sh | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index a3663da..71968ee 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"}
 }
-- 
1.9.1

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

[Remi Lespinet]

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

Remove the limitation imposed by 79ee555b (Check and document the
options to prevent mistakes, 2006-06-21) which rejected every argument
with comma in --cc, --to and --bcc

Helped-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
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>
Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
---
 Documentation/git-send-email.txt | 21 +++++++++++++++------
 git-send-email.perl              | 33 ++++++++++++++-------------------
 t/t9001-send-email.sh            | 30 ++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 043f345..f862fa6 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -49,17 +49,23 @@ 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.
+Addresses containing commas ("Foo, Bar" <foobar@example.com>) are not
+currently supported.
++
+This option may be specified multiple times
 
---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.
+Addresses containing commas ("Foo, Bar" <foobar@example.com>) are not
+currently supported.
++
+This option may be specified multiple times
 
 --compose::
 	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
@@ -110,13 +116,16 @@ is not set, this will be prompted for.
 	Only necessary if --compose is also set.  If --compose
 	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.
+Addresses containing commas ("Foo, Bar" <foobar@example.com>) are not
+currently supported.
++
+This option may be specified multiple times
 
 --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..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/,/;
-}
-
 sub parse_address_line {
 	if ($have_mail_address) {
 		return map { $_->format } Mail::Address->parse($_[0]);
@@ -838,11 +824,11 @@ sub expand_one_alias {
 }
 
 @initial_to = expand_aliases(@initial_to);
-@initial_to = validate_address_list(sanitize_address_list(@initial_to));
+@initial_to = extract_address_list(@initial_to);
 @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);
 
 if ($thread && !defined $initial_reply_to && $prompting) {
 	$initial_reply_to = ask(
@@ -1055,6 +1041,15 @@ sub sanitize_address_list {
 	return (map { sanitize_address($_) } @_);
 }
 
+sub split_address_list_items {
+	return (map { split /\s*,\s*/, $_ } @_);
+}
+
+sub extract_address_list {
+	return validate_address_list(sanitize_address_list(
+		split_address_list_items(@_)));
+}
+
 # Returns the local Fully Qualified Domain Name (FQDN) if available.
 #
 # Tightly configured MTAa require that a caller sends a real DNS
@@ -1564,8 +1559,8 @@ foreach my $t (@files) {
 		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
 	$needs_confirm = "inform" if ($needs_confirm && $confirm_unconfigured && @cc);
 
-	@to = validate_address_list(sanitize_address_list(@to));
-	@cc = validate_address_list(sanitize_address_list(@cc));
+	@to = extract_address_list(@to);
+	@cc = extract_address_list(@cc);
 
 	@to = (@initial_to, @to);
 	@cc = (@initial_cc, @cc);
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 71968ee..4245c06 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1612,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 2/2] send-email: allow multiple emails using --cc, --to and --bcc

[Matthieu Moy]

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

> Remove the limitation imposed by 79ee555b (Check and document the
> options to prevent mistakes, 2006-06-21) which rejected every argument
> with comma in --cc, --to and --bcc

Missing "." at the end of sentence.

> -The --bcc option must be repeated for each user you want on the bcc list.
> +Addresses containing commas ("Foo, Bar" <foobar@example.com>) are not
> +currently supported.
> ++
> +This option may be specified multiple times

Likewise (more instances in the patch).

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

Tab after |. Not really serious, but why not a space?

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

You may drop the space between TAB and >.

I would group the tests: the 'setup' is only used to generate
expected-list which is used only in the second test, so one is not
useful without the other (we commonly have a 'setup ...' test which sets
up something used by several other tests).

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

Having thought about this topic (not how the example should be
spelled in the log message ;-) a bit more, I do not think the
implementation of split_address_list_items in this patch is
acceptable.

The reason why we have the "verify the input" thing, allow users to
supply multiple --to/--cc/etc., and do not try to split the
addresses ourselves is because we want to avoid mistakenly splitting
a single address like the above into two and producing syntactically
incorrect addresses.  People have relied on the current behaviour
for a long time, without manually dropping comma when they send
their patches with --to='"Jane, Doe" <jdoe@example.com>'.

Until we can reliably split the address list, accepting this patch
will introduce a regression.

Note that I do agree with the goal of this series and appreciate the
effort.  I am only rejecting the current implementation of
split_address_list_items().

Thanks.

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

[Rémi Lespinet]

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

> The reason why we have the "verify the input" thing, allow users to
> supply multiple --to/--cc/etc., and do not try to split the
> addresses ourselves is because we want to avoid mistakenly splitting
> a single address like the above into two and producing syntactically
> incorrect addresses.  People have relied on the current behaviour
> for a long time, without manually dropping comma when they send
> their patches with --to='"Jane, Doe" <jdoe@example.com>'.

Yes, but they couldn't send with --to='"Jane, Doe" <jdoe@example.com>'
anyway since 79ee555b (Check and document the options to prevent
mistakes. 2006-06-21).  So I don't think that this part is a
regression. However when the user input is incorrect and contains
comma, the mail will be sent to the syntaxically valid addresses which
have been extracted and this would have failed without sending before
this patch. I agree that this part is not a desirable behavior.
If I fix that, would it be ok for you?

2015-06-01 18:52 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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.
>>>
>>> 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.
>
> Having thought about this topic (not how the example should be
> spelled in the log message ;-) a bit more, I do not think the
> implementation of split_address_list_items in this patch is
> acceptable.
>
> The reason why we have the "verify the input" thing, allow users to
> supply multiple --to/--cc/etc., and do not try to split the
> addresses ourselves is because we want to avoid mistakenly splitting
> a single address like the above into two and producing syntactically
> incorrect addresses.  People have relied on the current behaviour
> for a long time, without manually dropping comma when they send
> their patches with --to='"Jane, Doe" <jdoe@example.com>'.
>
> Until we can reliably split the address list, accepting this patch
> will introduce a regression.
>
> Note that I do agree with the goal of this series and appreciate the
> effort.  I am only rejecting the current implementation of
> split_address_list_items().
>
> Thanks.
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Rémi Lespinet <remi.lespinet@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> The reason why we have the "verify the input" thing, allow users to
>> supply multiple --to/--cc/etc., and do not try to split the
>> addresses ourselves is because we want to avoid mistakenly splitting
>> a single address like the above into two and producing syntactically
>> incorrect addresses.  People have relied on the current behaviour
>> for a long time, without manually dropping comma when they send
>> their patches with --to='"Jane, Doe" <jdoe@example.com>'.
>
> Yes, but they couldn't send with --to='"Jane, Doe" <jdoe@example.com>'
> anyway since 79ee555b (Check and document the options to prevent
> mistakes. 2006-06-21).  So I don't think that this part is a
> regression.

Ahh, stupid me.  Thanks.

Then starting with the "split at the comma" implementation is
perfectly fine.  People will learn not to use '"Doe, Jane" <jdoe>'
because that will not work while the implementation is limited, and
later when it is enhanced, they can start using that form.