From: Samuel GROOT <samuel.groot@grenoble-inp.org>
Date: Tue, 14 Jun 2016 22:35:15 GMT
Subject: Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields
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:
> 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.

>> --- 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.

>> +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!

>> +	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.