[PATCH v2] Edit recipient addresses with the --compose flag

[PATCH v2] Edit recipient addresses with the --compose flag

[Ian Hilt]

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

Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
---
Here's an updated commit with improved regex's from Junio and Francis.

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

diff --git a/git-send-email.perl b/git-send-email.perl
index 9039cfd..4d8aaa7 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -480,6 +480,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.
@@ -489,6 +492,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
 
@@ -512,9 +518,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.+?)\s*\nCc:/ism) {
+		@tmp_to = get_recipients($1);
+	}
+	if ($c_file =~ /^Cc:\s*(\S.+?)\s*\nBcc:/ism) {
+		@tmp_cc = get_recipients($1);
+	}
+	if ($c_file =~ /^Bcc:\s*(\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) {
@@ -543,15 +571,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, skipping it\n";
 		$compose = -1;
@@ -1095,3 +1129,10 @@ sub file_has_nonascii {
 	}
 	return 0;
 }
+
+sub get_recipients {
+	my $match = shift(@_);
+	my @recipients = split(/\s*,\s*(?![^"]+(?:\"[^*]*)*")/, $match);
+
+	return @recipients;
+}
-- 
1.6.0.3.523.g304d0

Re: [PATCH v2] Edit recipient addresses with the --compose flag

[Junio C Hamano]

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

> Sometimes specifying the recipient addresses can be tedious on the
> command-line.  This commit allows the user to edit the recipient
> addresses in their editor of choice.
>
> Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
> ---
> Here's an updated commit with improved regex's from Junio and Francis.

This heavily depends on Pierre's patch, so I am CC'ing him for comments.
Until his series settles down, I cannot apply this anyway.

> @@ -489,6 +492,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
>  
> @@ -512,9 +518,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.+?)\s*\nCc:/ism) {
> +		@tmp_to = get_recipients($1);
> +	}

Why "\S.+?" and not "\S.*?"?  A local user whose login name is 'q' is
disallowed?

Why does the user must keep "Cc:" in order for this new code to pick up
the list of recipients?  In other words, you are forbidding the user from
removing the entire "Cc:" line, even when the message should not be Cc'ed
to anywhere.  Instead there has to remain an empty Cc: line.  Worse yet,
such an empty "Cc:" line is printed to C2 with your patch and eventually
fed to sendmail.  I think it is a violation of 2822 to have Cc: that is
empty, as the format is specified as:

    cc              =       "Cc:" address-list CRLF
    bcc             =       "Bcc:" (address-list / [CFWS]) CRLF
    address-list    =       (address *("," address)) / obs-addr-list

> +	if ($c_file =~ /^Cc:\s*(\S.+?)\s*\nBcc:/ism) {
> +		@tmp_cc = get_recipients($1);
> +	}
> +	if ($c_file =~ /^Bcc:\s*(\S.+?)\s*\nSubject:/ism) {
> +		@tmp_bcc = get_recipients($1);
> +	}

Exactly the same comment applies to Bcc and Subject part of the parsing.

I think the parsing code you introduced simply suck.  Why isn't it done as
a part of the main loop to read the same file that already exists?

Unlike your additional code above that reads the whole file into a scalar
only to discard, the existing main loop processes one line at a file
(which should be more memory efficient), and you are not handling the
header continuation line anyway, processing one line at a time would make
your code much simpler (for one thing, you do not have to do /sm at all).
Also it won't be confused as your version would if the message happens to
have "To:" or "Cc:" in the message part, thanks to $in_body variable check
that is already in the code.

Re: [PATCH v2] Edit recipient addresses with the --compose flag

[Ian Hilt]

On Wed, 12 Nov 2008, Junio C Hamano wrote:
> Ian Hilt <ian.hilt@gmx.com> writes:
> 
> > Sometimes specifying the recipient addresses can be tedious on the
> > command-line.  This commit allows the user to edit the recipient
> > addresses in their editor of choice.
> >
> > Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
> > ---
> > Here's an updated commit with improved regex's from Junio and Francis.
> 
> This heavily depends on Pierre's patch, so I am CC'ing him for comments.
> Until his series settles down, I cannot apply this anyway.

I didn't realize this was such a bad time to submit this patch.

> > @@ -489,6 +492,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
> >  
> > @@ -512,9 +518,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.+?)\s*\nCc:/ism) {
> > +		@tmp_to = get_recipients($1);
> > +	}
> 
> Why "\S.+?" and not "\S.*?"?  A local user whose login name is 'q' is
> disallowed?

True.  Thanks for pointing this out.

> Why does the user must keep "Cc:" in order for this new code to pick up
> the list of recipients?  In other words, you are forbidding the user from
> removing the entire "Cc:" line, even when the message should not be Cc'ed
> to anywhere.  Instead there has to remain an empty Cc: line.  Worse yet,
> such an empty "Cc:" line is printed to C2 with your patch and eventually
> fed to sendmail.  I think it is a violation of 2822 to have Cc: that is
> empty, as the format is specified as:
> 
>     cc              =       "Cc:" address-list CRLF
>     bcc             =       "Bcc:" (address-list / [CFWS]) CRLF
>     address-list    =       (address *("," address)) / obs-addr-list

I think you're mistaken here.  It is entirely possible to delete the Cc
and Bcc lines with no ill effect.  It is also possible to leave them in
and not add any addresses and the code won't feed these empty lines to
sendmail.  In the subroutine send_message, I believe a check is made to
determine if $cc is equal to ''.  If it's not, then it will use it.  The
Bcc list is even simpler.  It is gathered into @recipients via
unique_email_list().  If it's empty, nothing happens.

> > +	if ($c_file =~ /^Cc:\s*(\S.+?)\s*\nBcc:/ism) {
> > +		@tmp_cc = get_recipients($1);
> > +	}
> > +	if ($c_file =~ /^Bcc:\s*(\S.+?)\s*\nSubject:/ism) {
> > +		@tmp_bcc = get_recipients($1);
> > +	}
> 
> Exactly the same comment applies to Bcc and Subject part of the parsing.
> 
> I think the parsing code you introduced simply suck.  Why isn't it done as
> a part of the main loop to read the same file that already exists?

Multiline recipient fields.

I know rfc 2822 that you cited above specifies that the address list not
contain a CRLF but as a terminator.  However, I thought that for
readability's sake it would be good to enable the user to introduce a
line break into the recipient field.  This is why I used the regex's the
way I did and slurp'd the file rather than worked on it line-by-line.

> Unlike your additional code above that reads the whole file into a scalar
> only to discard, the existing main loop processes one line at a file
> (which should be more memory efficient), and you are not handling the
> header continuation line anyway, processing one line at a time would make
> your code much simpler (for one thing, you do not have to do /sm at all).
> Also it won't be confused as your version would if the message happens to
> have "To:" or "Cc:" in the message part, thanks to $in_body variable check
> that is already in the code.

I definitely agree.  I started out coding exactly this way but then
thought about Pierre's comment about bloated To and Cc fields.  This is
why I wanted to allow the user to introduce line breaks into the
recipient fields.  It's a lot easier to read a nicely formatted email
list if there are a lot of addresses.

As a second thought, I probably should have put RFC into the subject of
this patch.

And as far as my code's recipient regex matching the body of the
message, I'll try to find a solution.  It may not be possible to do
multiline recipient fields without this problem.  Thanks for reviewing
this.

Re: [PATCH v2] Edit recipient addresses with the --compose flag

[Junio C Hamano]

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

> On Wed, 12 Nov 2008, Junio C Hamano wrote:
>> Ian Hilt <ian.hilt@gmx.com> writes:
>> 
>> > Sometimes specifying the recipient addresses can be tedious on the
>> > command-line.  This commit allows the user to edit the recipient
>> > addresses in their editor of choice.
>> >
>> > Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
>> > ---
>> > Here's an updated commit with improved regex's from Junio and Francis.
>> 
>> This heavily depends on Pierre's patch, so I am CC'ing him for comments.
>> Until his series settles down, I cannot apply this anyway.
>
> I didn't realize this was such a bad time to submit this patch.

It is not a bad time.  I just won't be able to apply it right away, but
people (like Pierre) who are interested in send-email enhancement can help
improving your patch by reviewing.

>> Why does the user must keep "Cc:" in order for this new code to pick up
>> the list of recipients?  ...
>> 
>>     cc              =       "Cc:" address-list CRLF
>>     bcc             =       "Bcc:" (address-list / [CFWS]) CRLF
>>     address-list    =       (address *("," address)) / obs-addr-list
>
> I think you're mistaken here.  It is entirely possible to delete the Cc
> and Bcc lines with no ill effect.

You have this piece of code

>> > +	if ($c_file =~ /^To:\s*(\S.+?)\s*\nCc:/ism) {
>> > +		@tmp_to = get_recipients($1);
>> > +	}

to pick up the "To: " addressees.  If your user deletes Cc: line, would
that regexp still capture them in @tmp_to?  How?

> determine if $cc is equal to ''.  If it's not, then it will use it.

Ah, somehow I thought C2 you are writing into (message.final) was used as
the final payload, but you are right.  The foreach () loop at the toplevel
reads them and interprets them.

>> I think the parsing code you introduced simply suck.  Why isn't it done as
>> a part of the main loop to read the same file that already exists?
>
> Multiline recipient fields.

So you were trying to handle folded headers after all, I see.

But if you were to go that route, I think you are much better off doing so
by enabling the header folding for all the header lines in the while (<C>)
loop that currently reads one line at a time.

I however hove to wonder why we are not using any canned e-mail header
parser for this part of the code.  Surely there must be a widely used one
that everybody who writes Perl uses???

Re: [PATCH v2] Edit recipient addresses with the --compose flag

[Ian Hilt]

On Thu, 13 Nov 2008, Junio C Hamano wrote:
> Ian Hilt <ian.hilt@gmx.com> writes:
> > On Wed, 12 Nov 2008, Junio C Hamano wrote:
> >> Ian Hilt <ian.hilt@gmx.com> writes:
> >> 
> >> > Sometimes specifying the recipient addresses can be tedious on the
> >> > command-line.  This commit allows the user to edit the recipient
> >> > addresses in their editor of choice.
> >> >
> >> > Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
> >> > ---
> >> > Here's an updated commit with improved regex's from Junio and Francis.
> >> 
> >> This heavily depends on Pierre's patch, so I am CC'ing him for comments.
> >> Until his series settles down, I cannot apply this anyway.
> >
> > I didn't realize this was such a bad time to submit this patch.
> 
> It is not a bad time.  I just won't be able to apply it right away, but
> people (like Pierre) who are interested in send-email enhancement can help
> improving your patch by reviewing.

And improvement it needs.

> >> Why does the user must keep "Cc:" in order for this new code to pick up
> >> the list of recipients?  ...
> >> 
> >>     cc              =       "Cc:" address-list CRLF
> >>     bcc             =       "Bcc:" (address-list / [CFWS]) CRLF
> >>     address-list    =       (address *("," address)) / obs-addr-list
> >
> > I think you're mistaken here.  It is entirely possible to delete the Cc
> > and Bcc lines with no ill effect.
> 
> You have this piece of code
> 
> >> > +	if ($c_file =~ /^To:\s*(\S.+?)\s*\nCc:/ism) {
> >> > +		@tmp_to = get_recipients($1);
> >> > +	}
> 
> to pick up the "To: " addressees.  If your user deletes Cc: line, would
> that regexp still capture them in @tmp_to?  How?

Wow.  I don't know why I didn't catch that.  You're right.

> > determine if $cc is equal to ''.  If it's not, then it will use it.
> 
> Ah, somehow I thought C2 you are writing into (message.final) was used as
> the final payload, but you are right.  The foreach () loop at the toplevel
> reads them and interprets them.
> 
> >> I think the parsing code you introduced simply suck.  Why isn't it done as
> >> a part of the main loop to read the same file that already exists?
> >
> > Multiline recipient fields.
> 
> So you were trying to handle folded headers after all, I see.
> 
> But if you were to go that route, I think you are much better off doing so
> by enabling the header folding for all the header lines in the while (<C>)
> loop that currently reads one line at a time.
> 
> I however hove to wonder why we are not using any canned e-mail header
> parser for this part of the code.  Surely there must be a widely used one
> that everybody who writes Perl uses???

I thought about this, but I didn't want to introduce another dependency.
I'm sure there's an easy way to do this stuff but I just don't have
enough perl know-how yet.

In any case, I think this concept needs serious work considering the
points that have been raised.  I don't have a lot of time to devote to
this however much I would like to complete it sooner than later.  So for
now, unless someone else wants to take up the mantle, I think it would
be better to put this patch aside.