Payre Nathan <second.payre@xxxxxxxxx> writes:
> From: Tom Russello <tom.russello@xxxxxxxxxxxxxxxx>
>
> ---
Missing something here???
> 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?
> 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.
> @@ -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).
> +
> + 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.
> + 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.
> + # 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?
> + 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?
> + 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.
