{"thread":{"id":"45939","subject":"[RFC] send-email: support validate hook","startedAt":"2017-05-11T19:38:04Z","lastAt":"2017-05-15T18:10:22Z","messageCount":9,"participants":["Jonathan Tan","Brandon Williams","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"319446","messageId":"20170511193753.19594-1-jonathantanmy@google.com","threadId":"45939","inReplyTo":null,"subject":"[RFC] send-email: support validate hook","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-11T19:37:53Z","receivedAt":"2017-05-11T19:38:04Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, send-email has support for rudimentary e-mail validation.\nAllow the user to add support for more validation by providing a\nsendemail-validate hook.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n\nThis is motivated by situations in which we discuss a work in progress\nusing Gerrit (which requires Change-Id trailers in patches), and then,\nforgetting to remove the Change-Id trailers, send them to the Git\nmailing list (which does not want such trailers). I can envision such\nfunctionality being useful in other situations, hence this patch\nsubmission.\n\nI'm not very familiar with Perl, and \"There Is More Than One Way To Do\nIt\", so advice on Perl style is appreciated.\n---\n Documentation/git-send-email.txt |  1 +\n Documentation/githooks.txt       |  8 ++++++++\n git-send-email.perl              | 18 +++++++++++++++++-\n t/t9001-send-email.sh            | 40 ++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 66 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 9d66166f6..bb23b02ca 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -377,6 +377,7 @@ have been specified, in which case default to 'compose'.\n \tCurrently, validation means the following:\n +\n --\n+\t\t*\tInvoke the sendemail-validate hook if present (see linkgit:githooks[5]).\n \t\t*\tWarn of patches that contain lines longer than 998 characters; this\n \t\t\tis due to SMTP limits as described by http://www.ietf.org/rfc/rfc2821.txt.\n --\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 706091a56..b2514f4d4 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -447,6 +447,14 @@ rebase::\n The commits are guaranteed to be listed in the order that they were\n processed by rebase.\n \n+sendemail-validate\n+~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by 'git send-email'.  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+\n \n GIT\n ---\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex eea0a517f..7de91ca7c 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -27,6 +27,7 @@ use Term::ANSIColor;\n use File::Temp qw/ tempdir tempfile /;\n use File::Spec::Functions qw(catfile);\n use Error qw(:try);\n+use Cwd qw(abs_path cwd);\n use Git;\n use Git::I18N;\n \n@@ -628,9 +629,24 @@ if (@rev_list_opts) {\n @files = handle_backup_files(@files);\n \n if ($validate) {\n+\tmy @hook = ($repo->repo_path().'/hooks/sendemail-validate', '');\n+\tmy $use_hook = -x $hook[0];\n+\tif ($use_hook) {\n+\t\t# The hook needs a correct GIT_DIR.\n+\t\t$ENV{\"GIT_DIR\"} = $repo->repo_path();\n+\t}\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n-\t\t\tmy $error = validate_patch($f);\n+\t\t\tmy $error;\n+\t\t\tif ($use_hook) {\n+\t\t\t\t$hook[1] = abs_path($f);\n+\t\t\t\tmy $cwd_save = cwd();\n+\t\t\t\tchdir($repo->wc_path() or $repo->repo_path());\n+\t\t\t\t$error = \"rejected by sendemail-validate hook\"\n+\t\t\t\t\tunless system(@hook) == 0;\n+\t\t\t\tchdir($cwd_save);\n+\t\t\t}\n+\t\t\t$error = validate_patch($f) unless $error;\n \t\t\t$error and die sprintf(__(\"fatal: %s: %s\\nwarning: no patches were sent\\n\"),\n \t\t\t\t\t\t  $f, $error);\n \t\t}\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 60a80f60b..f3f238d40 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1913,4 +1913,44 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '\n \ttest_cmp expected-list actual-list\n '\n \n+test_expect_success $PREREQ 'invoke hook' '\n+\tmkdir -p .git/hooks &&\n+\n+\twrite_script .git/hooks/sendemail-validate <<-\\EOF &&\n+\t\t# test that we have the correct environment variable, pwd, and\n+\t\t# argument\n+\t\tcase \"$GIT_DIR\" in\n+\t\t\t*.git)\n+\t\t\t\ttrue\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tfalse\n+\t\t\t\t;;\n+\t\tesac &&\n+\t\ttest -e 0001-add-master.patch &&\n+\t\tgrep \"add master\" \"$1\"\n+\tEOF\n+\n+\tmkdir subdir &&\n+\t(\n+\t\t# Test that it works even if we are not at the root of the\n+\t\t# working tree\n+\t\tcd subdir &&\n+\t\tgit send-email \\\n+\t\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t\t--to=nobody@example.com \\\n+\t\t\t--smtp-server=\"$(pwd)/../fake.sendmail\" \\\n+\t\t\t../0001-add-master.patch &&\n+\n+\t\t# Verify error message when a patch is rejected by the hook\n+\t\tsed -e \"s/add master/x/\" ../0001-add-master.patch >../another.patch &&\n+\t\tgit send-email \\\n+\t\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t\t--to=nobody@example.com \\\n+\t\t\t--smtp-server=\"$(pwd)/../fake.sendmail\" \\\n+\t\t\t../another.patch 2>err\n+\t\ttest_i18ngrep \"rejected by sendemail-validate hook\" err\n+\t)\n+'\n+\n test_done\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319462","messageId":"20170511203955.GL83655@google.com","threadId":"45939","inReplyTo":"20170511193753.19594-1-jonathantanmy@google.com","subject":"Re: [RFC] send-email: support validate hook","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-11T20:39:55Z","receivedAt":"2017-05-11T20:40:03Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/11, Jonathan Tan wrote:\n> Currently, send-email has support for rudimentary e-mail validation.\n> Allow the user to add support for more validation by providing a\n> sendemail-validate hook.\n> \n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> \n> This is motivated by situations in which we discuss a work in progress\n> using Gerrit (which requires Change-Id trailers in patches), and then,\n> forgetting to remove the Change-Id trailers, send them to the Git\n> mailing list (which does not want such trailers). I can envision such\n> functionality being useful in other situations, hence this patch\n> submission.\n> \n> I'm not very familiar with Perl, and \"There Is More Than One Way To Do\n> It\", so advice on Perl style is appreciated.\n\nI can't attest to the Perl code itself (I prefer to not touch it :D) but\nI've tested this and it works for my purposes!\n\n> ---\n>  Documentation/git-send-email.txt |  1 +\n>  Documentation/githooks.txt       |  8 ++++++++\n>  git-send-email.perl              | 18 +++++++++++++++++-\n>  t/t9001-send-email.sh            | 40 ++++++++++++++++++++++++++++++++++++++++\n>  4 files changed, 66 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index 9d66166f6..bb23b02ca 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -377,6 +377,7 @@ have been specified, in which case default to 'compose'.\n>  \tCurrently, validation means the following:\n>  +\n>  --\n> +\t\t*\tInvoke the sendemail-validate hook if present (see linkgit:githooks[5]).\n>  \t\t*\tWarn of patches that contain lines longer than 998 characters; this\n>  \t\t\tis due to SMTP limits as described by http://www.ietf.org/rfc/rfc2821.txt.\n>  --\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 706091a56..b2514f4d4 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -447,6 +447,14 @@ rebase::\n>  The commits are guaranteed to be listed in the order that they were\n>  processed by rebase.\n>  \n> +sendemail-validate\n> +~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by 'git send-email'.  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> +\n>  \n>  GIT\n>  ---\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index eea0a517f..7de91ca7c 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -27,6 +27,7 @@ use Term::ANSIColor;\n>  use File::Temp qw/ tempdir tempfile /;\n>  use File::Spec::Functions qw(catfile);\n>  use Error qw(:try);\n> +use Cwd qw(abs_path cwd);\n>  use Git;\n>  use Git::I18N;\n>  \n> @@ -628,9 +629,24 @@ if (@rev_list_opts) {\n>  @files = handle_backup_files(@files);\n>  \n>  if ($validate) {\n> +\tmy @hook = ($repo->repo_path().'/hooks/sendemail-validate', '');\n> +\tmy $use_hook = -x $hook[0];\n> +\tif ($use_hook) {\n> +\t\t# The hook needs a correct GIT_DIR.\n> +\t\t$ENV{\"GIT_DIR\"} = $repo->repo_path();\n> +\t}\n>  \tforeach my $f (@files) {\n>  \t\tunless (-p $f) {\n> -\t\t\tmy $error = validate_patch($f);\n> +\t\t\tmy $error;\n> +\t\t\tif ($use_hook) {\n> +\t\t\t\t$hook[1] = abs_path($f);\n> +\t\t\t\tmy $cwd_save = cwd();\n> +\t\t\t\tchdir($repo->wc_path() or $repo->repo_path());\n> +\t\t\t\t$error = \"rejected by sendemail-validate hook\"\n> +\t\t\t\t\tunless system(@hook) == 0;\n> +\t\t\t\tchdir($cwd_save);\n> +\t\t\t}\n> +\t\t\t$error = validate_patch($f) unless $error;\n>  \t\t\t$error and die sprintf(__(\"fatal: %s: %s\\nwarning: no patches were sent\\n\"),\n>  \t\t\t\t\t\t  $f, $error);\n>  \t\t}\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 60a80f60b..f3f238d40 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -1913,4 +1913,44 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '\n>  \ttest_cmp expected-list actual-list\n>  '\n>  \n> +test_expect_success $PREREQ 'invoke hook' '\n> +\tmkdir -p .git/hooks &&\n> +\n> +\twrite_script .git/hooks/sendemail-validate <<-\\EOF &&\n> +\t\t# test that we have the correct environment variable, pwd, and\n> +\t\t# argument\n> +\t\tcase \"$GIT_DIR\" in\n> +\t\t\t*.git)\n> +\t\t\t\ttrue\n> +\t\t\t\t;;\n> +\t\t\t*)\n> +\t\t\t\tfalse\n> +\t\t\t\t;;\n> +\t\tesac &&\n> +\t\ttest -e 0001-add-master.patch &&\n> +\t\tgrep \"add master\" \"$1\"\n> +\tEOF\n> +\n> +\tmkdir subdir &&\n> +\t(\n> +\t\t# Test that it works even if we are not at the root of the\n> +\t\t# working tree\n> +\t\tcd subdir &&\n> +\t\tgit send-email \\\n> +\t\t\t--from=\"Example <nobody@example.com>\" \\\n> +\t\t\t--to=nobody@example.com \\\n> +\t\t\t--smtp-server=\"$(pwd)/../fake.sendmail\" \\\n> +\t\t\t../0001-add-master.patch &&\n> +\n> +\t\t# Verify error message when a patch is rejected by the hook\n> +\t\tsed -e \"s/add master/x/\" ../0001-add-master.patch >../another.patch &&\n> +\t\tgit send-email \\\n> +\t\t\t--from=\"Example <nobody@example.com>\" \\\n> +\t\t\t--to=nobody@example.com \\\n> +\t\t\t--smtp-server=\"$(pwd)/../fake.sendmail\" \\\n> +\t\t\t../another.patch 2>err\n> +\t\ttest_i18ngrep \"rejected by sendemail-validate hook\" err\n> +\t)\n> +'\n> +\n>  test_done\n> -- \n> 2.13.0.rc2.291.g57267f2277-goog\n> \n\n-- \nBrandon Williams\n"},{"id":"319495","messageId":"xmqq8tm295xt.fsf@gitster.mtv.corp.google.com","threadId":"45939","inReplyTo":"20170511193753.19594-1-jonathantanmy@google.com","subject":"Re: [RFC] send-email: support validate hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-12T02:15:10Z","receivedAt":"2017-05-12T02:15:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index eea0a517f..7de91ca7c 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -27,6 +27,7 @@ use Term::ANSIColor;\n>  use File::Temp qw/ tempdir tempfile /;\n>  use File::Spec::Functions qw(catfile);\n>  use Error qw(:try);\n> +use Cwd qw(abs_path cwd);\n>  use Git;\n>  use Git::I18N;\n>  \n> @@ -628,9 +629,24 @@ if (@rev_list_opts) {\n>  @files = handle_backup_files(@files);\n>  \n>  if ($validate) {\n> +\tmy @hook = ($repo->repo_path().'/hooks/sendemail-validate', '');\n> +\tmy $use_hook = -x $hook[0];\n> +\tif ($use_hook) {\n> +\t\t# The hook needs a correct GIT_DIR.\n> +\t\t$ENV{\"GIT_DIR\"} = $repo->repo_path();\n> +\t}\n>  \tforeach my $f (@files) {\n>  \t\tunless (-p $f) {\n> -\t\t\tmy $error = validate_patch($f);\n> +\t\t\tmy $error;\n> +\t\t\tif ($use_hook) {\n> +\t\t\t\t$hook[1] = abs_path($f);\n> +\t\t\t\tmy $cwd_save = cwd();\n> +\t\t\t\tchdir($repo->wc_path() or $repo->repo_path());\n> +\t\t\t\t$error = \"rejected by sendemail-validate hook\"\n> +\t\t\t\t\tunless system(@hook) == 0;\n> +\t\t\t\tchdir($cwd_save);\n> +\t\t\t}\n> +\t\t\t$error = validate_patch($f) unless $error;\n>  \t\t\t$error and die sprintf(__(\"fatal: %s: %s\\nwarning: no patches were sent\\n\"),\n>  \t\t\t\t\t\t  $f, $error);\n>  \t\t}\n\nThis is not about \"Perl code\" but I find it somewhat strange to add\nthis code to outside validate_patch() when we have a helper function \nspecifically designed to \"validate patch\" and the new code is about\nteaching the program an additional way to \"validate patch\".\n\nAlso I am not sure if setting and exporting GIT_DIR for the entire\nprogram, only when we run this hook is a sensible thing to do.\n\nEither the remainder of the program is safe to see the GIT_DIR\npointing at the correct location (in which case $ENV{GIT_DIR}\nassignment can be done outside \"if ($validate && $use_hook)\" to\naffect everything), or the rest of the program is not prepared to\nsee such a forced assignment and export (in which case we should\nlimit the setting of the environment to the subprocess that runs\nsystem(@hook) without affecting anything else).  Doing something in\nthe middle conditionally feels like it is inviting future troubles\ncoming from the inconsistency (e.g. \"this feature totally unrelated\nto the validate hook works when validate hook is in use but\notherwise it doesn't, because $GIT_DIR is sometimes set and\nsometimes not\").\n\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 60a80f60b..f3f238d40 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -1913,4 +1913,44 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '\n>  \ttest_cmp expected-list actual-list\n>  '\n>  \n> +test_expect_success $PREREQ 'invoke hook' '\n> +\tmkdir -p .git/hooks &&\n> +\n> +\twrite_script .git/hooks/sendemail-validate <<-\\EOF &&\n> +\t\t# test that we have the correct environment variable, pwd, and\n> +\t\t# argument\n> +\t\tcase \"$GIT_DIR\" in\n> +\t\t\t*.git)\n> +\t\t\t\ttrue\n> +\t\t\t\t;;\n> +\t\t\t*)\n> +\t\t\t\tfalse\n> +\t\t\t\t;;\n> +\t\tesac &&\n\nCase arms indented one level too deep.\n\n> +\t\ttest -e 0001-add-master.patch &&\n\nDo we only care about existence and do not mind if it is a\ndirectory?  Otherwise using \"-f\" would be more sensible, perhaps?\n"},{"id":"319502","messageId":"CACBZZX74dsTJtAQdrMmEDPKOVdEaiL4dGrmz9OZ3jQhhmr5xEA@mail.gmail.com","threadId":"45939","inReplyTo":"20170511193753.19594-1-jonathantanmy@google.com","subject":"Re: [RFC] send-email: support validate hook","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-12T07:23:07Z","receivedAt":"2017-05-12T18:44:34Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, May 11, 2017 at 9:37 PM, Jonathan Tan <jonathantanmy@google.com> wrote:\n> Currently, send-email has support for rudimentary e-mail validation.\n> Allow the user to add support for more validation by providing a\n> sendemail-validate hook.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>\n> This is motivated by situations in which we discuss a work in progress\n> using Gerrit (which requires Change-Id trailers in patches), and then,\n> forgetting to remove the Change-Id trailers, send them to the Git\n> mailing list (which does not want such trailers). I can envision such\n> functionality being useful in other situations, hence this patch\n> submission.\n>\n> I'm not very familiar with Perl, and \"There Is More Than One Way To Do\n> It\", so advice on Perl style is appreciated.\n\n\nI hacked this up last night, it also addresses Junio's comment about GIT_DIR:\n\n+++ b/git-send-email.perl\n@@ -25,7 +25,7 @@ use Getopt::Long;\n use Text::ParseWords;\n use Term::ANSIColor;\n use File::Temp qw/ tempdir tempfile /;\n-use File::Spec::Functions qw(catfile);\n+use File::Spec::Functions qw(catdir catfile);\n use Error qw(:try);\n use Cwd qw(abs_path cwd);\n use Git;\n@@ -629,22 +629,19 @@ if (@rev_list_opts) {\n @files = handle_backup_files(@files);\n\n if ($validate) {\n-       my @hook = ($repo->repo_path().'/hooks/sendemail-validate', '');\n-       my $use_hook = -x $hook[0];\n-       if ($use_hook) {\n-               # The hook needs a correct GIT_DIR.\n-               $ENV{\"GIT_DIR\"} = $repo->repo_path();\n-       }\n+       my $validate_hook = catfile(catdir($repo->repo_path(),\n'hooks'), 'sendemail-validate');\n        foreach my $f (@files) {\n                unless (-p $f) {\n                        my $error;\n-                       if ($use_hook) {\n-                               $hook[1] = abs_path($f);\n+                       if (-x $validate_hook) {\n+                               my $target = abs_path($f);\n                                my $cwd_save = cwd();\n-                               chdir($repo->wc_path() or $repo->repo_path());\n+                               chdir($repo->wc_path() or\n$repo->repo_path()) or die \"chdir: $!\";\n+                               # The hook needs a correct GIT_DIR.\n+                               local $ENV{\"GIT_DIR\"} = $repo->repo_path();\n                                $error = \"rejected by sendemail-validate hook\"\n-                                       unless system(@hook) == 0;\n-                               chdir($cwd_save);\n+                                       if system($validate_hook, $target);\n+                               chdir($cwd_save) or die \"chdir: $!\";\n                        }\n                        $error = validate_patch($f) unless $error;\n                        $error and die sprintf(__(\"fatal: %s:\n%s\\nwarning: no patches were sent\\n\"),\n\nChanges there:\n\n * use catdir instead of string concat, I don't know if we run\nformat-patch on any platform where this matters in theory (e.g. VMS I\nthink), but the file uses that API already, so continue using it.\n * Just make this more brief by moving the -x test into the loop,\nwe're sending E-Mail here, no need to optimize stat calls (you did ask\nfor style advice :)\n* Check the return value of chdir & die appropriately\n* localize GIT_DIR\n* \"die if system\" is more idiomatic than \"die unless system == 0\"\n\nOr actually just move this into a function:\n\n@@ -629,23 +629,11 @@ if (@rev_list_opts) {\n @files = handle_backup_files(@files);\n\n if ($validate) {\n-       my @hook = ($repo->repo_path().'/hooks/sendemail-validate', '');\n-       my $use_hook = -x $hook[0];\n-       if ($use_hook) {\n-               # The hook needs a correct GIT_DIR.\n-               $ENV{\"GIT_DIR\"} = $repo->repo_path();\n-       }\n+       my $validate_hook = catfile(catdir($repo->repo_path(),\n'hooks'), 'sendemail-validate');\n        foreach my $f (@files) {\n                unless (-p $f) {\n                        my $error;\n-                       if ($use_hook) {\n-                               $hook[1] = abs_path($f);\n-                               my $cwd_save = cwd();\n-                               chdir($repo->wc_path() or $repo->repo_path());\n-                               $error = \"rejected by sendemail-validate hook\"\n-                                       unless system(@hook) == 0;\n-                               chdir($cwd_save);\n-                       }\n+                       $error = validate_via_hook($validate_hook, $f)\nif -x $validate_hook;\n                        $error = validate_patch($f) unless $error;\n                        $error and die sprintf(__(\"fatal: %s:\n%s\\nwarning: no patches were sent\\n\"),\n                                                  $f, $error);\n@@ -1763,6 +1751,22 @@ sub validate_patch {\n        return;\n }\n\n+sub validate_via_hook {\n+       my ($validate_hook, $patch) = @_;\n+       my $error;\n+\n+       my $target = abs_path($patch);\n+       my $cwd_save = cwd();\n+       chdir($repo->wc_path() or $repo->repo_path()) or die \"chdir: $!\";\n+       # The hook needs a correct GIT_DIR.\n+       local $ENV{\"GIT_DIR\"} = $repo->repo_path();\n+       $error = \"rejected by sendemail-validate hook\"\n+               if system($validate_hook, $target);\n+       chdir($cwd_save) or die \"chdir: $!\";\n+\n+       return $error;\n+}\n+\n sub handle_backup {\n        my ($last, $lastlen, $file, $known_suffix) = @_;\n        my ($suffix, $skip);\n\nI wonder if we were designing this interface today whether whether the\nexisting behavior of  --validate wouldn't just be shipped as a\n*.sample hook instead. There's also the caveat now that your hook\nmight be OK with really long lines, but the existing validate function\ndenies it, and there's no way to override that. I think a better way\nto do this is:\n\n        foreach my $f (@files) {\n                unless (-p $f) {\n-                       my $error;\n-                       if ($use_hook) {\n-                               $hook[1] = abs_path($f);\n-                               my $cwd_save = cwd();\n-                               chdir($repo->wc_path() or $repo->repo_path());\n-                               $error = \"rejected by sendemail-validate hook\"\n-                                       unless system(@hook) == 0;\n-                               chdir($cwd_save);\n-                       }\n-                       $error = validate_patch($f) unless $error;\n+                       my $error = -x $validate_hook\n+                               ? validate_via_hook($validate_hook, $f)\n+                               : validate_patch($f);\n\nI.e. if you specify a validate hook it replaces the existing hardcoded behavior.\n\nAlso, just to check, is this new thing still consistent with the cwd\ndocs in githooks (see e.g. 501d3cd7b8).?\n\n> ---\n>  Documentation/git-send-email.txt |  1 +\n>  Documentation/githooks.txt       |  8 ++++++++\n>  git-send-email.perl              | 18 +++++++++++++++++-\n>  t/t9001-send-email.sh            | 40 ++++++++++++++++++++++++++++++++++++++++\n>  4 files changed, 66 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index 9d66166f6..bb23b02ca 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -377,6 +377,7 @@ have been specified, in which case default to 'compose'.\n>         Currently, validation means the following:\n>  +\n>  --\n> +               *       Invoke the sendemail-validate hook if present (see linkgit:githooks[5]).\n>                 *       Warn of patches that contain lines longer than 998 characters; this\n>                         is due to SMTP limits as described by http://www.ietf.org/rfc/rfc2821.txt.\n>  --\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 706091a56..b2514f4d4 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -447,6 +447,14 @@ rebase::\n>  The commits are guaranteed to be listed in the order that they were\n>  processed by rebase.\n>\n> +sendemail-validate\n> +~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by 'git send-email'.  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> +\n>\n>  GIT\n>  ---\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index eea0a517f..7de91ca7c 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -27,6 +27,7 @@ use Term::ANSIColor;\n>  use File::Temp qw/ tempdir tempfile /;\n>  use File::Spec::Functions qw(catfile);\n>  use Error qw(:try);\n> +use Cwd qw(abs_path cwd);\n>  use Git;\n>  use Git::I18N;\n>\n> @@ -628,9 +629,24 @@ if (@rev_list_opts) {\n>  @files = handle_backup_files(@files);\n>\n>  if ($validate) {\n> +       my @hook = ($repo->repo_path().'/hooks/sendemail-validate', '');\n> +       my $use_hook = -x $hook[0];\n> +       if ($use_hook) {\n> +               # The hook needs a correct GIT_DIR.\n> +               $ENV{\"GIT_DIR\"} = $repo->repo_path();\n> +       }\n>         foreach my $f (@files) {\n>                 unless (-p $f) {\n> -                       my $error = validate_patch($f);\n> +                       my $error;\n> +                       if ($use_hook) {\n> +                               $hook[1] = abs_path($f);\n> +                               my $cwd_save = cwd();\n> +                               chdir($repo->wc_path() or $repo->repo_path());\n> +                               $error = \"rejected by sendemail-validate hook\"\n> +                                       unless system(@hook) == 0;\n> +                               chdir($cwd_save);\n> +                       }\n> +                       $error = validate_patch($f) unless $error;\n>                         $error and die sprintf(__(\"fatal: %s: %s\\nwarning: no patches were sent\\n\"),\n>                                                   $f, $error);\n>                 }\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 60a80f60b..f3f238d40 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -1913,4 +1913,44 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '\n>         test_cmp expected-list actual-list\n>  '\n>\n> +test_expect_success $PREREQ 'invoke hook' '\n> +       mkdir -p .git/hooks &&\n> +\n> +       write_script .git/hooks/sendemail-validate <<-\\EOF &&\n> +               # test that we have the correct environment variable, pwd, and\n> +               # argument\n> +               case \"$GIT_DIR\" in\n> +                       *.git)\n> +                               true\n> +                               ;;\n> +                       *)\n> +                               false\n> +                               ;;\n> +               esac &&\n> +               test -e 0001-add-master.patch &&\n> +               grep \"add master\" \"$1\"\n> +       EOF\n> +\n> +       mkdir subdir &&\n> +       (\n> +               # Test that it works even if we are not at the root of the\n> +               # working tree\n> +               cd subdir &&\n> +               git send-email \\\n> +                       --from=\"Example <nobody@example.com>\" \\\n> +                       --to=nobody@example.com \\\n> +                       --smtp-server=\"$(pwd)/../fake.sendmail\" \\\n> +                       ../0001-add-master.patch &&\n> +\n> +               # Verify error message when a patch is rejected by the hook\n> +               sed -e \"s/add master/x/\" ../0001-add-master.patch >../another.patch &&\n> +               git send-email \\\n> +                       --from=\"Example <nobody@example.com>\" \\\n> +                       --to=nobody@example.com \\\n> +                       --smtp-server=\"$(pwd)/../fake.sendmail\" \\\n> +                       ../another.patch 2>err\n> +               test_i18ngrep \"rejected by sendemail-validate hook\" err\n> +       )\n> +'\n> +\n>  test_done\n> --\n> 2.13.0.rc2.291.g57267f2277-goog\n>\n"},{"id":"319577","messageId":"45b9e2d1-0840-d163-468f-588253cb8527@google.com","threadId":"45939","inReplyTo":"CACBZZX74dsTJtAQdrMmEDPKOVdEaiL4dGrmz9OZ3jQhhmr5xEA@mail.gmail.com","subject":"Re: [RFC] send-email: support validate hook","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-12T22:31:54Z","receivedAt":"2017-05-12T22:32:01Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 05/12/2017 12:23 AM, Ævar Arnfjörð Bjarmason wrote:\n> I hacked this up last night, it also addresses Junio's comment about GIT_DIR:\n>\n[snip]\n>\n> Changes there:\n>\n>  * use catdir instead of string concat, I don't know if we run\n> format-patch on any platform where this matters in theory (e.g. VMS I\n> think), but the file uses that API already, so continue using it.\n>  * Just make this more brief by moving the -x test into the loop,\n> we're sending E-Mail here, no need to optimize stat calls (you did ask\n> for style advice :)\n> * Check the return value of chdir & die appropriately\n> * localize GIT_DIR\n> * \"die if system\" is more idiomatic than \"die unless system == 0\"\n\nThanks - all these are very helpful. Especially the one about localizing \nGIT_DIR - this allows me to move everything into the validate_patch() \nfunction.\n\n> Or actually just move this into a function:\n>\n[snip]\n\nI'll send out another version of the patch that puts all these into the \nvalidate_patch() function.\n\n> I wonder if we were designing this interface today whether whether the\n> existing behavior of  --validate wouldn't just be shipped as a\n> *.sample hook instead. There's also the caveat now that your hook\n> might be OK with really long lines, but the existing validate function\n> denies it, and there's no way to override that. I think a better way\n> to do this is:\n>\n>         foreach my $f (@files) {\n>                 unless (-p $f) {\n> -                       my $error;\n> -                       if ($use_hook) {\n> -                               $hook[1] = abs_path($f);\n> -                               my $cwd_save = cwd();\n> -                               chdir($repo->wc_path() or $repo->repo_path());\n> -                               $error = \"rejected by sendemail-validate hook\"\n> -                                       unless system(@hook) == 0;\n> -                               chdir($cwd_save);\n> -                       }\n> -                       $error = validate_patch($f) unless $error;\n> +                       my $error = -x $validate_hook\n> +                               ? validate_via_hook($validate_hook, $f)\n> +                               : validate_patch($f);\n>\n> I.e. if you specify a validate hook it replaces the existing hardcoded behavior.\n\nI'm OK either way.\n\n> Also, just to check, is this new thing still consistent with the cwd\n> docs in githooks (see e.g. 501d3cd7b8).?\n\nAnything particular that you think is inconsistent? It is consistent \nwith \"Before Git invokes a hook, it changes its working directory to \neither $GIT_DIR in a bare repository or the root of the working tree in \na non-bare repository\". (The commit you reference refers to push hooks, \nof which this isn't one.)\n"},{"id":"319579","messageId":"CACBZZX4zSmFrWfksAoDTyaRHCXEGBLXbY_8wxzNVyXgLq8N-KQ@mail.gmail.com","threadId":"45939","inReplyTo":"45b9e2d1-0840-d163-468f-588253cb8527@google.com","subject":"Re: [RFC] send-email: support validate hook","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-12T22:34:06Z","receivedAt":"2017-05-12T22:34:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, May 13, 2017 at 12:31 AM, Jonathan Tan <jonathantanmy@google.com> wrote:\n> On 05/12/2017 12:23 AM, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> I hacked this up last night, it also addresses Junio's comment about\n>> GIT_DIR:\n>>\n> [snip]\n>>\n>>\n>> Changes there:\n>>\n>>  * use catdir instead of string concat, I don't know if we run\n>> format-patch on any platform where this matters in theory (e.g. VMS I\n>> think), but the file uses that API already, so continue using it.\n>>  * Just make this more brief by moving the -x test into the loop,\n>> we're sending E-Mail here, no need to optimize stat calls (you did ask\n>> for style advice :)\n>> * Check the return value of chdir & die appropriately\n>> * localize GIT_DIR\n>> * \"die if system\" is more idiomatic than \"die unless system == 0\"\n>\n>\n> Thanks - all these are very helpful. Especially the one about localizing\n> GIT_DIR - this allows me to move everything into the validate_patch()\n> function.\n>\n>> Or actually just move this into a function:\n>>\n> [snip]\n>\n> I'll send out another version of the patch that puts all these into the\n> validate_patch() function.\n>\n>> I wonder if we were designing this interface today whether whether the\n>> existing behavior of  --validate wouldn't just be shipped as a\n>> *.sample hook instead. There's also the caveat now that your hook\n>> might be OK with really long lines, but the existing validate function\n>> denies it, and there's no way to override that. I think a better way\n>> to do this is:\n>>\n>>         foreach my $f (@files) {\n>>                 unless (-p $f) {\n>> -                       my $error;\n>> -                       if ($use_hook) {\n>> -                               $hook[1] = abs_path($f);\n>> -                               my $cwd_save = cwd();\n>> -                               chdir($repo->wc_path() or\n>> $repo->repo_path());\n>> -                               $error = \"rejected by sendemail-validate\n>> hook\"\n>> -                                       unless system(@hook) == 0;\n>> -                               chdir($cwd_save);\n>> -                       }\n>> -                       $error = validate_patch($f) unless $error;\n>> +                       my $error = -x $validate_hook\n>> +                               ? validate_via_hook($validate_hook, $f)\n>> +                               : validate_patch($f);\n>>\n>> I.e. if you specify a validate hook it replaces the existing hardcoded\n>> behavior.\n>\n>\n> I'm OK either way.\n>\n>> Also, just to check, is this new thing still consistent with the cwd\n>> docs in githooks (see e.g. 501d3cd7b8).?\n>\n>\n> Anything particular that you think is inconsistent? It is consistent with\n> \"Before Git invokes a hook, it changes its working directory to either\n> $GIT_DIR in a bare repository or the root of the working tree in a non-bare\n> repository\". (The commit you reference refers to push hooks, of which this\n> isn't one.)\n\nI don't think it's inconsistent, didn't check the patch carefully\nenough, just asking if it was, sounds like it isn't.\n"},{"id":"319581","messageId":"20170512223827.21372-1-jonathantanmy@google.com","threadId":"45939","inReplyTo":"20170511193753.19594-1-jonathantanmy@google.com","subject":"[PATCH v2] send-email: support validate hook","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-12T22:38:26Z","receivedAt":"2017-05-12T22:38:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, send-email has support for rudimentary e-mail validation.\nAllow the user to add support for more validation by providing a\nsendemail-validate hook.\n\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n\nChange from v1: followed Ævar's suggestions, and also moved the new\nfunctionality into validate_patch().\n---\n Documentation/git-send-email.txt |  1 +\n Documentation/githooks.txt       |  8 ++++++++\n git-send-email.perl              | 20 +++++++++++++++++++-\n t/t9001-send-email.sh            | 40 ++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 68 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 9d66166f6..bb23b02ca 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -377,6 +377,7 @@ have been specified, in which case default to 'compose'.\n \tCurrently, validation means the following:\n +\n --\n+\t\t*\tInvoke the sendemail-validate hook if present (see linkgit:githooks[5]).\n \t\t*\tWarn of patches that contain lines longer than 998 characters; this\n \t\t\tis due to SMTP limits as described by http://www.ietf.org/rfc/rfc2821.txt.\n --\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 706091a56..b2514f4d4 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -447,6 +447,14 @@ rebase::\n The commits are guaranteed to be listed in the order that they were\n processed by rebase.\n \n+sendemail-validate\n+~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by 'git send-email'.  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+\n \n GIT\n ---\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex eea0a517f..c314cc2b5 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -25,8 +25,9 @@ use Getopt::Long;\n use Text::ParseWords;\n use Term::ANSIColor;\n use File::Temp qw/ tempdir tempfile /;\n-use File::Spec::Functions qw(catfile);\n+use File::Spec::Functions qw(catdir catfile);\n use Error qw(:try);\n+use Cwd qw(abs_path cwd);\n use Git;\n use Git::I18N;\n \n@@ -1737,6 +1738,23 @@ sub unique_email_list {\n \n sub validate_patch {\n \tmy $fn = shift;\n+\n+\tmy $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),\n+\t\t\t\t    'sendemail-validate');\n+\tmy $hook_error;\n+\tif (-x $validate_hook) {\n+\t\tmy $target = abs_path($fn);\n+\t\t# The hook needs a correct cwd and GIT_DIR.\n+\t\tmy $cwd_save = cwd();\n+\t\tchdir($repo->wc_path() or $repo->repo_path())\n+\t\t\tor die(\"chdir: $!\");\n+\t\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n+\t\t$hook_error = \"rejected by sendemail-validate hook\"\n+\t\t\tif system($validate_hook, $target);\n+\t\tchdir($cwd_save) or die(\"chdir: $!\");\n+\t}\n+\treturn $hook_error if $hook_error;\n+\n \topen(my $fh, '<', $fn)\n \t\tor die sprintf(__(\"unable to open %s: %s\\n\"), $fn, $!);\n \twhile (my $line = <$fh>) {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 60a80f60b..f3f238d40 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1913,4 +1913,44 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '\n \ttest_cmp expected-list actual-list\n '\n \n+test_expect_success $PREREQ 'invoke hook' '\n+\tmkdir -p .git/hooks &&\n+\n+\twrite_script .git/hooks/sendemail-validate <<-\\EOF &&\n+\t\t# test that we have the correct environment variable, pwd, and\n+\t\t# argument\n+\t\tcase \"$GIT_DIR\" in\n+\t\t\t*.git)\n+\t\t\t\ttrue\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tfalse\n+\t\t\t\t;;\n+\t\tesac &&\n+\t\ttest -e 0001-add-master.patch &&\n+\t\tgrep \"add master\" \"$1\"\n+\tEOF\n+\n+\tmkdir subdir &&\n+\t(\n+\t\t# Test that it works even if we are not at the root of the\n+\t\t# working tree\n+\t\tcd subdir &&\n+\t\tgit send-email \\\n+\t\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t\t--to=nobody@example.com \\\n+\t\t\t--smtp-server=\"$(pwd)/../fake.sendmail\" \\\n+\t\t\t../0001-add-master.patch &&\n+\n+\t\t# Verify error message when a patch is rejected by the hook\n+\t\tsed -e \"s/add master/x/\" ../0001-add-master.patch >../another.patch &&\n+\t\tgit send-email \\\n+\t\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t\t--to=nobody@example.com \\\n+\t\t\t--smtp-server=\"$(pwd)/../fake.sendmail\" \\\n+\t\t\t../another.patch 2>err\n+\t\ttest_i18ngrep \"rejected by sendemail-validate hook\" err\n+\t)\n+'\n+\n test_done\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319738","messageId":"xmqqk25iyjbr.fsf@gitster.mtv.corp.google.com","threadId":"45939","inReplyTo":"20170512223827.21372-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] send-email: support validate hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T01:55:52Z","receivedAt":"2017-05-15T01:55:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 706091a56..b2514f4d4 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -447,6 +447,14 @@ rebase::\n>  The commits are guaranteed to be listed in the order that they were\n>  processed by rebase.\n>  \n> +sendemail-validate\n> +~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by 'git send-email'.  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> +\n\nI briefly wondered if an interface that allows only the name of the\nfile will close the door to future enhancements, but in the end\ndecided it is OK.  E.g. users may want \"here is the contents, is it\nappropriate to be sent to this and that address?\"---but this hook is\nmeant to enhances/extends the existing \"make sure the contents do\nnot syntactically bust the technical limit of underlying transport\",\nand sits at a place best to do so in the codeflow, i.e. before we\neven start to decide who the recipients of the patch are.  So those\nwho want \"given the contents of the patch and list of the recipients,\ndecide if it is OK to send the patch to all of them\" would be better\nserved by a separate hook, not this one.\n\n> +\twrite_script .git/hooks/sendemail-validate <<-\\EOF &&\n> +\t\t# test that we have the correct environment variable, pwd, and\n> +\t\t# argument\n> +\t\tcase \"$GIT_DIR\" in\n> +\t\t\t*.git)\n> +\t\t\t\ttrue\n> +\t\t\t\t;;\n> +\t\t\t*)\n> +\t\t\t\tfalse\n> +\t\t\t\t;;\n> +\t\tesac &&\n> +\t\ttest -e 0001-add-master.patch &&\n> +\t\tgrep \"add master\" \"$1\"\n> +\tEOF\n\nI'd squash in cosmetic changes to de-dent the contents of\nwrite_script (our standard style is that the body of the script is\nwritten as if the column at which write_script..EOF starts is the\nleft-edge of the page; I think this file already has a few style\nviolations that may want to be updated with a separate clean-up\npatch when the file is quiet), and then de-dent the case arm (case\narms are indented at the same depth as case/esac).  Also I think we\ncare that 0001-add-master.patch not just exists but is a file, so\nI'd do s/test -e/test -f/ while at it.\n\nThanks.\n"},{"id":"319809","messageId":"a3ab7799-f849-2f9a-79d6-a0f4693dc7a7@google.com","threadId":"45939","inReplyTo":"xmqqk25iyjbr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] send-email: support validate hook","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-15T18:10:14Z","receivedAt":"2017-05-15T18:10:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 05/14/2017 06:55 PM, Junio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n>> index 706091a56..b2514f4d4 100644\n>> --- a/Documentation/githooks.txt\n>> +++ b/Documentation/githooks.txt\n>> @@ -447,6 +447,14 @@ rebase::\n>>  The commits are guaranteed to be listed in the order that they were\n>>  processed by rebase.\n>>\n>> +sendemail-validate\n>> +~~~~~~~~~~~~~~~~~~\n>> +\n>> +This hook is invoked by 'git send-email'.  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>> +\n>\n> I briefly wondered if an interface that allows only the name of the\n> file will close the door to future enhancements, but in the end\n> decided it is OK.  E.g. users may want \"here is the contents, is it\n> appropriate to be sent to this and that address?\"---but this hook is\n> meant to enhances/extends the existing \"make sure the contents do\n> not syntactically bust the technical limit of underlying transport\",\n> and sits at a place best to do so in the codeflow, i.e. before we\n> even start to decide who the recipients of the patch are.  So those\n> who want \"given the contents of the patch and list of the recipients,\n> decide if it is OK to send the patch to all of them\" would be better\n> served by a separate hook, not this one.\n>\n>> +\twrite_script .git/hooks/sendemail-validate <<-\\EOF &&\n>> +\t\t# test that we have the correct environment variable, pwd, and\n>> +\t\t# argument\n>> +\t\tcase \"$GIT_DIR\" in\n>> +\t\t\t*.git)\n>> +\t\t\t\ttrue\n>> +\t\t\t\t;;\n>> +\t\t\t*)\n>> +\t\t\t\tfalse\n>> +\t\t\t\t;;\n>> +\t\tesac &&\n>> +\t\ttest -e 0001-add-master.patch &&\n>> +\t\tgrep \"add master\" \"$1\"\n>> +\tEOF\n>\n> I'd squash in cosmetic changes to de-dent the contents of\n> write_script (our standard style is that the body of the script is\n> written as if the column at which write_script..EOF starts is the\n> left-edge of the page; I think this file already has a few style\n> violations that may want to be updated with a separate clean-up\n> patch when the file is quiet), and then de-dent the case arm (case\n> arms are indented at the same depth as case/esac).  Also I think we\n> care that 0001-add-master.patch not just exists but is a file, so\n> I'd do s/test -e/test -f/ while at it.\n>\n> Thanks.\n\nThanks to you too. I agree with these changes - sorry, your previous \nreply must have slipped my mind when I was writing v2.\n"}]}