[PATCH] git send-email: edit recipient addresses with the --compose flag

[PATCH] git send-email: edit recipient addresses with the --compose flag

[Ian Hilt]

Sometimes specifying the recipient addresses can be tedious on the
command-line.  This commit will allow the user to edit the recipient
addresses in their editor of choice.

Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
---
This is on top of Pierre's most recent series.  I'm not exactly happy
with the way it turned out, but it seems to function correctly.
Comments are most welcome.

  [ This is a resend.  I don't know what happened to the first mail I
    sent to the list. ]

 git-send-email.perl |   60 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index fd72127..3a22767 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -455,6 +455,9 @@ if ($compose) {
 	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
 	my $tpl_subject = $initial_subject || '';
 	my $tpl_reply_to = $initial_reply_to || '';
+	my $tpl_to = join(', ', @to);
+	my $tpl_cc = join(', ', @initial_cc);
+	my $tpl_bcc = join(', ', @bcclist);
 
 	print C <<EOT;
 From $tpl_sender # This line is ignored.
@@ -464,6 +467,9 @@ GIT: for the patch you are writing.
 GIT:
 GIT: Clear the body content if you don't wish to send a summary.
 From: $tpl_sender
+To: $tpl_to
+Cc: $tpl_cc
+Bcc: $tpl_bcc
 Subject: $tpl_subject
 In-Reply-To: $tpl_reply_to
 
@@ -487,9 +493,31 @@ EOT
 	open(C,"<",$compose_filename)
 		or die "Failed to open $compose_filename : " . $!;
 
+	local $/;
+	my $c_file = <C>;
+	$/ = "\n";
+	close(C);
+
+	my (@tmp_to, @tmp_cc, @tmp_bcc);
+
+	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
+		@tmp_to = get_recipients($1);
+	}
+	if ($c_file =~ /^Cc:\s*+(.+)\s*\nBcc:/ism) {
+		@tmp_cc = get_recipients($1);
+	}
+	if ($c_file =~ /^Bcc:\s*+(.+)\s*\nSubject:/ism) {
+		@tmp_bcc = get_recipients($1);
+	}
+
+
 	my $need_8bit_cte = file_has_nonascii($compose_filename);
 	my $in_body = 0;
 	my $summary_empty = 1;
+
+	open(C,"<",$compose_filename)
+		or die "Failed to open $compose_filename : " . $!;
+
 	while(<C>) {
 		next if m/^GIT: /;
 		if ($in_body) {
@@ -518,15 +546,21 @@ EOT
 		} elsif (/^From:\s*(.+)\s*$/i) {
 			$sender = $1;
 			next;
-		} elsif (/^(?:To|Cc|Bcc):/i) {
-			print "To/Cc/Bcc fields are not interpreted yet, they have been ignored\n";
-			next;
 		}
 		print C2 $_;
 	}
 	close(C);
 	close(C2);
 
+	if (@tmp_to) {
+		@to = @tmp_to;
+	}
+	if (@tmp_cc) {
+		@initial_cc = @tmp_cc;
+	}
+	if (@tmp_bcc) {
+		@bcclist = @tmp_bcc;
+	}
 	if ($summary_empty) {
 		print "Summary email is empty, skpping it\n";
 		$compose = -1;
@@ -1070,3 +1104,23 @@ sub file_has_nonascii {
 	}
 	return 0;
 }
+
+sub get_recipients {
+	my $match = shift(@_);
+	my @recipients = split(/\s*,\s*/, $match);
+	for (my $i = 0; $i <= $#recipients; ++$i) {
+		if ($recipients[$i] eq "") {
+			splice(@recipients, $i, 1);
+		} elsif ($recipients[$i] =~ /"/) {
+			my $x = $i;
+			my $tmp;
+			while ($recipients[$i] !~ /</) {
+				$tmp = join(', ', $recipients[$i],$recipients[$i+1]);
+				++$i;
+				splice(@recipients, $i, 1, $tmp);
+			}
+			splice(@recipients, $x, $i, $tmp);
+		}
+	}
+	return @recipients;
+}
-- 
1.6.0.3.523.g304d0

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Francis Galiegue]

Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
> Sometimes specifying the recipient addresses can be tedious on the
> command-line.  This commit will allow the user to edit the recipient
> addresses in their editor of choice.
>
> Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
> ---
[...]
> +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {

Greedy operators are only supported with perl 5.10 or more... I think it's a 
bad idea to use them...

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Ian Hilt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 765 bytes --]

On Sun, 9 Nov 2008, Francis Galiegue wrote:
> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
> > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> 
> Greedy operators are only supported with perl 5.10 or more... I think it's a 
> bad idea to use them...

The problem here was that a space should follow the field, but it may
not.  The user may unwarily backup over it.  "\s*" would match this
case.

But if there is a space, it is included in the "(.+)".  So I tried
"\s+", which did not include the space, but it won't include the first
address if there isn't a space after the field.

The quantified subpattern seemed to do the trick.  But, if it could
result in a dependency issue, I would agree this would be a bad idea.


	Ian

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Junio C Hamano]

Ian Hilt <ian.hilt@gmx.com> writes:

> On Sun, 9 Nov 2008, Francis Galiegue wrote:
>> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
>> > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
>> 
>> Greedy operators are only supported with perl 5.10 or more... I think it's a 
>> bad idea to use them...
>
> The problem here was that a space should follow the field, but it may
> not.  The user may unwarily backup over it.  "\s*" would match this
> case.
>
> But if there is a space, it is included in the "(.+)".  So I tried
> "\s+", which did not include the space, but it won't include the first
> address if there isn't a space after the field.
>
> The quantified subpattern seemed to do the trick.  But, if it could
> result in a dependency issue, I would agree this would be a bad idea.

You expect something non-blank there anyway, so why not do:

	To:\s*(\S.*?)\s*\n....

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Ian Hilt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1309 bytes --]

On Sun, 9 Nov 2008, Junio C Hamano wrote:
> Ian Hilt <ian.hilt@gmx.com> writes:
> 
> > On Sun, 9 Nov 2008, Francis Galiegue wrote:
> >> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
> >> > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> >> 
> >> Greedy operators are only supported with perl 5.10 or more... I think it's a 
> >> bad idea to use them...
> >
> > The problem here was that a space should follow the field, but it may
> > not.  The user may unwarily backup over it.  "\s*" would match this
> > case.
> >
> > But if there is a space, it is included in the "(.+)".  So I tried
> > "\s+", which did not include the space, but it won't include the first
> > address if there isn't a space after the field.
> >
> > The quantified subpattern seemed to do the trick.  But, if it could
> > result in a dependency issue, I would agree this would be a bad idea.
> 
> You expect something non-blank there anyway, so why not do:
> 
> 	To:\s*(\S.*?)\s*\n....

That works.  Although, I seem to be missing Francis' point.  According
to perlre, a quantified subpattern is "greedy".  So a "greedy operator"
is any one of the standard quantified subpatterns.  The "+" and "?"
modify its matching behavior.  And it seems to me that it _has_ to use a
q.s. to work ...

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Junio C Hamano]

Ian Hilt <ian.hilt@gmx.com> writes:

> On Sun, 9 Nov 2008, Junio C Hamano wrote:
>> Ian Hilt <ian.hilt@gmx.com> writes:
>> 
>> > On Sun, 9 Nov 2008, Francis Galiegue wrote:
>> >> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
>> >> > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
>> >> 
>> >> Greedy operators are only supported with perl 5.10 or more... I think it's a 
>> >> bad idea to use them...
>> ...
>> You expect something non-blank there anyway, so why not do:
>> 
>> 	To:\s*(\S.*?)\s*\n....
>
> That works.  Although, I seem to be missing Francis' point.  According
> to perlre, a quantified subpattern is "greedy".  So a "greedy operator"
> is any one of the standard quantified subpatterns.  The "+" and "?"
> modify its matching behavior.  And it seems to me that it _has_ to use a
> q.s. to work ...

The "perlre" documentation you are reading is from Perl 5.10.0; check
"perldelta" documentation next to it.

I think you are wrong in saying that "it _has_ to use".  Yes, you _can_
use possessive quantifiers to write that pattern (provided if you can
limit your users to Perl 5.10.0 or later), but you do _not_ have to (and I
just showed you how).  By not using the new feature, you can make it work
for people with older version of Perl.

Not everybody who uses git can upgrade their Perl to newer versions.  We
try to stick to "5.6.1 or later"; anything that is not available in 5.8
series is too new to be used outside the contrib/ area.

That's the point Francis raised that you missed.  

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Francis Galiegue]

Le Monday 10 November 2008 01:38:30 Ian Hilt, vous avez écrit :
> On Sun, 9 Nov 2008, Junio C Hamano wrote:
> > Ian Hilt <ian.hilt@gmx.com> writes:
> > > On Sun, 9 Nov 2008, Francis Galiegue wrote:
> > >> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
> > >> > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> > >>
> > >> Greedy operators are only supported with perl 5.10 or more... I think
> > >> it's a bad idea to use them...
> > >
> > > The problem here was that a space should follow the field, but it may
> > > not.  The user may unwarily backup over it.  "\s*" would match this
> > > case.
> > >
> > > But if there is a space, it is included in the "(.+)".  So I tried
> > > "\s+", which did not include the space, but it won't include the first
> > > address if there isn't a space after the field.
> > >
> > > The quantified subpattern seemed to do the trick.  But, if it could
> > > result in a dependency issue, I would agree this would be a bad idea.
> >
> > You expect something non-blank there anyway, so why not do:
> >
> > 	To:\s*(\S.*?)\s*\n....
>
> That works.  Although, I seem to be missing Francis' point.

No. The _lazy_ quantifiers (*?, ??, *?, +?, {...}?) are supported all right. 
But they should be avoided in general.

> According 
> to perlre, a quantified subpattern is "greedy".  So a "greedy operator"
> is any one of the standard quantified subpatterns.  The "+" and "?"
> modify its matching behavior.  And it seems to me that it _has_ to use a
> q.s. to work ...

My wording may be bad, then. They're not greedy, they just don't allow for 
backtracking. They are more than greedy. Let me explain.

Consider "number 42" for instance. If you match it against:

* .*(\d+) => $1 would be "2": the * eats everything but _has_ to backtrack for 
\d+ to get anything, but just one number is enough;
* .*?(\d+) => $1 would be "42": as *? is lazy, this means that after each 
match, it looks to see whether the next element in the regex would match 
anything; as soon as \d+ matches 4, .*? stops there;
* .*+(\d+) => $1 would match nothing! *+ eats everything, but the + afterwards 
_doesn't allow it to backtrack_.

I hope this makes things a little clearer ;) I think the correct term for *+, 
++, ?+ etc is "possessive" quantifiers, I'm just not sure.

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Francis Galiegue]

Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
[...]
> +	my @recipients = split(/\s*,\s*/, $match);

This is where it gets complicated, for the "hey, I am" <some@one> case...

But then there is a solution: use a negative lookahead for the split regex.

I thought about splitting against /\s*,\s*(?![^"]+(?:\"[^*]*)*)"/.

A negative lookbehind would have been clearer to write, but perl doesn't 
support arbitrary length negative lookbehinds, except by using a very, very 
arcane construct.


-- 
fge

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Francis Galiegue]

Le Monday 10 November 2008 08:57:10 Francis Galiegue, vous avez écrit :
> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
> [...]
>
> > +	my @recipients = split(/\s*,\s*/, $match);
>
> This is where it gets complicated, for the "hey, I am" <some@one> case...
>
> But then there is a solution: use a negative lookahead for the split regex.
>
> I thought about splitting against /\s*,\s*(?![^"]+(?:\"[^*]*)*)"/.
>

Oops, that should have been /\s*,\s*(?![^"]+(?:\"[^*]*)*")/, sorry :/

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Aristotle Pagaltzis]

* Francis Galiegue <fg@one2team.com> [2008-11-10 08:55]:
> Le Monday 10 November 2008 01:38:30 Ian Hilt, vous avez écrit :
> > On Sun, 9 Nov 2008, Junio C Hamano wrote:
> > > Ian Hilt <ian.hilt@gmx.com> writes:
> > > > On Sun, 9 Nov 2008, Francis Galiegue wrote:
> > > >> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
> > > >> > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> > > >>
> > > >> Greedy operators are only supported with perl 5.10 or more... I think
> > > >> it's a bad idea to use them...

Possessive quantification is supported in much earlier versions
of Perl, it’s just more awkward syntactically:

    /^To:(?>\s*)(.+)\s*\nCc:/ism

But possessification is not going to make a difference in this
regex, since .+ can match anything that \s* can also match, so
the only difference is that if the regex does happen to
backtrack, it will backtrack over all the spaces after the To:
at once instead of one at a time.

I have only just subscribed so I do not have enough context to
know what the problem is, but based on what I have seen so far it
seems to me that all you want is simply

    /^To:\s?(.+)\s*\nCc:/ism

although I have to wonder if the /s modifier here is really what
you want.

> I think the correct term for *+, ++, ?+ etc is "possessive"
> quantifiers, I'm just not sure.

That is correct.

Regards,
-- 
Aristotle Pagaltzis // <http://plasmasturm.org/>

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Ian Hilt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1517 bytes --]

On Mon, 10 Nov 2008, Junio C Hamano wrote:
> Ian Hilt <ian.hilt@gmx.com> writes:
> 
> > On Sun, 9 Nov 2008, Junio C Hamano wrote:
> >> Ian Hilt <ian.hilt@gmx.com> writes:
> >> 
> >> > On Sun, 9 Nov 2008, Francis Galiegue wrote:
> >> >> Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :
> >> >> > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> >> >> 
> >> >> Greedy operators are only supported with perl 5.10 or more... I think it's a 
> >> >> bad idea to use them...
> >> ...
> >> You expect something non-blank there anyway, so why not do:
> >> 
> >> 	To:\s*(\S.*?)\s*\n....
> >
> > That works.  Although, I seem to be missing Francis' point.  According
> > to perlre, a quantified subpattern is "greedy".  So a "greedy operator"
> > is any one of the standard quantified subpatterns.  The "+" and "?"
> > modify its matching behavior.  And it seems to me that it _has_ to use a
> > q.s. to work ...
> 
> The "perlre" documentation you are reading is from Perl 5.10.0; check
> "perldelta" documentation next to it.
> 
> I think you are wrong in saying that "it _has_ to use".  Yes, you _can_
> use possessive quantifiers to write that pattern (provided if you can
> limit your users to Perl 5.10.0 or later), but you do _not_ have to (and I
> just showed you how).  By not using the new feature, you can make it work
> for people with older version of Perl.

Right.  I was saying it has to use quantifiers, in general, not that it
has to use possessive quantifiers.

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Tait]

> > > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> > 
> > Greedy operators are only supported with perl 5.10 or more... I think it's a 
> > bad idea to use them...
> 
> The problem here was that a space should follow the field, but it may
> not.  The user may unwarily backup over it.  "\s*" would match this
> case.
> 
> But if there is a space, it is included in the "(.+)".

Not in any version of Perl to which I have access. 

Why doesn't
	/^To:\s*(.+)\s*\nCc:/ism
work?

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Francis Galiegue]

Le Tuesday 11 November 2008 02:49:19 Tait, vous avez écrit :
> > > > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> > >
> > > Greedy operators are only supported with perl 5.10 or more... I think
> > > it's a bad idea to use them...
> >
> > The problem here was that a space should follow the field, but it may
> > not.  The user may unwarily backup over it.  "\s*" would match this
> > case.
> >
> > But if there is a space, it is included in the "(.+)".
>
> Not in any version of Perl to which I have access.
>

And if you see a space in (.+), your regex engine is buggy anyway.


-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Ian Hilt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1116 bytes --]

On Tue, 11 Nov 2008, Francis Galiegue wrote:
> Le Tuesday 11 November 2008 02:49:19 Tait, vous avez écrit :
> > > > > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> > > >
> > > > Greedy operators are only supported with perl 5.10 or more... I think
> > > > it's a bad idea to use them...
> > >
> > > The problem here was that a space should follow the field, but it may
> > > not.  The user may unwarily backup over it.  "\s*" would match this
> > > case.
> > >
> > > But if there is a space, it is included in the "(.+)".
> >
> > Not in any version of Perl to which I have access.
> >
> 
> And if you see a space in (.+), your regex engine is buggy anyway.

So what does this script produce on your systems?


#!/usr/bin/perl -Tw
--8<--
use strict;
my $ws = "To: \nCc:";

$ws =~ /^To:\s*(.+)\s*\nCc:/ism;

if ($1 eq ' ') {
	print "\$1 is equal to a space.\n";
}
-->8--

On mine, it prints the message.  So it seems it is matching _a_ space.
This resulted in an illegal recipient field.  Junio's suggestion to
change this to

  /^To:\s*(\S.+?)\s*\nCc:/ism

worked beautifully.

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Ian Hilt]

On Tue, 11 Nov 2008, Ian Hilt wrote:

Hm, how about keeping the path to perl in the snippet,

--8<--
#!/usr/bin/perl -Tw
use strict;
my $ws = "To: \nCc:";

$ws =~ /^To:\s*(.+)\s*\nCc:/ism;

if ($1 eq ' ') {
	print "\$1 is equal to a space.\n";
}
-->8--

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Francis Galiegue]

Le Tuesday 11 November 2008 21:47:55 Ian Hilt, vous avez écrit :
> On Tue, 11 Nov 2008, Francis Galiegue wrote:
> > Le Tuesday 11 November 2008 02:49:19 Tait, vous avez écrit :
> > > > > > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> > > > >
> > > > > Greedy operators are only supported with perl 5.10 or more... I
> > > > > think it's a bad idea to use them...
> > > >
> > > > The problem here was that a space should follow the field, but it may
> > > > not.  The user may unwarily backup over it.  "\s*" would match this
> > > > case.
> > > >
> > > > But if there is a space, it is included in the "(.+)".
> > >
> > > Not in any version of Perl to which I have access.
> >
> > And if you see a space in (.+), your regex engine is buggy anyway.
>
> So what does this script produce on your systems?
>
>
> #!/usr/bin/perl -Tw
> --8<--
> use strict;
> my $ws = "To: \nCc:";
>
> $ws =~ /^To:\s*(.+)\s*\nCc:/ism;
>
> if ($1 eq ' ') {
> 	print "\$1 is equal to a space.\n";
> }
> -->8--
>
> On mine, it prints the message.  So it seems it is matching _a_ space.

Which is perfectly normal. The first \s* wanted spaces, it got them. But it 
left nothing for the capturing .+ behind. And any quantifier (except when it 
is possessive) _MUST_ backtrack in order for the full regex to complete. This 
is why the .+ captured the space: the first \s* was perfectly fine with no 
space at all, and the second, well, didn't find any space but it didn't care 
either.

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] git send-email: edit recipient addresses with the --compose flag

[Tait]

> > > > > > +	if ($c_file =~ /^To:\s*+(.+)\s*\nCc:/ism) {
> > > > >
> > > > > Greedy operators are only supported with perl 5.10 or more... I think
> > > > > it's a bad idea to use them...
> > > >
> > > > The problem here was that a space should follow the field, but it may
> > > > not.  The user may unwarily backup over it.  "\s*" would match this
> > > > case.
> > > >
> > > > But if there is a space, it is included in the "(.+)".
> > >
> > > Not in any version of Perl to which I have access.
> > >
> > 
> > And if you see a space in (.+), your regex engine is buggy anyway.
> 
> So what does this script produce on your systems?
> 
> --8<--
> #!/usr/bin/perl -Tw
> use strict;
> my $ws = "To: \nCc:";
> 
> $ws =~ /^To:\s*(.+)\s*\nCc:/ism;
> 
> if ($1 eq ' ') {
> 	print "\$1 is equal to a space.\n";
> }
> -->8--

It does match a space in that case. I misunderstood the problem this was 
trying to solve. (Sorry for the confusion.)