Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 9, 2016, 11:49 UTC
- Message-ID
- <vpqvb1ir38m.fsf@anie.imag.fr>
- In-Reply-To
- <20160608130833.32274-1-samuel.groot@grenoble-inp.org>
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.
Show 9 quoted lines
> @@ -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.
Show 5 quoted lines
> @@ -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.
Show 16 quoted lines
> +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.
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.
> +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).
-- Matthieu Moy http://www-verimag.imag.fr/~moy/