Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body

[Tom Russello <tom.russello@grenoble-inp.org>]

Le 09/06/2016 à 13:49, Matthieu Moy a écrit :
> Samuel GROOT <samuel.groot@grenoble-inp.org> writes:
> 
>> If used with `in-reply-to=<email_file>`, cite the message body of the given
>> email file. Otherwise, do nothing.
> 
> It should at least warn when --in-reply-to=<email_file> is not given
> (either no --in-reply-to or --in-reply-to=<id>). I don't see any
> use-case where a user would want --cite on the command-line and not want
> --in-reply-to=<email_file>. OTOH, it seems a plausible user-error, and
> the user would appreciate a message saying what's going on.

You're right. We'll warn the user with the next version.

>> @@ -56,6 +57,8 @@ git send-email --dump-aliases
>>      --subject               <str>  * Email "Subject:"
>>      --in-reply-to           <str>  * Email "In-Reply-To:"
>>      --in-reply-to          <file>  * Populate header fields appropriately.
>> +    --cite                         * Quote the message body in the cover if
>> +                                     --compose is set, else in the first patch.
>>      --[no-]xmailer                 * Add "X-Mailer:" header (default).
>>      --[no-]annotate                * Review each patch that will be sent in an editor.
>>      --compose                      * Open an editor for introduction.
> 
> Just wondering: would it make sense to activate --cite by default when
> --in-reply-to=file is used, and to allow --no-cite to disable this?
> This is something we can easily do now without breaking backward
> compatibility (--in-reply-to=file doesn't exist yet), but would be more
> painful to do later.

Indeed, it can be more intuitive especially for a user who is used to
cite messages.

>> @@ -640,6 +644,7 @@ if (@files) {
>>  	usage();
>>  }
>>  
>> +my $message_cited;
> 
> Nit: I read "$message_cited" as "Boolean saying whether the message was
> cited". $cited_message would be clearer to me (but this is to be taken
> with a grain of salt as I'm not a native speaker), since the variable
> holds the content of the cited message.

Sorry, French habits.. :-)

>> +sub do_insert_cited_message {
>> +	my $tmp_file = shift;
>> +	my $original_file = shift;
>> +
>> +	open my $c, "<", $original_file
>> +	or die "Failed to open $original_file: " . $!;
>> +
>> +	open my $c2, ">", $tmp_file
>> +		or die "Failed to open $tmp_file: " . $!;
>> +
>> +	# Insertion after the triple-dash
>> +	while (<$c>) {
>> +		print $c2 $_;
>> +		last if (/^---$/);
>> +	}
>> +	print $c2 $message_cited;
> 
> I would add a newline here to get a blank line between the message cited
> and the diffstat.

Agreed.

> I think non-ascii characters would deserve particular attention here
> too. For example, if the patch contain only ascii and the cited part
> contains UTF-8, does the generated patch have a proper Content-type:
> header?
> 
> I can imagine worse, like a patch containing latin1 character and a
> cited message with another 8-bit encoding.

I tried to manage them with the built-in Base64 module but there is
still work in progress.

>> +test_expect_success $PREREQ 'correct cited message with --in-reply-to and --compose' '
>> +	grep "> On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt3 &&
> 
> I would prefer to have the full address including the real name here (A
> <author@example.com>) in this example. Actually, after a quick look at
> the code, I don't understand where the name has gone (what's shown here
> is extracted from the From: header).

Agreed, I'll figure out where the problem is.