Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields
- From
- Samuel GROOT <samuel.groot@grenoble-inp.org>
- Date
- Jun 14, 2016, 22:35 UTC
- Message-ID
- <5221a7c5-3f4b-4061-6fc2-5019d784e03f@grenoble-inp.org>
- In-Reply-To
- <vpqoa7asnkw.fsf@anie.imag.fr>
On 06/09/2016 11:45 AM, Matthieu Moy wrote:
Show 25 quoted lines
> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt >> index edbba3a..21776f0 100644 >> --- a/Documentation/git-send-email.txt >> +++ b/Documentation/git-send-email.txt >> @@ -84,13 +84,16 @@ See the CONFIGURATION section for 'sendemail.multiEdit'. >> the value of GIT_AUTHOR_IDENT, or GIT_COMMITTER_IDENT if that is not >> set, as returned by "git var -l". >> >> ---in-reply-to=<identifier>:: >> +--in-reply-to=<Message-Id|email_file>:: >> Make the first mail (or all the mails with `--no-thread`) appear as a >> - reply to the given Message-Id, which avoids breaking threads to >> - provide a new patch series. >> + reply to the given Message-Id (given directly by argument or via the email >> + file), which avoids breaking threads to provide a new patch series. >> The second and subsequent emails will be sent as replies according to >> the `--[no]-chain-reply-to` setting. >> + >> +Furthermore, if the argument is an email file, parse it and populate header >> +fields appropriately for the reply. > > "populate header fields appropriately" would seem obscure to someone not > having followed this converation. At least s/fields/To: and Cc: fields/.
We weren't sure how precise the documentation had to be, and tried to keep it concise.
Show 12 quoted lines
>> --- a/git-send-email.perl >> +++ b/git-send-email.perl >> @@ -55,6 +55,7 @@ git send-email --dump-aliases >> --[no-]bcc <str> * Email Bcc: >> --subject <str> * Email "Subject:" >> --in-reply-to <str> * Email "In-Reply-To:" >> + --in-reply-to <file> * Populate header fields appropriately. > > Likewise. To avoid an overly long line, I'd write just "Populate > To/Cc/In-reply-to". > > Probably <file> should be <email_file>.
Thanks, will do in v5.
Show 18 quoted lines
>> +if ($initial_reply_to && -f $initial_reply_to) {
>> + my $error = validate_patch($initial_reply_to);
>> + die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n"
>> + if $error;
>> +
>> + open my $fh, "<", $initial_reply_to or die "can't open file $initial_reply_to";
>> + my $mail = Git::parse_email($fh);
>> + close $fh;
>> +
>> + my $initial_sender = $sender || $repoauthor || $repocommitter || '';
>
> This is duplicated from the "if ($compose) { ... my $tpl_sender = ..." a
> bit later in the existing file. It would be better to get this "my
> $initial_sender = ..." out of your "if" and use $initial_sender directly
> later IMHO.
>
> Actually, $initial_sender does not seem to be a good variable name. It's
> not really "initial", right?$sender looks like a better name, I will work on that, thanks!
Show 15 quoted lines
>> + my $prefix_re = "";
>> + my $subject_re = $mail->{"subject"}[0];
>> + if ($subject_re =~ /^[^Re:]/) {
>> + $prefix_re = "Re: ";
>> + }
>> + $initial_subject = $prefix_re . $subject_re;
>
> Why introduce $prefix_re. You can just
>
> my $subject = $mail->{"subject"}[0];
> if (...) {
> $subject = "Re: " . $subject;
> }
>
> (preferably using sensible as '...' as noted by Junio ;-) ).I will keep Junio's suggestion :-)
> In previous iterations of this series, you had issues with non-ascii > characters in at least To: and Cc: fields (perhaps in the Subject field > too?). Are they solved? I don't see any tests about it ...
Non-ascii characters are still an issue, I will work on that next week.