Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body
Le 09/06/2016 à 13:49, Matthieu Moy a écrit :
> 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.
You're right. We'll warn the user with the next version.
>> @@ -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.
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, aut...@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).
Agreed, I'll figure out where the problem is.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body
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, aut...@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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body
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.
> @@ -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.
> @@ -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.
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, aut...@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).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 6/6] send-email: add option --cite to quote the message body
If used with `in-reply-to=`, cite the message body of the given
email file. Otherwise, do nothing.
If `--compose` is also set, quote the message body in the cover letter. Else,
imply `--annotate` by default and quote the message body below the triple-dash
section in the first patch only.
Signed-off-by: Tom RUSSELLO
Signed-off-by: Samuel GROOT
Signed-off-by: Matthieu MOY
---
Documentation/git-send-email.txt | 8
git-send-email.perl | 81 ++--
t/t9001-send-email.sh| 32
3 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 21776f0..23cbd17 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -109,6 +109,14 @@ illustration below where `[PATCH v2 0/3]` is in reply to
`[PATCH 0/2]`:
Only necessary if --compose is also set. If --compose
is not set, this will be prompted for.
+--cite::
+ When used with `--in-reply-to=`, quote the message body of
the
+ given email file.
++
+If `--compose` is also set, the message cited will be in the cover letter. If
+`--compose` is not set, `--annotate` option is implied by default and the
+message body will be cited in the "below-triple-dash" section.
+
--subject=::
Specify the initial subject of the email thread.
Only necessary if --compose is also set. If --compose
diff --git a/git-send-email.perl b/git-send-email.perl
index b444ea6..6877ea7 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -26,6 +26,7 @@ use Text::ParseWords;
use Term::ANSIColor;
use File::Temp qw/ tempdir tempfile /;
use File::Spec::Functions qw(catfile);
+use File::Copy;
use Error qw(:try);
use Git;
@@ -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.
@@ -161,7 +164,7 @@ my $re_encoded_word =
qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/;
# Variables we fill in automatically, or via prompting:
my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,
- $initial_reply_to,$initial_references,$initial_subject,@files,
+ $initial_reply_to,$initial_references,$cite,$initial_subject,@files,
$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
my $envelope_sender;
@@ -305,6 +308,7 @@ $rc = GetOptions(
"sender|from=s" => \$sender,
"in-reply-to=s" => \$initial_reply_to,
"subject=s" => \$initial_subject,
+ "cite" => \$cite,
"to=s" => \@initial_to,
"to-cmd=s" => \$to_cmd,
"no-to" => \$no_to,
@@ -640,6 +644,7 @@ if (@files) {
usage();
}
+my $message_cited;
if ($initial_reply_to && -f $initial_reply_to) {
my $error = validate_patch($initial_reply_to);
die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n"
@@ -658,7 +663,8 @@ if ($initial_reply_to && -f $initial_reply_to) {
}
$initial_subject = $prefix_re . $subject_re;
- push @initial_to, $mail->{"from"}[0];
+ my $recipient = $mail->{"from"}[0];
+ push @initial_to, $recipient;
foreach my $to_addr (parse_address_line(join ",", @{$mail->{"to"}})) {
if (!($to_addr eq $initial_sender)) {
@@ -684,6 +690,25 @@ if ($initial_reply_to && -f $initial_reply_to) {
$initial_references = join("", @{$mail->{"references"}}) .
" " . $initial_reply_to;
}
+
+ if ($cite) {
+ my $date = $mail->{"date"}[0];
+ my $tpl_date = $date && "On $date, " || '';
+ $message_cited = $tpl_date . $recipient . " wrote:\n";
+
+ # Quote the message body
+ foreach (@{$mail->{"body"}}) {
+ my $space = "";
+ if (/^[^>]/) {
+ $space = " ";
+ }
+ $message_cited .= ">" . $space . $_;
+ }
+
+ if (!$compose) {
+ $annotate = 1;
+ }
+ }
}
sub get_patch_subject {
@@ -711,6 +736,9 @@ if ($compose) {
my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
my $tpl_subject = $initial_subject || '';
my $tpl_reply_to = $initial_reply_to || '';
+ my $tpl_quote
