Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Tom Russello <tom.russello@grenoble-inp.org> writes:

> This option involves the `--compose` mode to edit the cover letter quoting the

s/involves/implies/

?

I don't think this is right: I often reply to an email with a single
patch, for which it would clearly be overkill to have a cover-letter.

Your --quote-mail does two things:

1) Populate the To and Cc field

2) Include the original message body with quotation prefix.

When not using --compose, 1) clearly makes sense already, and there's no
reason to prevent this use-case. When sending a single patch, 2) also
makes sense as "below-tripple-dash comment", like

  This is the commit message for feature A.
  ---
  John Smith wrote:
  > You should implement feature A.

  Indeed, here's a patch.

  modified-file.c   | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-

When sending multiple patches without --compose, 2) may not make sense,
but I think a sane behavior would be:

* If --compose is given, cite the message there.

* If --compose is not given, don't send a cover-letter but cite the body
  as comment in the first patch.

As a first step, the second point can be changed to "if --compose is not
given, don't cite the message, just populate the To: and Cc: fields".

> ---
>
> diff --git a/git-send-email.perl b/git-send-email.perl

No diffstat?

> @@ -638,6 +640,98 @@ if (@files) {
>  	print STDERR "\nNo patch files specified!\n\n";
>  	usage();
>  }
> +my $message_quoted;
> +if ($quote_mail) {

Style: The code you're adding doesn't look related to the code right
before => separate them with a blank line.

> +	while(<$fh>) {

Style: space before (.

> +			push(@header, $_);

I think the code would be clearer if @header was a list of pairs
(header-name, header-content). Then you'd need much less regex magic
when going through it.

> +		#for files containing crlf line endings

Sytle: space after #.

> +	foreach(@header) {

Space before (.

> +			elsif (/^From:\s+(.*)$/i) {
> +				push @initial_to, $1;
> +			}
> +			elsif (/^To:\s+(.*)$/i) {
> +				foreach my $addr (parse_address_line($1)) {
> +					if (!($addr eq $initial_sender)) {
> +						push @initial_to, $addr;
> +					}
> +				}

This adds the content of the To: field in the original email to the Cc:
field in the new message, right? If so, this is a weird behavior: when
following up to an email, one usually addresses to the person s/he's
replying, keeping the others Cc-ed, hence the original From: becomes the
To header, and the original To: and Cc: become Cc:.

> +			} elsif (/^Cc:\s+(.*)$/i) {

Style: IIRC, there's no consensus on whether "elsif" should be on the
same line as the closing }, but please follow the same convention inside
a single if/elsif/ chain.

> +	#Message body

Style: space after # (more below). And while you're there, the comment
could be "Quote the message body" or so, to give a hint to the user
about what's going on.

> +	while (<$fh>) {
> +		#for files containing crlf line endings
> +		$_=~ s/\r//g;
> +		my $space="";

Style: spaces around =.

> @@ -676,6 +771,8 @@ From: $tpl_sender
>  Subject: $tpl_subject
>  In-Reply-To: $tpl_reply_to
>  
> +$tpl_quote
> +
>  EOT

Doesn't this add two extra useless blank lines if $tpl_quote is empty?

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