From: Samuel GROOT Date: Tue, 14 Jun 2016 22:53:36 GMT Subject: Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body Message-ID: In-Reply-To: On 06/09/2016 01:49 PM, Matthieu Moy wrote: > Samuel GROOT writes: > >> If used with `in-reply-to=`, cite the message body of the given >> email file. Otherwise, do nothing. > > It should at least warn when --in-reply-to= is not given > (either no --in-reply-to or --in-reply-to=). I don't see any > use-case where a user would want --cite on the command-line and not want > --in-reply-to=. OTOH, it seems a plausible user-error, and > the user would appreciate a message saying what's going on. We weren't sure how to warn the user. If --in-reply-to is not an mail file, should we check it with a regex to make sure it's a message-id? >> @@ -56,6 +57,8 @@ git send-email --dump-aliases >> --subject * Email "Subject:" >> --in-reply-to * Email "In-Reply-To:" >> --in-reply-to * 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. It's an interesting question. IMHO we should have `--cite` by default and allow `--no-cite` to disable quoting the message body, because it's easier to remove extra unwanted lines than copying lines from another file and adding "> " before each line. >> @@ -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. > >> +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. A newline here makes it easier to distinguish the different sections in the email file indeed. > 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? Non-ascii characters are still an issue, I'll work on that next week. > 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 > ) 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). Yep, after a quick look at the code involved, I don't understand either, I will investigate this week.