{"thread":{"id":"44667","subject":"[RFC PATCH] send-email: allow a custom hook to prevent sending email","startedAt":"2016-12-09T20:35:03Z","lastAt":"2016-12-12T22:39:00Z","messageCount":9,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"307395","messageId":"20161209203449.17940-1-sbeller@google.com","threadId":"44667","inReplyTo":null,"subject":"[RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-09T20:34:49Z","receivedAt":"2016-12-09T20:35:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This custom hook could be used to prevent sending out e.g. patches\nwith change ids or other information that upstream doesn't like to see\nor is not supposed to see.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nMy first perl contribution to Git. :)\n\nMarked as RFC to gauge general interest before writing tests and documentation.\n\nThanks,\nStefan\n\n git-send-email.perl | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex da81be40cb..d3ebf666c3 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -815,6 +815,15 @@ if (!$force) {\n \t\t\t\t. \"Pass --force if you really want to send.\\n\";\n \t\t}\n \t}\n+\tmy @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )\n+\tif( -x $hook[0] ) {\n+\t\tunless( system( @hook ) == 0 )\n+\t\t{\n+\t\t\tdie \"Refusing to send because the patch\\n\\t$f\\n\"\n+\t\t\t\t. \"was refused by the send-email hook.\"\n+\t\t\t\t. \"Pass --force if you really want to send.\\n\";\n+\t\t}\n+\t}\n }\n \n if (defined $sender) {\n-- \n2.11.0.rc2.49.ge1f3b0c.dirty\n\n"},{"id":"307399","messageId":"xmqqfulwraq2.fsf@gitster.mtv.corp.google.com","threadId":"44667","inReplyTo":"20161209203449.17940-1-sbeller@google.com","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-09T22:36:37Z","receivedAt":"2016-12-09T22:36:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This custom hook could be used to prevent sending out e.g. patches\n> with change ids or other information that upstream doesn't like to see\n> or is not supposed to see.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> My first perl contribution to Git. :)\n>\n> Marked as RFC to gauge general interest before writing tests and documentation.\n>\n> Thanks,\n> Stefan\n>\n>  git-send-email.perl | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index da81be40cb..d3ebf666c3 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -815,6 +815,15 @@ if (!$force) {\n>  \t\t\t\t. \"Pass --force if you really want to send.\\n\";\n>  \t\t}\n>  \t}\n> +\tmy @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )\n> +\tif( -x $hook[0] ) {\n> +\t\tunless( system( @hook ) == 0 )\n> +\t\t{\n> +\t\t\tdie \"Refusing to send because the patch\\n\\t$f\\n\"\n> +\t\t\t\t. \"was refused by the send-email hook.\"\n> +\t\t\t\t. \"Pass --force if you really want to send.\\n\";\n> +\t\t}\n> +\t}\n>  }\n\nI doubt that this is the best place to call this hook, because the\ncalled hook does not have access to information that may help it\nmake a better decision.  \n\nFor example, because the hook gets one patchfile at a time, it does\nnot have the entire picture (e.g. \"are you sure you want 01/05,\n02/05, 04/05 and 05/05 without 03/05?\").  For another example, the\nhook does not have access to the decision git-send-email makes on\nvarious \"parameters\", which are computed based on the contents of\nthe patchfiles and command line arguments at this point in the code.\n(e.g. @to, @cc, etc. are computed much later, so you cannot say \"do\nnot send anythnng outside corp by mistake\" with this mechanism).\n\n\n\n\n"},{"id":"307400","messageId":"CAGZ79kaqw=XqrNF5+Ta8CwcD7FyA853UQUdMxHmBAaMHPMHrXg@mail.gmail.com","threadId":"44667","inReplyTo":"xmqqfulwraq2.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-09T22:53:24Z","receivedAt":"2016-12-09T22:53:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 9, 2016 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> I doubt that this is the best place to call this hook, because the\n> called hook does not have access to information that may help it\n> make a better decision.\n\nAs the commit message may elude, I chose this place as it would be\nsufficient for checking for ChangeIds, missing signoffs, or even\nrudimentary check for coding style and commit message line length.\n\n>\n> For example, because the hook gets one patchfile at a time, it does\n> not have the entire picture (e.g. \"are you sure you want 01/05,\n> 02/05, 04/05 and 05/05 without 03/05?\").  For another example, the\n> hook does not have access to the decision git-send-email makes on\n> various \"parameters\", which are computed based on the contents of\n> the patchfiles and command line arguments at this point in the code.\n> (e.g. @to, @cc, etc. are computed much later, so you cannot say \"do\n> not send anythnng outside corp by mistake\" with this mechanism).\n>\n\nSo you are suggesting to\n* have the check later in the game (e.g. just after asking\n   \"Send this email? ([y]es|[n]o|[q]uit|[a]ll): \" as then other information\n  such as additional @to @cc are available.\n* the hook should not just be called one file at a time, but rather\n  we would give all file names via e.g. stdin. With the current code\n  structure this contradicts the first point.\n\nI wonder if we want to have multiple hooks for these different things\nof either looking at the big picture or looking at each in detail.\n\nFor me currently I am only interested in the small picture thing.\n\nStefan\n"},{"id":"307408","messageId":"xmqqshpwpsn8.fsf@gitster.mtv.corp.google.com","threadId":"44667","inReplyTo":"CAGZ79kaqw=XqrNF5+Ta8CwcD7FyA853UQUdMxHmBAaMHPMHrXg@mail.gmail.com","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-09T23:52:27Z","receivedAt":"2016-12-09T23:52:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> So you are suggesting to\n> * have the check later in the game (e.g. just after asking\n>    \"Send this email? ([y]es|[n]o|[q]uit|[a]ll): \" as then other information\n>   such as additional @to @cc are available.\n\nYeah, probably before the loop starts asking that question for each\nmessage.  And hook does not necessarily need to cause the program to\ndie.  The question can be reworded to \"Your hook says no, but do you\nreally want to send it?\",\n"},{"id":"307409","messageId":"CAGZ79kZKify7VLfNp3FXXvc4UOGjdXfUaspNaeW0t5+hWc+cpw@mail.gmail.com","threadId":"44667","inReplyTo":"xmqqshpwpsn8.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-09T23:56:17Z","receivedAt":"2016-12-09T23:56:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> So you are suggesting to\n>> * have the check later in the game (e.g. just after asking\n>>    \"Send this email? ([y]es|[n]o|[q]uit|[a]ll): \" as then other information\n>>   such as additional @to @cc are available.\n>\n> Yeah, probably before the loop starts asking that question for each\n> message.  And hook does not necessarily need to cause the program to\n> die.  The question can be reworded to \"Your hook says no, but do you\n> really want to send it?\",\n\nYou could, but that would be inconsistent with the \"*** SUBJECT ***\"\ntreatment, which currently dies. That could also ask \"do you really want\nto send out an unfinished series\" and continue if the user wants.\n\nI assume we want to be consistent with the existing UI and just ask the\nuser to use force instead?\n"},{"id":"307424","messageId":"20161210091333.rhssskrvzjqnzelb@sigill.intra.peff.net","threadId":"44667","inReplyTo":"20161209203449.17940-1-sbeller@google.com","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-10T09:13:34Z","receivedAt":"2016-12-10T09:13:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 09, 2016 at 12:34:49PM -0800, Stefan Beller wrote:\n\n> My first perl contribution to Git. :)\n\nYes, I have some style suggestions below. :)\n\n> Marked as RFC to gauge general interest before writing tests and\n> documentation.\n\nIt's hard to evaluate without seeing an example of what you'd actually\nwant to put into a hook.\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index da81be40cb..d3ebf666c3 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -815,6 +815,15 @@ if (!$force) {\n>  \t\t\t\t. \"Pass --force if you really want to send.\\n\";\n>  \t\t}\n>  \t}\n> +\tmy @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )\n\nIt's awkward to use a list here, when you just end up accessing\n$hook[0]. You can form the list on the fly when you call system(). You\nalso probably are missing a trailing \"/\".\n\nI'm not sure that $GIT_DIR is consistently set; you probably want to\nlook at \"git rev-parse --git-dir\" here. But I think we make a Git\nrepository object earlier, and you can just do:\n\n  my $hook = join('/', $repo->repo_path(), 'hooks/send-email');\n\nOr you can just do string concatenation:\n\n  my $hook = $repo->repo_path() . '/hooks/send-email';\n\nIf I were writing repo_path myself, I'd probably allow:\n\n  my $hook = $repo->repo_path('hooks/send-email');\n\nlike we do with git_path() in the C code. It wouldn't be hard to add.\n\n> +\tif( -x $hook[0] ) {\n\nFunny whitespace. I think:\n\n  if (-x $hook)\n\nwould be more usual for us.\n\n> +\t\tunless( system( @hook ) == 0 )\n> +\t\t{\n> +\t\t\tdie \"Refusing to send because the patch\\n\\t$f\\n\"\n> +\t\t\t\t. \"was refused by the send-email hook.\"\n> +\t\t\t\t. \"Pass --force if you really want to send.\\n\";\n> +\t\t}\n\nI like \"unless\" as a trailing one-liner conditional for edge cases,\nlike:\n\n   do_this($foo) unless $foo < 5;\n\nbut I think here it just makes things more Perl-ish than our code base\nusually goes for. Probably:\n\n  if (system($hook, $f) != 0) {\n        die ...\n  }\n\nwould be more usual for us (in a more Perl-ish code base I'd probably\nwrite \"system(...) and die ...\").\n\n-Peff\n"},{"id":"307457","messageId":"xmqqtwabo2n1.fsf@gitster.mtv.corp.google.com","threadId":"44667","inReplyTo":"CAGZ79kZKify7VLfNp3FXXvc4UOGjdXfUaspNaeW0t5+hWc+cpw@mail.gmail.com","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-10T22:11:46Z","receivedAt":"2016-12-10T22:11:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> So you are suggesting to\n>>> * have the check later in the game (e.g. just after asking\n>>>    \"Send this email? ([y]es|[n]o|[q]uit|[a]ll): \" as then other information\n>>>   such as additional @to @cc are available.\n>>\n>> Yeah, probably before the loop starts asking that question for each\n>> message.  And hook does not necessarily need to cause the program to\n>> die.  The question can be reworded to \"Your hook says no, but do you\n>> really want to send it?\",\n>\n> You could, but...\n\nYes, \"does not necessarily need to die\" is different from \"hence you\nmust avoid dying\".  Running the hook before you start the loop to\nask for each message merely makes it possible to avoid dying.\n"},{"id":"307534","messageId":"CAGZ79kZVjsJjreKJgnOH1T-k-zfpfqivxw1gGm_dx4GU4tte2Q@mail.gmail.com","threadId":"44667","inReplyTo":"20161210091333.rhssskrvzjqnzelb@sigill.intra.peff.net","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-12T19:35:43Z","receivedAt":"2016-12-12T19:35:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Dec 10, 2016 at 1:13 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Dec 09, 2016 at 12:34:49PM -0800, Stefan Beller wrote:\n>\n>> My first perl contribution to Git. :)\n>\n> Yes, I have some style suggestions below. :)\n>\n>> Marked as RFC to gauge general interest before writing tests and\n>> documentation.\n>\n> It's hard to evaluate without seeing an example of what you'd actually\n> want to put into a hook.\n>\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index da81be40cb..d3ebf666c3 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -815,6 +815,15 @@ if (!$force) {\n>>                               . \"Pass --force if you really want to send.\\n\";\n>>               }\n>>       }\n>> +     my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )\n>\n> It's awkward to use a list here, when you just end up accessing\n> $hook[0]. You can form the list on the fly when you call system(). You\n> also probably are missing a trailing \"/\".\n>\n> I'm not sure that $GIT_DIR is consistently set; you probably want to\n> look at \"git rev-parse --git-dir\" here. But I think we make a Git\n> repository object earlier, and you can just do:\n>\n>   my $hook = join('/', $repo->repo_path(), 'hooks/send-email');\n>\n> Or you can just do string concatenation:\n>\n>   my $hook = $repo->repo_path() . '/hooks/send-email';\n>\n> If I were writing repo_path myself, I'd probably allow:\n>\n>   my $hook = $repo->repo_path('hooks/send-email');\n>\n> like we do with git_path() in the C code. It wouldn't be hard to add.\n>\n>> +     if( -x $hook[0] ) {\n>\n> Funny whitespace. I think:\n>\n>   if (-x $hook)\n>\n> would be more usual for us.\n>\n>> +             unless( system( @hook ) == 0 )\n>> +             {\n>> +                     die \"Refusing to send because the patch\\n\\t$f\\n\"\n>> +                             . \"was refused by the send-email hook.\"\n>> +                             . \"Pass --force if you really want to send.\\n\";\n>> +             }\n>\n> I like \"unless\" as a trailing one-liner conditional for edge cases,\n> like:\n>\n>    do_this($foo) unless $foo < 5;\n>\n> but I think here it just makes things more Perl-ish than our code base\n> usually goes for. Probably:\n>\n>   if (system($hook, $f) != 0) {\n>         die ...\n>   }\n>\n> would be more usual for us (in a more Perl-ish code base I'd probably\n> write \"system(...) and die ...\").\n>\n> -Peff\n\nOh, that is quite some feedback on how not to write perl.\nThanks for pointing out how you would do it. My patch was heavily inspired\nby git-cvsserver.perl:1720 so maybe that is not the best example to follow.\n\nWhile trying to figure out the right place, where to put the actual code, I was\nwondering about the possible interaction with it, e.g. looking at\noutput like this\n\n$ git send-email 00* --cc=list --cc=bmwill --cc=duy --to=jch\n0000-cover-letter.patch\n0001-submodule-use-absolute-path-for-computing-relative-p.patch\n0002-submodule-helper-support-super-prefix.patch\n0003-test-lib-functions.sh-teach-test_commit-C-dir.patch\n0004-worktree-check-if-a-submodule-uses-worktrees.patch\n0005-move-connect_work_tree_and_git_dir-to-dir.h.patch\n0006-submodule-add-absorb-git-dir-function.patch\n(mbox) Adding cc: Stefan Beller <sbeller@google.com> from line 'From:\nStefan Beller <sbeller@google.com>'\n\nFrom: Stefan Beller <sbeller@google.com>\nTo: gitster@pobox.com\nCc: git@vger.kernel.org,\nbmwill@google.com,\npclouds@gmail.com,\nStefan Beller <sbeller@google.com>\nSubject: [PATCHv7 0/6] submodule absorbgitdirs\nDate: Mon, 12 Dec 2016 11:04:29 -0800\nMessage-Id: <20161212190435.10358-1-sbeller@google.com>\nX-Mailer: git-send-email 2.11.0.rc2.49.ge1f3b0c.dirty\n\nSend this email? ([y]es|[n]o|[q]uit|[a]ll):\n\n---\nThis is usually where I just say \"a\" and carry on with life.\nThe hook would ideally reformat the last lines to be\n---\n    X-Mailer: git-send-email 2.11.0.rc2.49.ge1f3b0c.dirty\n    send-email hook thinks it is a bad idea to send out this series:\n    <output of hook, e.g.\n    referencing a typo in patch 5.\n    or a mismatch of patch version numbers>\n\n    Send this email? ([y]es|[n]o|[q]uit|[a]ll):\n---\n\nSo I still need to look around to see the big picture for this\npatch to be implemented ideal.\n"},{"id":"307555","messageId":"xmqqr35c7oxx.fsf@gitster.mtv.corp.google.com","threadId":"44667","inReplyTo":"CAGZ79kZVjsJjreKJgnOH1T-k-zfpfqivxw1gGm_dx4GU4tte2Q@mail.gmail.com","subject":"Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-12T22:38:50Z","receivedAt":"2016-12-12T22:39:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> $ git send-email 00* --cc=list --cc=bmwill --cc=duy --to=jch\n> 0000-cover-letter.patch\n> ...\n> (mbox) Adding cc: Stefan Beller <sbeller@google.com> from line 'From:\n> Stefan Beller <sbeller@google.com>'\n>\n> From: Stefan Beller <sbeller@google.com>\n> To: gitster@pobox.com\n> Cc: git@vger.kernel.org,\n> bmwill@google.com,\n> pclouds@gmail.com,\n> Stefan Beller <sbeller@google.com>\n> Subject: [PATCHv7 0/6] submodule absorbgitdirs\n> Date: Mon, 12 Dec 2016 11:04:29 -0800\n> Message-Id: <20161212190435.10358-1-sbeller@google.com>\n> X-Mailer: git-send-email 2.11.0.rc2.49.ge1f3b0c.dirty\n>\n> Send this email? ([y]es|[n]o|[q]uit|[a]ll):\n>\n> ---\n> This is usually where I just say \"a\" and carry on with life.\n> The hook would ideally reformat the last lines to be\n> ---\n>     X-Mailer: git-send-email 2.11.0.rc2.49.ge1f3b0c.dirty\n>     send-email hook thinks it is a bad idea to send out this series:\n>     <output of hook, e.g.\n>     referencing a typo in patch 5.\n>     or a mismatch of patch version numbers>\n>\n>     Send this email? ([y]es|[n]o|[q]uit|[a]ll):\n> ---\n\nYup, sounds sensible (the \"... thinks it is a bad idea ...\" line may\nprobably not be needed; the hook can say why it is unhappy itself).\n\nThanks.\n\n"}]}