{"thread":{"id":"59041","subject":"[PATCH v4 0/1] Expose header information to git-send-email's sendemail-validate hook","startedAt":"2023-01-06T21:51:07Z","lastAt":"2023-01-06T23:48:12Z","messageCount":4,"participants":["Strawbridge, Michael","Luben Tuikov","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":1},"messages":[{"id":"469853","messageId":"20230106215012.1079319-1-michael.strawbridge@amd.com","threadId":"59041","inReplyTo":null,"subject":"[PATCH v4 0/1] Expose header information to git-send-email's sendemail-validate hook","fromName":"Strawbridge, Michael","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-06T21:50:55Z","receivedAt":"2023-01-06T21:51:07Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"I have fixed the t9001 errors this patch was getting earlier.\n\nFor reference of previous patch versions:\nv1 - https://public-inbox.org/git/20221111021502.449662-1-michael.strawbridge@amd.com/T/#t\nv2 - https://public-inbox.org/git/20221111193042.641898-1-michael.strawbridge@amd.com/T/#t\nv3 - https://public-inbox.org/git/20221111194223.644845-1-michael.strawbridge@amd.com/T/#t\n\nMichael Strawbridge (1):\n  Expose header information to git-send-email's sendemail-validate hook\n\n Documentation/githooks.txt |  8 +++---\n git-send-email.perl        | 55 +++++++++++++++++++++++++-------------\n t/t9001-send-email.sh      | 25 +++++++++++++++++\n 3 files changed, 65 insertions(+), 23 deletions(-)\n\n-- \n2.34.1\n"},{"id":"469854","messageId":"20230106215012.1079319-2-michael.strawbridge@amd.com","threadId":"59041","inReplyTo":"20230106215012.1079319-1-michael.strawbridge@amd.com","subject":"[PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook","fromName":"Strawbridge, Michael","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-06T21:51:09Z","receivedAt":"2023-01-06T21:51:35Z","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 that git-send-email intends to send, is now\npassed as a 2nd argument to the sendemail-validate hook.  A new t9001\ntest was added to test this 2nd arg and docs are also updated.\n\nAs an example, this can be useful for acting upon keywords in the\nsubject or specific email addresses.\n\nCc: Luben Tuikov <luben.tuikov@amd.com>\nCc: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: Michael Strawbridge <michael.strawbridge@amd.com>\n---\n Documentation/githooks.txt |  8 +++---\n git-send-email.perl        | 55 +++++++++++++++++++++++++-------------\n t/t9001-send-email.sh      | 25 +++++++++++++++++\n 3 files changed, 65 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex a16e62bc8c..346e536cbe 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -583,10 +583,10 @@ 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].  It takes two parameters,\n+the name of a file that holds the patch and the name of a file that holds the\n+SMTP headers.  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 5861e99a6e..5a626a4238 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -787,14 +787,6 @@ sub is_format_patch_arg {\n \n @files = handle_backup_files(@files);\n \n-if ($validate) {\n-\tforeach my $f (@files) {\n-\t\tunless (-p $f) {\n-\t\t\tvalidate_patch($f, $target_xfer_encoding);\n-\t\t}\n-\t}\n-}\n-\n if (@files) {\n \tunless ($quiet) {\n \t\tprint $_,\"\\n\" for (@files);\n@@ -1495,16 +1487,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 +1529,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@@ -1955,6 +1954,15 @@ sub process_file {\n \t\t}\n \t}\n \n+\n+\tif ($validate) {\n+\t\tforeach my $f (@files) {\n+\t\t\tunless (-p $f) {\n+\t\t\t\tvalidate_patch($f, $target_xfer_encoding);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tmy $message_was_sent = send_message();\n \tif ($message_was_sent == -1) {\n \t\tdo_edit($t);\n@@ -2088,11 +2096,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, \"<patch>\");\n-\t\t\tmy @cmd_run = (@cmd, $target);\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..11e68f9c18 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -565,6 +565,31 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success $PREREQ \"--validate hook supports header argument\" '\n+\ttest_when_finished \"rm my-hooks.ran\" &&\n+\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n+\tfilesize=$(stat -c%s \"$2\")\n+\tif [ \"$filesize\" != \"0\" ]; then\n+\t>my-hooks.ran\n+\tfi\n+\texit 1\n+\tEOF\n+\ttest_config core.hooksPath \"my-hooks\" &&\n+\ttest_must_fail git send-email \\\n+\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t--to=nobody@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--validate \\\n+\t\tlongline.patch 2>actual &&\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 -- <patch>'\"'\"' died with exit code 1\n+\twarning: no patches were sent\n+\tEOF\n+\ttest_cmp expect actual\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"},{"id":"469855","messageId":"f6027f18-b0d9-8aee-3f9e-0ac91bb86e96@amd.com","threadId":"59041","inReplyTo":"20230106215012.1079319-2-michael.strawbridge@amd.com","subject":"Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook","fromName":"Luben Tuikov","fromEmail":"luben.tuikov@amd.com","sentAt":"2023-01-06T22:27:19Z","receivedAt":"2023-01-06T22:27:37Z","isPatch":true,"sender":{"key":"luben.tuikov@amd.com","avatar":null},"body":"Looks good to me.\n\nReviewed-by: Luben Tuikov <luben.tuikov@amd.com>\n\nRegards,\nLuben\n\nOn 2023-01-06 16:51, Strawbridge, Michael wrote:\n> To allow further flexibility in the git hook, the smtp header\n> information of the email that git-send-email intends to send, is now\n> passed as a 2nd argument to the sendemail-validate hook.  A new t9001\n> test was added to test this 2nd arg and docs are also updated.\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 <luben.tuikov@amd.com>\n> Cc: brian m. carlson <sandals@crustytoothpaste.net>\n> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>\n> ---\n>  Documentation/githooks.txt |  8 +++---\n>  git-send-email.perl        | 55 +++++++++++++++++++++++++-------------\n>  t/t9001-send-email.sh      | 25 +++++++++++++++++\n>  3 files changed, 65 insertions(+), 23 deletions(-)\n> \n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index a16e62bc8c..346e536cbe 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -583,10 +583,10 @@ 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].  It takes two parameters,\n> +the name of a file that holds the patch and the name of a file that holds the\n> +SMTP headers.  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 5861e99a6e..5a626a4238 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -787,14 +787,6 @@ sub is_format_patch_arg {\n>  \n>  @files = handle_backup_files(@files);\n>  \n> -if ($validate) {\n> -\tforeach my $f (@files) {\n> -\t\tunless (-p $f) {\n> -\t\t\tvalidate_patch($f, $target_xfer_encoding);\n> -\t\t}\n> -\t}\n> -}\n> -\n>  if (@files) {\n>  \tunless ($quiet) {\n>  \t\tprint $_,\"\\n\" for (@files);\n> @@ -1495,16 +1487,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 +1529,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> @@ -1955,6 +1954,15 @@ sub process_file {\n>  \t\t}\n>  \t}\n>  \n> +\n> +\tif ($validate) {\n> +\t\tforeach my $f (@files) {\n> +\t\t\tunless (-p $f) {\n> +\t\t\t\tvalidate_patch($f, $target_xfer_encoding);\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n>  \tmy $message_was_sent = send_message();\n>  \tif ($message_was_sent == -1) {\n>  \t\tdo_edit($t);\n> @@ -2088,11 +2096,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, \"<patch>\");\n> -\t\t\tmy @cmd_run = (@cmd, $target);\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..11e68f9c18 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -565,6 +565,31 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success $PREREQ \"--validate hook supports header argument\" '\n> +\ttest_when_finished \"rm my-hooks.ran\" &&\n> +\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n> +\tfilesize=$(stat -c%s \"$2\")\n> +\tif [ \"$filesize\" != \"0\" ]; then\n> +\t>my-hooks.ran\n> +\tfi\n> +\texit 1\n> +\tEOF\n> +\ttest_config core.hooksPath \"my-hooks\" &&\n> +\ttest_must_fail git send-email \\\n> +\t\t--from=\"Example <nobody@example.com>\" \\\n> +\t\t--to=nobody@example.com \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> +\t\t--validate \\\n> +\t\tlongline.patch 2>actual &&\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 -- <patch>'\"'\"' died with exit code 1\n> +\twarning: no patches were sent\n> +\tEOF\n> +\ttest_cmp expect actual\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\n"},{"id":"469860","messageId":"xmqqcz7rp6mk.fsf@gitster.g","threadId":"59041","inReplyTo":"20230106215012.1079319-2-michael.strawbridge@amd.com","subject":"Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-06T23:48:03Z","receivedAt":"2023-01-06T23:48:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Strawbridge, Michael\" <Michael.Strawbridge@amd.com> writes:\n\n> Subject: Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook\n\nSubject: [PATCH v5 1/1] send-email: expose blah blah ...\n\nI.e. follow \"<area>: describe the change with a single sentence\"\nconvention to allow this change blend in better in \"git shortlog\"\noutput in the future release, once we accept the change.\n\n> To allow further flexibility in the git hook, the smtp header\n\nI recall that in the cover letter for a previous round you mentioned\nthat s/smtp/SMTP/ was done?\n\n> information of the email that git-send-email intends to send, is now\n> passed as a 2nd argument to the sendemail-validate hook.\n\nOK.  Existing hooks will not see the second argument, ignore it and\ncontinue to work as before in the best case.  In the worst case, it\nnotices that there is an unexpected argument on its command line and\nbarf.\n\n> A new t9001\n> test was added to test this 2nd arg and docs are also updated.\n\nIt is not wrong per-se but it is perfectly expected to add tests to\nprotect a new feature from future breakage and docs to describe it,\nso strictly speaking this sentence is not necessary.\n\n> As an example, this can be useful for acting upon keywords in the\n> subject or specific email addresses.\n\nGood.\n\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index a16e62bc8c..346e536cbe 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -583,10 +583,10 @@ 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].  It takes two parameters,\n> +the name of a file that holds the patch and the name of a file that holds the\n> +SMTP headers.  Exiting with a non-zero status causes `git send-email` to abort\n> +before sending any e-mails.\n\nAre you changing the format and contents of what you feed as the\nfirst command line argument?  I am wondering why you did \"the name\nof the file that holds the e-mail to be sent\" (which is very clear\nthat it would contain both the proposed log message and the patch\nproper) to \"the name of the file that holds the patch\" (which now is\nvery unclear if we lost the proposed log message before the patch\nproper).\n\nIf we expect that the \"SMTP headers\" is the last update for this\nfeature, then the above is OK, but most likely we will want to add\nthe third one in a few years.  Doing\n\n    It takes these command line arguments:\n\n    1. the name of the file that holds the e-mail to be sent.\n    2. the name of the file that holds the SMTP headers to be used.\n\nwould be more future-proof.\n\nThe added description does not make (at least) one thing clear for\nme to write an experimental hook to make use of this new feature.\n\nThe message I am responding to has these headers, for example:\n\n    From:   \"Strawbridge, Michael\" <Michael.Strawbridge@amd.com>\n    Subject: [PATCH v4 1/1] Expose header information to git-send-email's\n     sendemail-validate hook\n    To:     \"git@vger.kernel.org\" <git@vger.kernel.org>\n    CC:     \"Strawbridge, Michael\" <Michael.Strawbridge@amd.com>,\n            \"Tuikov, Luben\" <Luben.Tuikov@amd.com>,\n            \"brian m . carlson\" <sandals@crustytoothpaste.net>\n\nNow, does my hook need to know about RFC 5322 rules govering the \ne-mail headers, like \"Cc:\" and \"CC:\" are equivalent, and a line that\nbegins with a whitespace adds to the value of the previous line\n(i.e. header folding)?\n\nAnother thing.  This is not a new issue, but the above paragraph\ndoes not mention the fact that the hook silently chdir's to the root\nof the working tree (or the repository) while running the hook.  We\nshould fix that but not as a part of this patch.\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 5861e99a6e..5a626a4238 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -787,14 +787,6 @@ sub is_format_patch_arg {\n\nThe patch looks very messy and unreviewable.  Each step of a patch\nshould do a single logical change well and cover the change in the\nbehaviour in documentation and tests if necessary.\n\nBut this seems to do too many things in a single step, I suspect.\nIt probably should be split into a handful of steps, earlier ones\njust reorganizing the code structure (like splitting the early part\nof \"send-message\" into a separate \"gen-header\" helper function, or\nnot calling validate_patch() early) without changing any externally\nobservable behaviour, and then final ones (possibly just the single\nlast step) changing how the hook is invoked (hence the documentation\nand test changes would only appear in later steps).\n\n>  @files = handle_backup_files(@files);\n>  \n> -if ($validate) {\n> -\tforeach my $f (@files) {\n> -\t\tunless (-p $f) {\n> -\t\t\tvalidate_patch($f, $target_xfer_encoding);\n> -\t\t}\n> -\t}\n> -}\n> -\n>  if (@files) {\n>  \tunless ($quiet) {\n>  \t\tprint $_,\"\\n\" for (@files);\n> @@ -1495,16 +1487,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 +1529,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> @@ -1955,6 +1954,15 @@ sub process_file {\n>  \t\t}\n>  \t}\n>  \n> +\n> +\tif ($validate) {\n> +\t\tforeach my $f (@files) {\n> +\t\t\tunless (-p $f) {\n> +\t\t\t\tvalidate_patch($f, $target_xfer_encoding);\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n\nIs this now done inside \"process_file\"?  Is your \"process_file\"\nstill called once per e-mail message?  If both are true, then this\npart of the patch smells very wrong.  The original checks _all_\nfiles with validate_patch() before sending even a single message,\nbecause it does not send just a few, find a problem in the third\npatch and stop.  Moving the loop to check all messages into a\nfunction that is called once for each message simply does not make\nsense---the desired \"all or none\" semantics may be retained because\nthe invocation of process_file for the first message will make all\nmessages to be inspected and a failure on any of them would cause\nthe process to stop, but that is by accident and not by a sound\ndesign.  When sending a 5-patch series, in the normal case where the\npatches we have are all good, we will inspect these patches over and\nover again, no?\n\n> @@ -2088,11 +2096,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, \"<patch>\");\n> -\t\t\tmy @cmd_run = (@cmd, $target);\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\nOutside this topic, we probably would want to get rid of these \"chdir()\"\nand possibly \"local $ENV{}\" assignments.  The former should be\ndoable simply by starting @cmd with (\"git\", \"-C\", $dir).\n"}]}