Re: [PATCH 1/2] quote-email populates the fields
Payre Nathan <second.payre@gmail.com> writes:
> From: Tom Russello <tom.russello@grenoble-inp.org>
>
> ---
Missing something here???
Show 16 quoted lines
> Documentation/git-send-email.txt | 3 +
> git-send-email.perl | 70 ++++++++++++++++++++++-
> t/t9001-send-email.sh | 117 +++++++++++++++++++++++++--------------
> 3 files changed, 147 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index bac9014ac..710b5ff32 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -106,6 +106,9 @@ illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`:
> Only necessary if --compose is also set. If --compose
> is not set, this will be prompted for.
>
> +--quote-email=<email_file>::
> + Fill appropriately header fields for the reply to the given email.
> +
The cover letter said:
This patch allows send-email to do most of the job for the user, who can
now save the email to a file and use: git send-email --quote-email=<file>
"To" and "Cc" will be added automaticaly and the email quoted.
It's possible to edit the email before sending with --compose.and I somehow expected to see the body of the e-mail this option is "quoting" to be also inserted in the text. After all, that is what "quote" means.
But the description above (and the code below, judging from the way the reading from $fh that was opened form $quote_email stops at the first blank line, aka end of header) says what is happening is quite different. The contents of the file is used to extract what the user would have given to --cc/--to/--in-reply-to from the command line by looking at it, if this option were not available.
I personally prefer the "pick up the header information so that the user do not have to formulate the command line options" behaviour that does *NOT* quote the body of the message into the outgoing message. So:
* Do not call this option "quote" anything; you are not quoting,
just using some info from the given file.
I wonder if we can simply reuse "--in-reply-to" option for this
purpose. If it is a message id and not a file on the filesystem,
we behave just as before. Otherwise we try to open it as a file
and grab the "Message-ID:" header from it and use it.
* The description "Fill *appropriately* header fileds" is useless,
as what looks "appropriate" to you is not clear/known to
readers. Instead, say what header is filled with what
information (e.g. "find Message-Id: and place its value on
In-Reply-To: header").
For that matter, "To and CC will be added automatically" in the
coer letter is still vague; are you reading To/CC in the given
file and placing their values on some (unnamed) header of the
outgoing message? Or are you reading some (unnamed) header in
the given file and placing their values on To/CC header of the
outging message?
Show 9 quoted lines
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..665c47d15 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -57,6 +57,7 @@ git send-email --dump-aliases
> --[no-]bcc <str> * Email Bcc:
> --subject <str> * Email "Subject:"
> --in-reply-to <str> * Email "In-Reply-To:"
> + --quote-email <file> * Populate header fields appropriately.
Likewise. If what's "appropriate" is clear to the readers, the word in this description adds no value because everybody would know how fields are populated. Otherwise, it does not add any value because everybody would have no clue how fields are populated.
Show 8 quoted lines
> @@ -652,6 +654,70 @@ if (@files) {
> usage();
> }
>
> +if ($quote_email) {
> + my $error = validate_patch($quote_email);
> + die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
> + if $error;validate_patch() calls sendemail-validate hook that is expecting to be fed a patch email you are going to send out that might have errors so that it can catch it and save you from embarrassment. The file you are feeding it is *NOT* what you are going to send out, but is what you are responding to with your patch. Even if it had an embarassing error as a patch, that is not something you care about (and it is something you received, so catching this late won't save the sender from embarrassment anyway).
Show 11 quoted lines
> +
> + my @header = ();
> +
> + open my $fh, "<", $quote_email or die "can't open file $quote_email";
> +
> + # Get the email header
> + while (<$fh>) {
> + # Turn crlf line endings into lf-only
> + s/\r//g;
> + last if /^\s*$/;
> + if (/^\s+\S/ and @header) {I wonder how significant this requirement to have at least one "\S" on the line is. I know you copied&pasted this from the main sending loop, so this is not a new issue and not something we may want to fix in this patch.
Show 7 quoted lines
> + chomp($header[$#header]);
> + s/^\s+/ /;
> + $header[$#header] .= $_;
> + } else {
> + push(@header, $_);
> + }
> + }You do not use $fh after this point. Do not force readers to realize that fact by scanning to the end of the function--instead, close it here.
Show 9 quoted lines
> + # Parse the header
> + foreach (@header) {
> + my $initial_sender = $sender || $repoauthor || $repocommitter || '';
> +
> + chomp;
> +
> + if (/^Subject:\s+(.*)$/i) {
> + my $prefix_re = "";
> + my $subject_re = $1;What does "_re" mean in the variable name $subject_re?
Show 9 quoted lines
> + if ($1 =~ /^[^Re:]/) {
> + $prefix_re = "Re: ";
> + }
> + $initial_subject = $prefix_re . $subject_re;
> + } elsif (/^From:\s+(.*)$/i) {
> + push @initial_to, $1;
> + } elsif (/^To:\s+(.*)$/i) {
> + foreach my $addr (parse_address_line($1)) {
> + if (!($addr eq $initial_sender)) {This if() condition makes a policy decision; shouldn't it honor the setting of "--[no-]suppress-from", "--suppress-cc" and friends?
Show 21 quoted lines
> + push @initial_cc, $addr;
> + }
> + }
> + } elsif (/^Cc:\s+(.*)$/i) {
> + foreach my $addr (parse_address_line($1)) {
> + my $qaddr = unquote_rfc2047($addr);
> + my $saddr = sanitize_address($qaddr);
> + if ($saddr eq $initial_sender) {
> + next if ($suppress_cc{'self'});
> + } else {
> + next if ($suppress_cc{'cc'});
> + }
> + push @initial_cc, $addr;
> + }
> + } elsif (/^Message-Id: (.*)/i) {
> + $initial_reply_to = $1;
> + } elsif (/^References:\s+(.*)/i) {
> + $initial_references = $1;
> + }
> + }
> + $initial_references = $initial_references . $initial_reply_to;I cannot see how this can produce correct result by simply concatenating them with nothing in between. Shouldn't you make sure there is a SP in between, at least?
By the way, if you are adding a new variable $initial_references, make sure it is initialized to either an empty string or an undef (and if you choose to do the latter, the right hand side of this assignment cannot blindly reference $initial_references that could still be undef); the way the existing code handles $initial_reply_to may serve as an example.
