threads / patch / 16300

v2Edit recipient addresses with the --compose flag

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

## tl;dr

5 messages between Nov 13, 2008 and Nov 14, 2008. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Ian Hilt· Nov 13, 2008, 02:50 UTC · lore

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

Show changes to git-send-email.perl +44 −3

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

Junio C Hamano· Nov 13, 2008, 03:18 UTC · re: Ian Hilt · lore

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

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

Show 7 quoted lines

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

Show 24 quoted lines

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

Show 6 quoted lines

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

Ian Hilt· Nov 14, 2008, 02:10 UTC · re: Junio C Hamano · lore

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

On Wed, 12 Nov 2008, Junio C Hamano wrote:

Show 12 quoted lines

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

Show 27 quoted lines

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

Show 11 quoted lines

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

Show 11 quoted lines

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

Show 8 quoted lines

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

Junio C Hamano· Nov 14, 2008, 03:31 UTC · re: Ian Hilt · lore

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

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

Show 15 quoted lines

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

Show 9 quoted lines

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

Ian Hilt· Nov 14, 2008, 16:58 UTC · re: Junio C Hamano · lore

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

On Thu, 13 Nov 2008, Junio C Hamano wrote:

Show 20 quoted lines

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

Show 18 quoted lines

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

Show 20 quoted lines

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