On 05/28/16 16:35, Matthieu Moy wrote:
>> +--quote-email=<email_file>::
>> + Reply to the given email and automatically populate the "To:",
"Cc:" and
>> + "In-Reply-To:" fields.
>
> I think this is a bit too technical for a user documentation. To: and
> Cc: is OK, but people need not know about "In-Reply-To:" to understand
> this. See what the doc of --in-reply-to says. If you want to be
> technical, you'd need to mention the References: field too.
You have a point here. Maybe, we can explain that the `--quote-email`
option behaves like a mailer when replying to someone without getting
into details.
> Talking about Reference: field, something your patch could do is to add
> all references in <email_file> to the references of the new email (see
> what a mailer is doing when replying). This way, the recipient can still
> get threading if the last message being replied-to is missing.
I didn't know about this field, it looks like it appends all the
parent message ID's.
>> +"Re: [<email_file>'s subject]".
>
> Perhaps `Re: ...` instead of double-quotes.
Agreed.
>> +if ($quote_email) {
>> + my $error = validate_patch($quote_email);
>> + $error and die "fatal: $quote_email: $error\nwarning: no patches
were sent\n";
>
> I know it's done this way elsewhere, but I don't like this "$error and
> die", I'd rather see a proper if here.
You're right, I'll change that in the next version.
>> + if (defined $input_format && $input_format eq 'mbox') {
>
> To me, the input format refers to patch files, not the <email_file>.
>
> I'm not sure anyone still use the "lots of email" format, and you are
> not testing it. So, this is claiming that we have a feature without
> being sure we have it nor that anyone's ever going to use it.
You summed up the situation well.
> I'd just drop this "if" and the "else" branch, and just assume the email
> file is a normal email file.
I'll do that.
Thank you for the review.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html