{"thread":{"id":"59126","subject":"[PATCH v9 1/2] send-email: refactor header generation functions","startedAt":"2023-01-20T01:25:38Z","lastAt":"2023-04-19T18:05:19Z","messageCount":10,"participants":["Michael Strawbridge","Luben Tuikov","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":9,"patchTotal":2},"messages":[{"id":"470757","messageId":"20230120012459.920932-2-michael.strawbridge@amd.com","threadId":"59126","inReplyTo":"20230120012459.920932-1-michael.strawbridge@amd.com","subject":"[PATCH v9 1/2] send-email: refactor header generation functions","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-20T01:24:58Z","receivedAt":"2023-01-20T01:25:38Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"Split process_file and send_message into easier to use functions.\nMaking SMTP header information widely available.\n\nCc: Luben Tuikov \nCc: Junio C Hamano \nCc: Ævar Arnfjörð Bjarmason \nAcked-by: Luben Tuikov \nSigned-off-by: Michael Strawbridge \n---\n git-send-email.perl | 49 ++++++++++++++++++++++++++++-----------------\n 1 file changed, 31 insertions(+), 18 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5861e99a6e..42f135a266 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1495,16 +1495,7 @@ sub file_name_is_absolute {\n \treturn File::Spec::Functions::file_name_is_absolute($path);\n }\n \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+sub gen_header {\n \tmy @recipients = unique_email_list(@to);\n \t@cc = (grep { my $cc = extract_valid_address_or_die($_);\n \t\t not grep { $cc eq $_ || $_ =~ /<\\Q${cc}\\E>$/ } @recipients\n@@ -1546,6 +1537,22 @@ sub send_message {\n \tif (@xh) {\n \t\t$header .= join(\"\\n\", @xh) . \"\\n\";\n \t}\n+\tmy $recipients_ref = \\@recipients;\n+\treturn ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header);\n+}\n+\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_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();\n+\tmy @recipients = @$recipients_ref;\n \n \tmy @sendmail_parameters = ('-i', @recipients);\n \tmy $raw_from = $sender;\n@@ -1735,11 +1742,8 @@ sub send_message {\n $references = $initial_in_reply_to || '';\n $message_num = 0;\n \n-# Prepares the email, prompts the user, sends it out\n-# Returns 0 if an edit was done and the function should be called again, or 1\n-# otherwise.\n-sub process_file {\n-\tmy ($t) = @_;\n+sub pre_process_file {\n+\tmy ($t, $quiet) = @_;\n \n \topen my $fh, \"<\", $t or die sprintf(__(\"can't open file %s\"), $t);\n \n@@ -1893,9 +1897,9 @@ sub process_file {\n \t}\n \tclose $fh;\n \n-\tpush @to, recipients_cmd(\"to-cmd\", \"to\", $to_cmd, $t)\n+\tpush @to, recipients_cmd(\"to-cmd\", \"to\", $to_cmd, $t, $quiet)\n \t\tif defined $to_cmd;\n-\tpush @cc, recipients_cmd(\"cc-cmd\", \"cc\", $cc_cmd, $t)\n+\tpush @cc, recipients_cmd(\"cc-cmd\", \"cc\", $cc_cmd, $t, $quiet)\n \t\tif defined $cc_cmd && !$suppress_cc{'cccmd'};\n \n \tif ($broken_encoding{$t} && !$has_content_type) {\n@@ -1954,6 +1958,15 @@ sub process_file {\n \t\t\t@initial_to = @to;\n \t\t}\n \t}\n+}\n+\n+# Prepares the email, prompts the user, and sends it out\n+# Returns 0 if an edit was done and the function should be called again, or 1\n+# on the email being successfully sent out.\n+sub process_file {\n+\tmy ($t) = @_;\n+\n+ pre_process_file($t, $quiet);\n \n \tmy $message_was_sent = send_message();\n \tif ($message_was_sent == -1) {\n@@ -2002,7 +2015,7 @@ sub process_file {\n # Execute a command (e.g. $to_cmd) to get a list of email addresses\n # and return a results array\n sub recipients_cmd {\n-\tmy ($prefix, $what, $cmd, $file) = @_;\n+\tmy ($prefix, $what, $cmd, $file, $quiet) = @_;\n \n \tmy @addresses = ();\n \topen my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n-- \n2.34.1\n\n"},{"id":"470758","messageId":"20230120012459.920932-3-michael.strawbridge@amd.com","threadId":"59126","inReplyTo":"20230120012459.920932-1-michael.strawbridge@amd.com","subject":"[PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-20T01:24:59Z","receivedAt":"2023-01-20T01:25:42Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"To allow further flexibility in the Git hook, the SMTP header\ninformation of the email which git-send-email intends to send, is now\npassed as the 2nd argument to the sendemail-validate hook.\n\nAs an example, this can be useful for acting upon keywords in the\nsubject or specific email addresses.\n\nCc: Luben Tuikov \nCc: Junio C Hamano \nCc: Ævar Arnfjörð Bjarmason \nAcked-by: Luben Tuikov \nSigned-off-by: Michael Strawbridge \n---\n Documentation/githooks.txt | 27 ++++++++++++++++++----\n git-send-email.perl | 46 ++++++++++++++++++++++----------------\n t/t9001-send-email.sh | 27 ++++++++++++++++++++--\n 3 files changed, 75 insertions(+), 25 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex a16e62bc8c..0decbfc92d 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -583,10 +583,29 @@ processed by rebase.\n sendemail-validate\n ~~~~~~~~~~~~~~~~~~\n \n-This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,\n-the name of the file that holds the e-mail to be sent. Exiting with a\n-non-zero status causes `git send-email` to abort before sending any\n-e-mails.\n+This hook is invoked by linkgit:git-send-email[1].\n+\n+It takes these command line arguments. They are,\n+1. the name of the file which holds the contents of the email to be sent.\n+2. The name of the file which holds the SMTP headers of the email.\n+\n+The SMTP headers are passed in the exact same way as they are passed to the\n+user's Mail Transport Agent (MTA). In effect, the email given to the user's\n+MTA, is the contents of $2 followed by the contents of $1.\n+\n+Below is an example for a few common headers. Take notice of the\n+capitalization and multi-line tab structure.\n+\n+ From: Example \n+ To: to@example.com\n+ Cc: cc@example.com,\n+\t A ,\n+\t One ,\n+\t two@example.com\n+ Subject: PATCH-STRING\n+\n+Exiting with a non-zero status causes `git send-email` to abort\n+before sending any e-mails.\n \n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 42f135a266..0e595d6ac5 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -785,16 +785,31 @@ sub is_format_patch_arg {\n \tpush @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);\n }\n \n-@files = handle_backup_files(@files);\n+if (defined $sender) {\n+\t$sender =~ s/^\\s+|\\s+$//g;\n+\t($sender) = expand_aliases($sender);\n+} else {\n+\t$sender = $repoauthor->() || $repocommitter->() || '';\n+}\n+\n+# $sender could be an already sanitized address\n+# (e.g. sendemail.from could be manually sanitized by user).\n+# But it's a no-op to run sanitize_address on an already sanitized address.\n+$sender = sanitize_address($sender);\n+\n+$time = time - scalar $#files;\n \n if ($validate) {\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n+\t\t pre_process_file($f, 1);\n \t\t\tvalidate_patch($f, $target_xfer_encoding);\n \t\t}\n \t}\n }\n \n+@files = handle_backup_files(@files);\n+\n if (@files) {\n \tunless ($quiet) {\n \t\tprint $_,\"\\n\" for (@files);\n@@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {\n \t}\n }\n \n-if (defined $sender) {\n-\t$sender =~ s/^\\s+|\\s+$//g;\n-\t($sender) = expand_aliases($sender);\n-} else {\n-\t$sender = $repoauthor->() || $repocommitter->() || '';\n-}\n-\n-# $sender could be an already sanitized address\n-# (e.g. sendemail.from could be manually sanitized by user).\n-# But it's a no-op to run sanitize_address on an already sanitized address.\n-$sender = sanitize_address($sender);\n-\n my $to_whom = __(\"To whom should the emails be sent (if anyone)?\");\n my $prompting = 0;\n if (!@initial_to && !defined $to_cmd) {\n@@ -1214,10 +1217,6 @@ sub make_message_id {\n \t#print \"new message id = $message_id\\n\"; # Was useful for debugging\n }\n \n-\n-\n-$time = time - scalar $#files;\n-\n sub unquote_rfc2047 {\n \tlocal ($_) = @_;\n \tmy $charset;\n@@ -2101,11 +2100,20 @@ sub validate_patch {\n \t\t\tchdir($repo->wc_path() or $repo->repo_path())\n \t\t\t\tor die(\"chdir: $!\");\n \t\t\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n+\n+\t\t\tmy ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();\n+\n+\t\t\trequire File::Temp;\n+\t\t\tmy ($header_filehandle, $header_filename) = File::Temp::tempfile(\n+ \".gitsendemail.header.XXXXXX\", DIR => $repo->repo_path());\n+\t\t\tprint $header_filehandle $header;\n+\n \t\t\tmy @cmd = (\"git\", \"hook\", \"run\", \"--ignore-missing\",\n \t\t\t\t $hook_name, \"--\");\n-\t\t\tmy @cmd_msg = (@cmd, \"\");\n-\t\t\tmy @cmd_run = (@cmd, $target);\n+\t\t\tmy @cmd_msg = (@cmd, \"\", \"
\");\n+\t\t\tmy @cmd_run = (@cmd, $target, $header_filename);\n \t\t\t$hook_error = system_or_msg(\\@cmd_run, undef, \"@cmd_msg\");\n+\t\t\tunlink($header_filehandle);\n \t\t\tchdir($cwd_save) or die(\"chdir: $!\");\n \t\t}\n \t\tif ($hook_error) {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 1130ef21b3..8a5c111a24 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -540,7 +540,7 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate -- '\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate --
'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n@@ -559,12 +559,35 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate -- '\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate --
'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success $PREREQ \"--validate hook supports header argument\" '\n+\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n+ if test \"$#\" -ge 2\n+\tthen\n+\t\tgrep \"X-test-header: v1.0\" \"$2\"\n+\telse\n+\t\techo \"No header arg passed\"\n+\t\texit 1\n+\tfi\n+\tEOF\n+\ttest_config core.hooksPath \"my-hooks\" &&\n+\trm -fr outdir &&\n+\tgit format-patch \\\n+\t\t--add-header=\"X-test-header: v1.0\" \\\n+\t\t-n HEAD^1 -o outdir &&\n+\tgit send-email \\\n+\t\t--dry-run \\\n+\t\t--to=nobody@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--validate \\\n+\t\toutdir/000?-*.patch\n+'\n+\n for enc in 7bit 8bit quoted-printable base64\n do\n \ttest_expect_success $PREREQ \"--transfer-encoding=$enc produces correct header\" '\n-- \n2.34.1\n\n"},{"id":"470759","messageId":"20230120012459.920932-1-michael.strawbridge@amd.com","threadId":"59126","inReplyTo":null,"subject":"[PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-20T01:24:57Z","receivedAt":"2023-01-20T01:26:36Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"Thanks to Ævar for an idea to simplify these patches further.\n\nMichael Strawbridge (2):\n send-email: refactor header generation functions\n send-email: expose header information to git-send-email's\n sendemail-validate hook\n\n Documentation/githooks.txt | 27 +++++++++--\n git-send-email.perl | 95 +++++++++++++++++++++++---------------\n t/t9001-send-email.sh | 27 ++++++++++-\n 3 files changed, 106 insertions(+), 43 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"470780","messageId":"e353df62-c189-755f-5536-5ea91177c55c@amd.com","threadId":"59126","inReplyTo":"20230120012459.920932-3-michael.strawbridge@amd.com","subject":"Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Luben Tuikov","fromEmail":"luben.tuikov@amd.com","sentAt":"2023-01-20T13:07:40Z","receivedAt":"2023-01-20T13:07:50Z","isPatch":true,"sender":{"key":"luben.tuikov@amd.com","avatar":null},"body":"On 2023-01-19 20:24, Michael Strawbridge wrote:\n> To allow further flexibility in the Git hook, the SMTP header\n> information of the email which git-send-email intends to send, is now\n> passed as the 2nd argument to the sendemail-validate hook.\n> \n> As an example, this can be useful for acting upon keywords in the\n> subject or specific email addresses.\n> \n> Cc: Luben Tuikov \n> Cc: Junio C Hamano \n> Cc: Ævar Arnfjörð Bjarmason \n> Acked-by: Luben Tuikov \n> Signed-off-by: Michael Strawbridge \n> ---\n> Documentation/githooks.txt | 27 ++++++++++++++++++----\n> git-send-email.perl | 46 ++++++++++++++++++++++----------------\n> t/t9001-send-email.sh | 27 ++++++++++++++++++++--\n> 3 files changed, 75 insertions(+), 25 deletions(-)\n> \n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index a16e62bc8c..0decbfc92d 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -583,10 +583,29 @@ processed by rebase.\n> sendemail-validate\n> ~~~~~~~~~~~~~~~~~~\n> \n> -This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,\n> -the name of the file that holds the e-mail to be sent. Exiting with a\n> -non-zero status causes `git send-email` to abort before sending any\n> -e-mails.\n> +This hook is invoked by linkgit:git-send-email[1].\n> +\n> +It takes these command line arguments. They are,\n> +1. the name of the file which holds the contents of the email to be sent.\n> +2. The name of the file which holds the SMTP headers of the email.\n> +\n> +The SMTP headers are passed in the exact same way as they are passed to the\n> +user's Mail Transport Agent (MTA). In effect, the email given to the user's\n> +MTA, is the contents of $2 followed by the contents of $1.\n> +\n> +Below is an example for a few common headers. Take notice of the\n\n\"example of\" not \"for\".\n\nThis maybe clearer:\n\"An example of a few common headers is shown below. Take notice ...\"\n\n> +capitalization and multi-line tab structure.\n> +\n> + From: Example \n> + To: to@example.com\n> + Cc: cc@example.com,\n> +\t A ,\n> +\t One ,\n> +\t two@example.com\n> + Subject: PATCH-STRING\n> +\n> +Exiting with a non-zero status causes `git send-email` to abort\n> +before sending any e-mails.\n> \n> fsmonitor-watchman\n> ~~~~~~~~~~~~~~~~~~\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 42f135a266..0e595d6ac5 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -785,16 +785,31 @@ sub is_format_patch_arg {\n> \tpush @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);\n> }\n> \n> -@files = handle_backup_files(@files);\n> +if (defined $sender) {\n> +\t$sender =~ s/^\\s+|\\s+$//g;\n> +\t($sender) = expand_aliases($sender);\n> +} else {\n> +\t$sender = $repoauthor->() || $repocommitter->() || '';\n> +}\n> +\n> +# $sender could be an already sanitized address\n> +# (e.g. sendemail.from could be manually sanitized by user).\n> +# But it's a no-op to run sanitize_address on an already sanitized address.\n> +$sender = sanitize_address($sender);\n> +\n> +$time = time - scalar $#files;\n> \n> if ($validate) {\n> \tforeach my $f (@files) {\n> \t\tunless (-p $f) {\n> +\t\t pre_process_file($f, 1);\n> \t\t\tvalidate_patch($f, $target_xfer_encoding);\n> \t\t}\n> \t}\n> }\n> \n> +@files = handle_backup_files(@files);\n> +\n> if (@files) {\n> \tunless ($quiet) {\n> \t\tprint $_,\"\\n\" for (@files);\n> @@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {\n> \t}\n> }\n> \n> -if (defined $sender) {\n> -\t$sender =~ s/^\\s+|\\s+$//g;\n> -\t($sender) = expand_aliases($sender);\n> -} else {\n> -\t$sender = $repoauthor->() || $repocommitter->() || '';\n> -}\n> -\n> -# $sender could be an already sanitized address\n> -# (e.g. sendemail.from could be manually sanitized by user).\n> -# But it's a no-op to run sanitize_address on an already sanitized address.\n> -$sender = sanitize_address($sender);\n> -\n> my $to_whom = __(\"To whom should the emails be sent (if anyone)?\");\n> my $prompting = 0;\n> if (!@initial_to && !defined $to_cmd) {\n> @@ -1214,10 +1217,6 @@ sub make_message_id {\n> \t#print \"new message id = $message_id\\n\"; # Was useful for debugging\n> }\n> \n> -\n> -\n> -$time = time - scalar $#files;\n> -\n> sub unquote_rfc2047 {\n> \tlocal ($_) = @_;\n> \tmy $charset;\n> @@ -2101,11 +2100,20 @@ sub validate_patch {\n> \t\t\tchdir($repo->wc_path() or $repo->repo_path())\n> \t\t\t\tor die(\"chdir: $!\");\n> \t\t\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n> +\n> +\t\t\tmy ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();\n> +\n> +\t\t\trequire File::Temp;\n> +\t\t\tmy ($header_filehandle, $header_filename) = File::Temp::tempfile(\n> + \".gitsendemail.header.XXXXXX\", DIR => $repo->repo_path());\n> +\t\t\tprint $header_filehandle $header;\n> +\n> \t\t\tmy @cmd = (\"git\", \"hook\", \"run\", \"--ignore-missing\",\n> \t\t\t\t $hook_name, \"--\");\n> -\t\t\tmy @cmd_msg = (@cmd, \"\");\n> -\t\t\tmy @cmd_run = (@cmd, $target);\n> +\t\t\tmy @cmd_msg = (@cmd, \"\", \"
\");\n> +\t\t\tmy @cmd_run = (@cmd, $target, $header_filename);\n> \t\t\t$hook_error = system_or_msg(\\@cmd_run, undef, \"@cmd_msg\");\n> +\t\t\tunlink($header_filehandle);\n> \t\t\tchdir($cwd_save) or die(\"chdir: $!\");\n> \t\t}\n> \t\tif ($hook_error) {\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 1130ef21b3..8a5c111a24 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -540,7 +540,7 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n> \ttest_path_is_file my-hooks.ran &&\n> \tcat >expect <<-EOF &&\n> \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate -- '\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate --
'\"'\"' died with exit code 1\n> \twarning: no patches were sent\n> \tEOF\n> \ttest_cmp expect actual\n> @@ -559,12 +559,35 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n> \ttest_path_is_file my-hooks.ran &&\n> \tcat >expect <<-EOF &&\n> \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate -- '\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate --
'\"'\"' died with exit code 1\n> \twarning: no patches were sent\n> \tEOF\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success $PREREQ \"--validate hook supports header argument\" '\n> +\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n> + if test \"$#\" -ge 2\n> +\tthen\n\nThere appears to be an extra indentation of the \"if\" statement.\n-- \nRegards,\nLuben\n\n"},{"id":"470781","messageId":"c1ba0a28-3c39-b313-2757-dceb02930334@amd.com","threadId":"59126","inReplyTo":"e353df62-c189-755f-5536-5ea91177c55c@amd.com","subject":"Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-20T14:25:49Z","receivedAt":"2023-01-20T14:27:08Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\nOn 2023-01-20 08:07, Luben Tuikov wrote:\n> On 2023-01-19 20:24, Michael Strawbridge wrote:\n>> To allow further flexibility in the Git hook, the SMTP header\n>> information of the email which git-send-email intends to send, is now\n>> passed as the 2nd argument to the sendemail-validate hook.\n>>\n>> As an example, this can be useful for acting upon keywords in the\n>> subject or specific email addresses.\n>>\n>> Cc: Luben Tuikov \n>> Cc: Junio C Hamano \n>> Cc: Ævar Arnfjörð Bjarmason \n>> Acked-by: Luben Tuikov \n>> Signed-off-by: Michael Strawbridge \n>> ---\n>> Documentation/githooks.txt | 27 ++++++++++++++++++----\n>> git-send-email.perl | 46 ++++++++++++++++++++++----------------\n>> t/t9001-send-email.sh | 27 ++++++++++++++++++++--\n>> 3 files changed, 75 insertions(+), 25 deletions(-)\n>>\n>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n>> index a16e62bc8c..0decbfc92d 100644\n>> --- a/Documentation/githooks.txt\n>> +++ b/Documentation/githooks.txt\n>> @@ -583,10 +583,29 @@ processed by rebase.\n>> sendemail-validate\n>> ~~~~~~~~~~~~~~~~~~\n>> \n>> -This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,\n>> -the name of the file that holds the e-mail to be sent. Exiting with a\n>> -non-zero status causes `git send-email` to abort before sending any\n>> -e-mails.\n>> +This hook is invoked by linkgit:git-send-email[1].\n>> +\n>> +It takes these command line arguments. They are,\n>> +1. the name of the file which holds the contents of the email to be sent.\n>> +2. The name of the file which holds the SMTP headers of the email.\n>> +\n>> +The SMTP headers are passed in the exact same way as they are passed to the\n>> +user's Mail Transport Agent (MTA). In effect, the email given to the user's\n>> +MTA, is the contents of $2 followed by the contents of $1.\n>> +\n>> +Below is an example for a few common headers. Take notice of the\n> \"example of\" not \"for\".\n>\n> This maybe clearer:\n> \"An example of a few common headers is shown below. Take notice ...\"\n\nGood idea - I've fixed it locally.\n\n>> +capitalization and multi-line tab structure.\n>> +\n>> + From: Example \n>> + To: to@example.com\n>> + Cc: cc@example.com,\n>> +\t A ,\n>> +\t One ,\n>> +\t two@example.com\n>> + Subject: PATCH-STRING\n>> +\n>> +Exiting with a non-zero status causes `git send-email` to abort\n>> +before sending any e-mails.\n>> \n>> fsmonitor-watchman\n>> ~~~~~~~~~~~~~~~~~~\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 42f135a266..0e595d6ac5 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -785,16 +785,31 @@ sub is_format_patch_arg {\n>> \tpush @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);\n>> }\n>> \n>> -@files = handle_backup_files(@files);\n>> +if (defined $sender) {\n>> +\t$sender =~ s/^\\s+|\\s+$//g;\n>> +\t($sender) = expand_aliases($sender);\n>> +} else {\n>> +\t$sender = $repoauthor->() || $repocommitter->() || '';\n>> +}\n>> +\n>> +# $sender could be an already sanitized address\n>> +# (e.g. sendemail.from could be manually sanitized by user).\n>> +# But it's a no-op to run sanitize_address on an already sanitized address.\n>> +$sender = sanitize_address($sender);\n>> +\n>> +$time = time - scalar $#files;\n>> \n>> if ($validate) {\n>> \tforeach my $f (@files) {\n>> \t\tunless (-p $f) {\n>> +\t\t pre_process_file($f, 1);\n>> \t\t\tvalidate_patch($f, $target_xfer_encoding);\n>> \t\t}\n>> \t}\n>> }\n>> \n>> +@files = handle_backup_files(@files);\n>> +\n>> if (@files) {\n>> \tunless ($quiet) {\n>> \t\tprint $_,\"\\n\" for (@files);\n>> @@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {\n>> \t}\n>> }\n>> \n>> -if (defined $sender) {\n>> -\t$sender =~ s/^\\s+|\\s+$//g;\n>> -\t($sender) = expand_aliases($sender);\n>> -} else {\n>> -\t$sender = $repoauthor->() || $repocommitter->() || '';\n>> -}\n>> -\n>> -# $sender could be an already sanitized address\n>> -# (e.g. sendemail.from could be manually sanitized by user).\n>> -# But it's a no-op to run sanitize_address on an already sanitized address.\n>> -$sender = sanitize_address($sender);\n>> -\n>> my $to_whom = __(\"To whom should the emails be sent (if anyone)?\");\n>> my $prompting = 0;\n>> if (!@initial_to && !defined $to_cmd) {\n>> @@ -1214,10 +1217,6 @@ sub make_message_id {\n>> \t#print \"new message id = $message_id\\n\"; # Was useful for debugging\n>> }\n>> \n>> -\n>> -\n>> -$time = time - scalar $#files;\n>> -\n>> sub unquote_rfc2047 {\n>> \tlocal ($_) = @_;\n>> \tmy $charset;\n>> @@ -2101,11 +2100,20 @@ sub validate_patch {\n>> \t\t\tchdir($repo->wc_path() or $repo->repo_path())\n>> \t\t\t\tor die(\"chdir: $!\");\n>> \t\t\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n>> +\n>> +\t\t\tmy ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();\n>> +\n>> +\t\t\trequire File::Temp;\n>> +\t\t\tmy ($header_filehandle, $header_filename) = File::Temp::tempfile(\n>> + \".gitsendemail.header.XXXXXX\", DIR => $repo->repo_path());\n>> +\t\t\tprint $header_filehandle $header;\n>> +\n>> \t\t\tmy @cmd = (\"git\", \"hook\", \"run\", \"--ignore-missing\",\n>> \t\t\t\t $hook_name, \"--\");\n>> -\t\t\tmy @cmd_msg = (@cmd, \"\");\n>> -\t\t\tmy @cmd_run = (@cmd, $target);\n>> +\t\t\tmy @cmd_msg = (@cmd, \"\", \"
\");\n>> +\t\t\tmy @cmd_run = (@cmd, $target, $header_filename);\n>> \t\t\t$hook_error = system_or_msg(\\@cmd_run, undef, \"@cmd_msg\");\n>> +\t\t\tunlink($header_filehandle);\n>> \t\t\tchdir($cwd_save) or die(\"chdir: $!\");\n>> \t\t}\n>> \t\tif ($hook_error) {\n>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n>> index 1130ef21b3..8a5c111a24 100755\n>> --- a/t/t9001-send-email.sh\n>> +++ b/t/t9001-send-email.sh\n>> @@ -540,7 +540,7 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n>> \ttest_path_is_file my-hooks.ran &&\n>> \tcat >expect <<-EOF &&\n>> \tfatal: longline.patch: rejected by sendemail-validate hook\n>> -\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate -- '\"'\"' died with exit code 1\n>> +\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate --
'\"'\"' died with exit code 1\n>> \twarning: no patches were sent\n>> \tEOF\n>> \ttest_cmp expect actual\n>> @@ -559,12 +559,35 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n>> \ttest_path_is_file my-hooks.ran &&\n>> \tcat >expect <<-EOF &&\n>> \tfatal: longline.patch: rejected by sendemail-validate hook\n>> -\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate -- '\"'\"' died with exit code 1\n>> +\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate --
'\"'\"' died with exit code 1\n>> \twarning: no patches were sent\n>> \tEOF\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_success $PREREQ \"--validate hook supports header argument\" '\n>> +\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n>> + if test \"$#\" -ge 2\n>> +\tthen\n> There appears to be an extra indentation of the \"if\" statement.\nGood catch.  It was a matter of spaces and tabs combining that wasn't\neasy to see.\n"},{"id":"470909","messageId":"230123.86wn5ds602.gmgdl@evledraar.gmail.com","threadId":"59126","inReplyTo":"20230120012459.920932-1-michael.strawbridge@amd.com","subject":"Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T13:51:28Z","receivedAt":"2023-01-23T14:03:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 19 2023, Michael Strawbridge wrote:\n\n> Thanks to Ævar for an idea to simplify these patches further.\n>\n> Michael Strawbridge (2):\n> send-email: refactor header generation functions\n> send-email: expose header information to git-send-email's\n> sendemail-validate hook\n>\n> Documentation/githooks.txt | 27 +++++++++--\n> git-send-email.perl | 95 +++++++++++++++++++++++---------------\n> t/t9001-send-email.sh | 27 ++++++++++-\n> 3 files changed, 106 insertions(+), 43 deletions(-)\n\nThanks for the update. Aside from any quibbles, I still have some\nfundimental concerns about the implementation here:\n\n * Other hooks take stdin, not this sort of file argument.\n\n We discussed that ending in\n https://public-inbox.org/git/20230117215811.78313-1-michael.strawbridge@amd.com/;\n but I probably shouldn't have mentioned \"git hook\" at all.\n\n I do think though that we shouldn't expose a UX discrepancy like this\n forever, but the ways forward out of that would seem to be to either\n to revert a7555304546 (send-email: use 'git hook run' for\n 'sendemail-validate', 2021-12-22) & move forward from there, or to\n wait for those patches (which I'm currentnly CI-ing).\n\n * Aside from that, shouldn't we have a new \"validate-headers\" or\n whatever hook, instead of assuming that we can add another argument\n to existing users?...\n\n * ...except can we do it safely? Now, it seems to me like you have\n potential correctness issues here. We call format_2822_time() to make\n the headers, but that's based on \"$time\", which we save away earlier.\n\n But for the rest (e.g. \"Message-Id\" are we sure that we're giving the\n hook the same headers as the one we actually end up sending?\n\n But regardless of that, something that would bypass this entire\n stdin/potential correctness etc. problem is if we just pass an offset\n to the the, i.e. currently we have a \"validate\" which gets the\n contents, if we had a \"validate-raw\" or whatever we could just pass:\n\n\t\n\t\\n\\n\n\t\n\n Where the current \"validate\" just gets \"content\", no? We could then\n either pass the offset to the \"\\n\\n\", or just trust that such a hook\n knows to find the \"\\n\\n\".\n\n I also think that would be more generally usable, as the tiny\n addition of some exit code interpretation would allow us to say \"I\n got this, and consider this sent\", which would also satisfy some who\n have wanted e.g. a way to intrecept it before it invokes \"sendmail\"\n (I remember a recent thread about that in relation to using \"mutt\" to\n send it directly)\n\n \n"},{"id":"470927","messageId":"5758ffc7-eb8c-4c16-d226-dd882cb2406b@amd.com","threadId":"59126","inReplyTo":"230123.86wn5ds602.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-23T16:03:37Z","receivedAt":"2023-01-23T16:03:46Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\nOn 2023-01-23 08:51, Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Jan 19 2023, Michael Strawbridge wrote:\n>\n>> Thanks to Ævar for an idea to simplify these patches further.\n>>\n>> Michael Strawbridge (2):\n>> send-email: refactor header generation functions\n>> send-email: expose header information to git-send-email's\n>> sendemail-validate hook\n>>\n>> Documentation/githooks.txt | 27 +++++++++--\n>> git-send-email.perl | 95 +++++++++++++++++++++++---------------\n>> t/t9001-send-email.sh | 27 ++++++++++-\n>> 3 files changed, 106 insertions(+), 43 deletions(-)\n> Thanks for the update. Aside from any quibbles, I still have some\n> fundimental concerns about the implementation here:\n>\n> * Other hooks take stdin, not this sort of file argument.\n>\n> We discussed that ending in\n> https://public-inbox.org/git/20230117215811.78313-1-michael.strawbridge@amd.com/;\n> but I probably shouldn't have mentioned \"git hook\" at all.\n>\n> I do think though that we shouldn't expose a UX discrepancy like this\n> forever, but the ways forward out of that would seem to be to either\n> to revert a7555304546 (send-email: use 'git hook run' for\n> 'sendemail-validate', 2021-12-22) & move forward from there, or to\n> wait for those patches (which I'm currentnly CI-ing).\n\nOk.  If we are at the point where the change is just trying to pass CI\nbut the main logic is there I am willing to wait some time.\n\n>\n> * Aside from that, shouldn't we have a new \"validate-headers\" or\n> whatever hook, instead of assuming that we can add another argument\n> to existing users?...\n\nWhile it's true we could (and I don't have a super strong opinion here),\nI suppose I was foreseeing the potential that a user may want to have\nlogic that requires both the email headers and contents.  For example,\nonly checking contents for a specific mailing list.  If we split the\nhooks, a user would then need to figure out how to have them coordinate.\n\n>\n> * ...except can we do it safely? Now, it seems to me like you have\n> potential correctness issues here. We call format_2822_time() to make\n> the headers, but that's based on \"$time\", which we save away earlier.\n>\n> But for the rest (e.g. \"Message-Id\" are we sure that we're giving the\n> hook the same headers as the one we actually end up sending?\n>\n> But regardless of that, something that would bypass this entire\n> stdin/potential correctness etc. problem is if we just pass an offset\n> to the the, i.e. currently we have a \"validate\" which gets the\n> contents, if we had a \"validate-raw\" or whatever we could just pass:\n\nI think there might be a part missing here: \"problem is if we just pass\nan offset to the ___.\"  So there's a chance I may not fully grasp your\nsuggestion.\n\n> \t\n> \t\\n\\n\n> \t\n>\n> Where the current \"validate\" just gets \"content\", no? We could then\n> either pass the offset to the \"\\n\\n\", or just trust that such a hook\n> knows to find the \"\\n\\n\".\n>\n> I also think that would be more generally usable, as the tiny\n> addition of some exit code interpretation would allow us to say \"I\n> got this, and consider this sent\", which would also satisfy some who\n> have wanted e.g. a way to intrecept it before it invokes \"sendmail\"\n> (I remember a recent thread about that in relation to using \"mutt\" to\n> send it directly)\n>\n> \n\nAre you suggesting to simply add the header to the current\nsendemail-validate hook?\n\nI appreciate the feedback.\n\n"},{"id":"471136","messageId":"230130.86bkmgs37x.gmgdl@evledraar.gmail.com","threadId":"59126","inReplyTo":"5758ffc7-eb8c-4c16-d226-dd882cb2406b@amd.com","subject":"Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-30T10:40:14Z","receivedAt":"2023-01-30T10:53:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 23 2023, Michael Strawbridge wrote:\n\n> On 2023-01-23 08:51, Ævar Arnfjörð Bjarmason wrote:\n * Aside from that, shouldn't we have a new \"validate-headers\" or\n>> whatever hook, instead of assuming that we can add another argument\n>> to existing users?...\n>\n> While it's true we could (and I don't have a super strong opinion here),\n> I suppose I was foreseeing the potential that a user may want to have\n> logic that requires both the email headers and contents.  For example,\n> only checking contents for a specific mailing list.  If we split the\n> hooks, a user would then need to figure out how to have them coordinate.\n\n...\n\n>>\n>> * ...except can we do it safely? Now, it seems to me like you have\n>> potential correctness issues here. We call format_2822_time() to make\n>> the headers, but that's based on \"$time\", which we save away earlier.\n>>\n>> But for the rest (e.g. \"Message-Id\" are we sure that we're giving the\n>> hook the same headers as the one we actually end up sending?\n>>\n>> But regardless of that, something that would bypass this entire\n>> stdin/potential correctness etc. problem is if we just pass an offset\n>> to the the, i.e. currently we have a \"validate\" which gets the\n>> contents, if we had a \"validate-raw\" or whatever we could just pass:\n>\n> I think there might be a part missing here: \"problem is if we just pass\n> an offset to the ___.\"  So there's a chance I may not fully grasp your\n> suggestion.\n\nSorry, a byte offset into the file to indicate the boundary between the\nheaders and the content.\n\n>\n>> \t\n>> \t\\n\\n\n>> \t\n>>\n>> Where the current \"validate\" just gets \"content\", no? We could then\n>> either pass the offset to the \"\\n\\n\", or just trust that such a hook\n>> knows to find the \"\\n\\n\".\n>>\n>> I also think that would be more generally usable, as the tiny\n>> addition of some exit code interpretation would allow us to say \"I\n>> got this, and consider this sent\", which would also satisfy some who\n>> have wanted e.g. a way to intrecept it before it invokes \"sendmail\"\n>> (I remember a recent thread about that in relation to using \"mutt\" to\n>> send it directly)\n>>\n>> \n>\n> Are you suggesting to simply add the header to the current\n> sendemail-validate hook?\n\nNo, I'm saying that we currently don't pass them at all, and your patch\nadds another argument to a file with the headers.\n\nThat *may* break some existing users if they're only expecting the\ncurrent argument(s) (although that's probably unlikely), more\nimportantly we're now doing extra work for all existing hook users, for\nthe benefit of only some new users.\n\nSo I'm suggesting having some opt-in mechanism for the new semantics,\nboth to preserve the existing semantics for existing users, and for\ncurrent and new users avoid writing out the file etc. when we don't need\nto.\n\nWhich we could do with a config variable,\ne.g. hooks.\"sendemail-validate\".includeHeaders=true, or just by having a\nnew \"sendemail-validate-raw\" (or whatever we'd call it).\n\nI think it's fine to enforce that if such a new hook exists we'd take it\nover the \"sendemail-validate\" (if any), i.e. we wouldn't need to support\nboth.\n\n"},{"id":"475706","messageId":"xmqqttxb956n.fsf@gitster.g","threadId":"59126","inReplyTo":"c1ba0a28-3c39-b313-2757-dceb02930334@amd.com","subject":"Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-19T17:01:52Z","receivedAt":"2023-04-19T17:01:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Strawbridge writes:\n\n>>> +Below is an example for a few common headers. Take notice of the\n>> \"example of\" not \"for\".\n>>\n>> This maybe clearer:\n>> \"An example of a few common headers is shown below. Take notice ...\"\n> ...\n>>> +test_expect_success $PREREQ \"--validate hook supports header argument\" '\n>>> +\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n>>> + if test \"$#\" -ge 2\n>>> +\tthen\n>> There appears to be an extra indentation of the \"if\" statement.\n> Good catch. It was a matter of spaces and tabs combining that wasn't\n> easy to see.\n\nI was reading the list of stalled topics in the periodical \"What's\ncooking\" report and noticed that this topic has been marked as\n\"Expecting a hopefully minor and final reroll.\" for full three\nmonths after we saw this message. Should we be waiting more?\n\nThanks.\n"},{"id":"475708","messageId":"668c657a-34e1-fa70-e448-5c297fc506d4@amd.com","threadId":"59126","inReplyTo":"xmqqttxb956n.fsf@gitster.g","subject":"Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook","fromName":"Luben Tuikov","fromEmail":"luben.tuikov@amd.com","sentAt":"2023-04-19T18:05:09Z","receivedAt":"2023-04-19T18:05:19Z","isPatch":true,"sender":{"key":"luben.tuikov@amd.com","avatar":null},"body":"On 2023-04-19 13:01, Junio C Hamano wrote:\n> Michael Strawbridge writes:\n> \n>>>> +Below is an example for a few common headers. Take notice of the\n>>> \"example of\" not \"for\".\n>>>\n>>> This maybe clearer:\n>>> \"An example of a few common headers is shown below. Take notice ...\"\n>> ...\n>>>> +test_expect_success $PREREQ \"--validate hook supports header argument\" '\n>>>> +\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n>>>> + if test \"$#\" -ge 2\n>>>> +\tthen\n>>> There appears to be an extra indentation of the \"if\" statement.\n>> Good catch. It was a matter of spaces and tabs combining that wasn't\n>> easy to see.\n> \n> I was reading the list of stalled topics in the periodical \"What's\n> cooking\" report and noticed that this topic has been marked as\n> \"Expecting a hopefully minor and final reroll.\" for full three\n> months after we saw this message. Should we be waiting more?\n> \n> Thanks.\n\nThanks Junio for the reminder--I was wondering the same thing not\nso long ago.\n\nWe'll re-roll it and submit it for inclusion.\n-- \nRegards,\nLuben\n\n"}]}