{"thread":{"id":"48400","subject":"[PATCH] git-send-email: allow re-editing of message","startedAt":"2018-05-02T14:32:20Z","lastAt":"2018-05-04T08:19:41Z","messageCount":2,"participants":["Drew DeVault","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"346453","messageId":"20180502143200.9371-1-sir@cmpwn.com","threadId":"48400","inReplyTo":null,"subject":"[PATCH] git-send-email: allow re-editing of message","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2018-05-02T14:32:00Z","receivedAt":"2018-05-02T14:32:20Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"When shown the email summary, an opportunity is presented for the user\nto edit the email as if they had specified --annotate. This also permits\nthem to edit it multiple times.\n\nSigned-off-by: Drew DeVault <sir@cmpwn.com>\nReviewed-by: Simon Ser <contact@emersion.fr>\n\n---\nSent to the ML without comment, sending back around with more people\nCc'd for better visibility.\n\n git-send-email.perl | 35 ++++++++++++++++++++++++++++-------\n 1 file changed, 28 insertions(+), 7 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2fa7818ca..14f2e8ae4 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1330,9 +1330,14 @@ sub file_name_is_absolute {\n \treturn File::Spec::Functions::file_name_is_absolute($path);\n }\n \n-# Returns 1 if the message was sent, and 0 otherwise.\n-# In actuality, the whole program dies when there\n-# is an error sending a message.\n+# Prepares the email, then asks the user what to do.\n+#\n+# If the user chooses to send the email, it's sent and 1 is returned.\n+# If the user chooses not to send the email, 0 is returned.\n+# If the user decides they want to make further edits, -1 is returned and the\n+# caller is expected to call send_message again after the edits are performed.\n+#\n+# If an error occurs sending the email, this just dies.\n \n sub send_message {\n \tmy @recipients = unique_email_list(@to);\n@@ -1404,15 +1409,17 @@ Message-Id: $message_id\n \n EOF\n \t\t}\n-\t\t# TRANSLATORS: Make sure to include [y] [n] [q] [a] in your\n+\t\t# TRANSLATORS: Make sure to include [y] [n] [e] [q] [a] in your\n \t\t# translation. The program will only accept English input\n \t\t# at this point.\n-\t\t$_ = ask(__(\"Send this email? ([y]es|[n]o|[q]uit|[a]ll): \"),\n-\t\t         valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,\n+\t\t$_ = ask(__(\"Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): \"),\n+\t\t         valid_re => qr/^(?:yes|y|no|n|edit|e|quit|q|all|a)/i,\n \t\t         default => $ask_default);\n \t\tdie __(\"Send this email reply required\") unless defined $_;\n \t\tif (/^n/i) {\n \t\t\treturn 0;\n+\t\t} elsif (/^e/i) {\n+\t\t\treturn -1;\n \t\t} elsif (/^q/i) {\n \t\t\tcleanup_compose_files();\n \t\t\texit(0);\n@@ -1552,7 +1559,9 @@ $references = $initial_in_reply_to || '';\n $subject = $initial_subject;\n $message_num = 0;\n \n-foreach my $t (@files) {\n+sub process_file {\n+\tmy ($t) = @_;\n+\n \topen my $fh, \"<\", $t or die sprintf(__(\"can't open file %s\"), $t);\n \n \tmy $author = undef;\n@@ -1755,6 +1764,10 @@ foreach my $t (@files) {\n \t}\n \n \tmy $message_was_sent = send_message();\n+\tif ($message_was_sent == -1) {\n+\t\tdo_edit($t);\n+\t\treturn 0;\n+\t}\n \n \t# set up for the next message\n \tif ($thread && $message_was_sent &&\n@@ -1776,6 +1789,14 @@ foreach my $t (@files) {\n \t\tundef $auth;\n \t\tsleep($relogin_delay) if defined $relogin_delay;\n \t}\n+\n+\treturn 1;\n+}\n+\n+foreach my $t (@files) {\n+\twhile (!process_file($t)) {\n+\t\t# This space deliberately left blank\n+\t}\n }\n \n # Execute a command (e.g. $to_cmd) to get a list of email addresses\n-- \n2.17.0\n\n"},{"id":"346639","messageId":"20180504081936.GA9133@dcvr","threadId":"48400","inReplyTo":"20180502143200.9371-1-sir@cmpwn.com","subject":"Re: [PATCH] git-send-email: allow re-editing of message","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2018-05-04T08:19:36Z","receivedAt":"2018-05-04T08:19:41Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Drew DeVault <sir@cmpwn.com> wrote:\n> When shown the email summary, an opportunity is presented for the user\n> to edit the email as if they had specified --annotate. This also permits\n> them to edit it multiple times.\n\nThanks, this seems like a good idea for the cover letter, especially.\nI prefer to get the commit messages right in the git history, first,\nbut I more often screw up the cover letter.\n\nI haven't looked at the code to send-email in a while,\nso some thinking out loud below :>\n\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1330,9 +1330,14 @@ sub file_name_is_absolute {\n>  \treturn File::Spec::Functions::file_name_is_absolute($path);\n>  }\n>  \n> -# Returns 1 if the message was sent, and 0 otherwise.\n> -# In actuality, the whole program dies when there\n> -# is an error sending a message.\n> +# Prepares the email, then asks the user what to do.\n> +#\n> +# If the user chooses to send the email, it's sent and 1 is returned.\n> +# If the user chooses not to send the email, 0 is returned.\n> +# If the user decides they want to make further edits, -1 is returned and the\n> +# caller is expected to call send_message again after the edits are performed.\n\nOK, -1 is the new return value.  Thanks for documenting this.\nThe rest of the prompt implementation looks fine and I won't quote it.\n\n> @@ -1552,7 +1559,9 @@ $references = $initial_in_reply_to || '';\n>  $subject = $initial_subject;\n>  $message_num = 0;\n>  \n> -foreach my $t (@files) {\n> +sub process_file {\n> +\tmy ($t) = @_;\n> +\n>  \topen my $fh, \"<\", $t or die sprintf(__(\"can't open file %s\"), $t);\n>  \n>  \tmy $author = undef;\n\nOK, process_file is a new function...\n\n> @@ -1755,6 +1764,10 @@ foreach my $t (@files) {\n>  \t}\n>  \n>  \tmy $message_was_sent = send_message();\n> +\tif ($message_was_sent == -1) {\n> +\t\tdo_edit($t);\n> +\t\treturn 0;\n> +\t}\n\nAnd the previously documented -1 return value is used here.\nMental note: process_file returns 0 to indicate an edit was done.\n\n>  \t# set up for the next message\n>  \tif ($thread && $message_was_sent &&\n> @@ -1776,6 +1789,14 @@ foreach my $t (@files) {\n>  \t\tundef $auth;\n>  \t\tsleep($relogin_delay) if defined $relogin_delay;\n>  \t}\n> +\n> +\treturn 1;\n\nMental note: process_file normally returns 1\n\n> +}\n> +\n> +foreach my $t (@files) {\n> +\twhile (!process_file($t)) {\n> +\t\t# This space deliberately left blank\n\nCute, but that comment could say something useful, instead:\n\n\t# user is still editing the file\n\n\nOtherwise, I think the patch is great.  Thanks!\n"}]}