From: Tom Russello <tom.russello@grenoble-inp.org>
Date: Sun, 29 May 2016 23:38:02 GMT
Subject: Re: [RFC-PATCH v2 1/2] send-email: quote-email populates the fields
Message-ID: <16f3b6f8-0a4c-3102-8723-e4dfacc51906@grenoble-inp.org>
In-Reply-To: <vpqk2iel081.fsf@anie.imag.fr>

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.