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

Previous thread: Re: Init on push by Jakub Narebski on Saturday, November 8, 2008 - 4:06 pm. (7 messages)

Next thread: [PATCH] Documentation: rev-list: change a few instances of "git-cmd" to "git cmd" by Christian Couder on Sunday, November 9, 2008 - 9:46 am. (1 message)
To: Git Mailing List <git@...>
Cc: Pierre Habouzit <madcoder@...>
Date: Sunday, November 9, 2008 - 8:59 am

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

To: Ian Hilt <ian.hilt@...>
Cc: Pierre Habouzit <madcoder@...>, Junio C Hamano <gitster@...>, Git Mailing List <git@...>
Date: Monday, November 10, 2008 - 3:57 am

Le Sunday 09 November 2008 13:59:48 Ian Hilt, vous avez écrit :

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

To: Ian Hilt <ian.hilt@...>
Cc: Pierre Habouzit <madcoder@...>, Junio C Hamano <gitster@...>, Git Mailing List <git@...>
Date: Monday, November 10, 2008 - 3:59 am

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

To: Ian Hilt <ian.hilt@...>
Cc: Pierre Habouzit <madcoder@...>, Git Mailing List <git@...>
Date: Sunday, November 9, 2008 - 10:13 am

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

To: Francis Galiegue <fg@...>
Cc: Pierre Habouzit <madcoder@...>, Git Mailing List <git@...>
Date: Sunday, November 9, 2008 - 4:09 pm

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.

=09Ian

To: Git Mailing List <git@...>
Date: Monday, November 10, 2008 - 9:49 pm

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

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

--

To: Tait <git.git@...>
Cc: Git Mailing List <git@...>
Date: Tuesday, November 11, 2008 - 7:30 am

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

To: Francis Galiegue <fg@...>
Cc: Tait <git.git@...>, Git Mailing List <git@...>
Date: Tuesday, November 11, 2008 - 4:47 pm

So what does this script produce on your systems?

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

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

if ($1 eq ' ') {
=09print "\$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.

To: Git Mailing List <git@...>
Cc: Ian Hilt <ian.hilt@...>, Francis Galiegue <fg@...>
Date: Tuesday, November 11, 2008 - 6:14 pm

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

--

To: Ian Hilt <ian.hilt@...>
Cc: Tait <git.git@...>, Git Mailing List <git@...>
Date: Tuesday, November 11, 2008 - 4:53 pm

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

To: Francis Galiegue <fg@...>
Cc: Tait <git.git@...>, Git Mailing List <git@...>
Date: Tuesday, November 11, 2008 - 4:53 pm

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

To: Ian Hilt <ian.hilt@...>
Cc: Pierre Habouzit <madcoder@...>, Francis Galiegue <fg@...>, Git Mailing List <git@...>
Date: Sunday, November 9, 2008 - 6:09 pm

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

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

--

To: Junio C Hamano <gitster@...>
Cc: Pierre Habouzit <madcoder@...>, Francis Galiegue <fg@...>, Git Mailing List <git@...>
Date: Sunday, November 9, 2008 - 8:38 pm

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

To: Ian Hilt <ian.hilt@...>
Cc: Pierre Habouzit <madcoder@...>, Junio C Hamano <gitster@...>, Git Mailing List <git@...>
Date: Monday, November 10, 2008 - 3:49 am

No. The _lazy_ quantifiers (*?, ??, *?, +?, {...}?) are supported all right.

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

To: Git Mailing List <git@...>
Date: Monday, November 10, 2008 - 4:08 am

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

That is correct.

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

To: Ian Hilt <ian.hilt@...>
Cc: Pierre Habouzit <madcoder@...>, Francis Galiegue <fg@...>, Git Mailing List <git@...>
Date: Monday, November 10, 2008 - 1:18 am

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

To: Junio C Hamano <gitster@...>
Cc: Pierre Habouzit <madcoder@...>, Francis Galiegue <fg@...>, Git Mailing List <git@...>
Date: Monday, November 10, 2008 - 2:12 pm

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

Previous thread: Re: Init on push by Jakub Narebski on Saturday, November 8, 2008 - 4:06 pm. (7 messages)

Next thread: [PATCH] Documentation: rev-list: change a few instances of "git-cmd" to "git cmd" by Christian Couder on Sunday, November 9, 2008 - 9:46 am. (1 message)