I Will send the modification in the next patch, I prefer to refractor
a part of the code before.
>> 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.
Remove "approprietly" done.
>> @@ -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).
I will remove lines which use validate_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.
In fact $fh is reuse at the end of the if($quote_email) {} but if you
don't see it maybe it's because it's anormal to reuse it after a
long block of code, that's why I think to create a subroutine
for the following code which is similar to the part of if($compose).
foreach (@header) {
my $initial_sender = $sender || $repoauthor || $repocommitter || '';
chomp;
if (/^Subject:\s+(.*)$/i) {
my $prefix_re = "";
my $subject_re = $1;
if ($1 =~ /^[^Re:]/) {
$prefix_re = "Re: ";
}
$initial_subject = $prefix_re . $subject_re;
} elsif (/^From:\s+(.*)$/i) {
$recipient = $1;
push @initial_to, $recipient;
} elsif (/^To:\s+(.*)$/i) {
foreach my $addr (parse_address_line($1)) {
if (!($addr eq $initial_sender)) {
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;
} elsif (/^Date: (.*)/i) {
$date = $1;
}
}
I close $fh after the second call then.
>> + # 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?
"_re" mean regular expression but maybe it's clumsy because
it contain the result of a regular expression. What do you think
about rename it into "$prefix" and "$subject" ?
2017-11-01 3:44 GMT+01:00 Junio C Hamano <gitster@xxxxxxxxx>:
> 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.
