{"thread":{"id":"59525","subject":"[PATCH RESEND] hooks: add sendemail-validate-series","startedAt":"2023-04-02T18:56:50Z","lastAt":"2023-04-11T15:58:50Z","messageCount":18,"participants":["Robin Jarry","Eric Sunshine","Phillip Wood","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474666","messageId":"20230402185635.302653-1-robin@jarry.cc","threadId":"59525","inReplyTo":null,"subject":"[PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-02T18:56:35Z","receivedAt":"2023-04-02T18:56:50Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"When sending patch series (with a cover-letter or not)\nsendemail-validate is called with every email/patch file independently\nfrom the others. When one of the patches depends on a previous one, it\nmay not be possible to use this hook in a meaningful way.\n\nChanging sendemail-validate to take all patches as arguments would break\nbackward compatibility.\n\nAdd a new hook to allow validating patch series instead of patch by\npatch. The patch files are provided in the hook script standard input.\n\ngit hook run cannot be used since it closes the hook standard input. Run\nthe hook directly.\n\nSigned-off-by: Robin Jarry <robin@jarry.cc>\n---\nRebased on 140b9478dad5 (\"The sixth batch\")\n\n Documentation/git-send-email.txt |  1 +\n Documentation/githooks.txt       | 17 +++++++++++++\n git-send-email.perl              | 42 ++++++++++++++++++++++++++++++++\n 3 files changed, 60 insertions(+)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 765b2df8530d..45113b928593 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -438,6 +438,7 @@ have been specified, in which case default to 'compose'.\n +\n --\n \t\t*\tInvoke the sendemail-validate hook if present (see linkgit:githooks[5]).\n+\t\t*\tInvoke the sendemail-validate-series hook if present (see linkgit:githooks[5]).\n \t\t*\tWarn of patches that contain lines longer than\n \t\t\t998 characters unless a suitable transfer encoding\n \t\t\t('auto', 'base64', or 'quoted-printable') is used;\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 62908602e7be..b81783235111 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -600,6 +600,23 @@ 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+sendemail-validate-series\n+~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by linkgit:git-send-email[1].  It allows performing\n+validation on a complete patch series at once, instead of patch by patch with\n+`sendemail-validate`.\n+\n+`sendemail-validate-series` takes no arguments, but for each e-mail to be sent\n+it receives on standard input a line of the format:\n+\n+  <patch-file> LF\n+\n+where `<patch-file>` is a name of a file that holds an e-mail to be sent,\n+\n+If the hook exits with non-zero status, `git send-email` will abort before\n+sending any e-mails.\n+\n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbeaad..bec4d0f4ab47 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -800,6 +800,7 @@ sub is_format_patch_arg {\n \t\t\tvalidate_patch($f, $target_xfer_encoding);\n \t\t}\n \t}\n+\tvalidate_patch_series(@files)\n }\n \n if (@files) {\n@@ -2125,6 +2126,47 @@ sub validate_patch {\n \treturn;\n }\n \n+sub validate_patch_series {\n+\tmy @files = @_;\n+\n+\tunless ($repo) {\n+\t\treturn;\n+\t}\n+\n+\tmy $hook_name = 'sendemail-validate-series';\n+\tmy $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');\n+\trequire File::Spec;\n+\tmy $validate_hook = File::Spec->catfile($hooks_path, $hook_name);\n+\tmy $hook_error;\n+\tunless (-x $validate_hook) {\n+\t\treturn;\n+\t}\n+\n+\t# The hook needs a correct cwd and GIT_DIR.\n+\trequire Cwd;\n+\tmy $cwd_save = Cwd::getcwd();\n+\tchdir($repo->wc_path() or $repo->repo_path()) or die(\"chdir: $!\");\n+\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n+\t# cannot use git hook run, it closes stdin before forking the hook\n+\topen(my $stdin, \"|-\", $validate_hook) or die(\"fork: $!\");\n+\tchdir($cwd_save) or die(\"chdir: $!\");\n+\tfor my $fn (@files) {\n+\t\tunless (-p $fn) {\n+\t\t\t$fn = Cwd::abs_path($fn);\n+\t\t\t$stdin->print(\"$fn\\n\");\n+\t\t}\n+\t}\n+\tclose($stdin); # calls waitpid\n+\tif ($? & 0x7f) {\n+\t\tmy $sig = $? & 0x7f;\n+\t\tdie(\"fatal: hook $hook_name killed by signal $sig\");\n+\t} elsif ($? >> 8) {\n+\t\tmy $err = $? >> 8;\n+\t\tdie(\"fatal: hook $hook_name rejected patch series (exit code $err)\");\n+\t}\n+\treturn;\n+}\n+\n sub handle_backup {\n \tmy ($last, $lastlen, $file, $known_suffix) = @_;\n \tmy ($suffix, $skip);\n-- \n2.39.2\n\n"},{"id":"474671","messageId":"CAPig+cSAvLcVTYF21ksyuhMtFxQkg71ktGd7tw595VRq1kcyvA@mail.gmail.com","threadId":"59525","inReplyTo":"20230402185635.302653-1-robin@jarry.cc","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-04-03T00:17:54Z","receivedAt":"2023-04-03T00:18:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 2, 2023 at 3:10 PM Robin Jarry <robin@jarry.cc> wrote:\n> When sending patch series (with a cover-letter or not)\n> sendemail-validate is called with every email/patch file independently\n> from the others. When one of the patches depends on a previous one, it\n> may not be possible to use this hook in a meaningful way.\n>\n> Changing sendemail-validate to take all patches as arguments would break\n> backward compatibility.\n>\n> Add a new hook to allow validating patch series instead of patch by\n> patch. The patch files are provided in the hook script standard input.\n\nIt's not clear from this description whether the pathnames of the\npatches are fed to the hook on stdin or if the patch contents are fed\non stdin.\n\n> git hook run cannot be used since it closes the hook standard input. Run\n> the hook directly.\n>\n> Signed-off-by: Robin Jarry <robin@jarry.cc>\n> ---\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> @@ -600,6 +600,23 @@ the name of the file that holds the e-mail to be sent.  Exiting with a\n> +sendemail-validate-series\n> +~~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by linkgit:git-send-email[1].  It allows performing\n> +validation on a complete patch series at once, instead of patch by patch with\n> +`sendemail-validate`.\n> +\n> +`sendemail-validate-series` takes no arguments, but for each e-mail to be sent\n> +it receives on standard input a line of the format:\n> +\n> +  <patch-file> LF\n> +\n> +where `<patch-file>` is a name of a file that holds an e-mail to be sent,\n\nThis does a better job than the commit message of explaining that\nstdin receives the names of the patches rather than the content of the\npatches themselves. It's a nit, but it might be even clearer to say\nthat <patch-file> is the _pathname_ of the file rather than merely the\n_name_.\n\n> +If the hook exits with non-zero status, `git send-email` will abort before\n> +sending any e-mails.\n\nIt was a bit startling to see this spelled \"e-mail\" rather than\n\"email\", the latter of which is used far more frequently in the\ndocumentation. However, \"e-mail\" does indeed appear in githooks.txt\nmore frequently than \"email\", so the use of \"e-mail\" here is probably\nfine.\n\nI doubt that any of the above comments warrant a reroll.\n"},{"id":"474680","messageId":"66099367-4ea0-7d2a-a089-7a88e27f695e@dunelm.org.uk","threadId":"59525","inReplyTo":"20230402185635.302653-1-robin@jarry.cc","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-03T14:09:32Z","receivedAt":"2023-04-03T14:10:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Robin\n\nOn 02/04/2023 19:56, Robin Jarry wrote:\n> When sending patch series (with a cover-letter or not)\n> sendemail-validate is called with every email/patch file independently\n> from the others. When one of the patches depends on a previous one, it\n> may not be possible to use this hook in a meaningful way.\n> \n> Changing sendemail-validate to take all patches as arguments would break\n> backward compatibility.\n> \n> Add a new hook to allow validating patch series instead of patch by\n> patch. The patch files are provided in the hook script standard input.\n> \n> git hook run cannot be used since it closes the hook standard input. Run\n> the hook directly.\n\nI've left some comments about this lower down as \"git hook run\" now has \na --to-stdin option.\n\n> Signed-off-by: Robin Jarry <robin@jarry.cc>\n> ---\n\n> +sendemail-validate-series\n> +~~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by linkgit:git-send-email[1].  It allows performing\n> +validation on a complete patch series at once, instead of patch by patch with\n> +`sendemail-validate`.\n> +\n> +`sendemail-validate-series` takes no arguments, but for each e-mail to be sent\n> +it receives on standard input a line of the format:\n> +\n> +  <patch-file> LF\n\nUsually git commands that produce or consume paths either use quoted \npaths terminated by LF or unquoted paths terminated by NUL. That way \nthere is no ambiguity when a path contains LF.\n\n> +where `<patch-file>` is a name of a file that holds an e-mail to be sent,\n> +\n> +If the hook exits with non-zero status, `git send-email` will abort before\n> +sending any e-mails.\n> +\n>   fsmonitor-watchman\n>   ~~~~~~~~~~~~~~~~~~\n>   \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 07f2a0cbeaad..bec4d0f4ab47 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -800,6 +800,7 @@ sub is_format_patch_arg {\n>   \t\t\tvalidate_patch($f, $target_xfer_encoding);\n>   \t\t}\n>   \t}\n> +\tvalidate_patch_series(@files)\n\nThis happens fairly early, before the user has had a chance to edit the \npatches and before we have added all the recipient and in-reply-to \nheaders to the patch files. Would it be more useful to validate what \nwill actually be sent?\n\n>   }\n>   \n>   if (@files) {\n> @@ -2125,6 +2126,47 @@ sub validate_patch {\n>   \treturn;\n>   }\n>   \n> +sub validate_patch_series {\n> +\tmy @files = @_;\n> +\n> +\tunless ($repo) {\n> +\t\treturn;\n> +\t}\n> +\n> +\tmy $hook_name = 'sendemail-validate-series';\n> +\tmy $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');\n\n$hooks_path maybe a relative path, this is problematic because we change \ndirectory before executing the hook (using \"git hook run\" would avoid this).\n\n> +\trequire File::Spec;\n> +\tmy $validate_hook = File::Spec->catfile($hooks_path, $hook_name);\n> +\tmy $hook_error;\n> +\tunless (-x $validate_hook) {\n> +\t\treturn;\n> +\t}\n> +\n> +\t# The hook needs a correct cwd and GIT_DIR.\n> +\trequire Cwd;\n> +\tmy $cwd_save = Cwd::getcwd();\n> +\tchdir($repo->wc_path() or $repo->repo_path()) or die(\"chdir: $!\");\n> +\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n\nThis looks like it is copied from the existing code but why do we need \nto do this? I'm struggling to come up with a scenario where \"git \nsend-email\" can find the repository but the hook cannot.\n\n> +\t# cannot use git hook run, it closes stdin before forking the hook\n> +\topen(my $stdin, \"|-\", $validate_hook) or die(\"fork: $!\");\n\nThis passes $validate_hook to the shell to execute which is not what we \nwant as it will split the hook path on whitespace etc. I think it would \nbe better to use \"git hook run --to-stdin\" (see 0414b3891c (hook: \nsupport a --to-stdin=<path> option, 2023-02-08))\n\nBest Wishes\n\nPhillip\n\n> +\tchdir($cwd_save) or die(\"chdir: $!\");\n> +\tfor my $fn (@files) {\n> +\t\tunless (-p $fn) {\n> +\t\t\t$fn = Cwd::abs_path($fn);\n> +\t\t\t$stdin->print(\"$fn\\n\");\n> +\t\t}\n> +\t}\n> +\tclose($stdin); # calls waitpid\n> +\tif ($? & 0x7f) {\n> +\t\tmy $sig = $? & 0x7f;\n> +\t\tdie(\"fatal: hook $hook_name killed by signal $sig\");\n> +\t} elsif ($? >> 8) {\n> +\t\tmy $err = $? >> 8;\n> +\t\tdie(\"fatal: hook $hook_name rejected patch series (exit code $err)\");\n> +\t}\n> +\treturn;\n> +}\n> +\n>   sub handle_backup {\n>   \tmy ($last, $lastlen, $file, $known_suffix) = @_;\n>   \tmy ($suffix, $skip);\n"},{"id":"474681","messageId":"CRN7096DENCQ.1HF4OQ0ZD4HFP@ringo","threadId":"59525","inReplyTo":"66099367-4ea0-7d2a-a089-7a88e27f695e@dunelm.org.uk","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-03T14:32:50Z","receivedAt":"2023-04-03T14:33:13Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Hi Phillip,\n\nPhillip Wood, Apr 03, 2023 at 16:09:\n> > +  <patch-file> LF\n>\n> Usually git commands that produce or consume paths either use quoted \n> paths terminated by LF or unquoted paths terminated by NUL. That way \n> there is no ambiguity when a path contains LF.\n\nI had never imagined that some twisted mind would insert LF in path\nnames but since nothing will forbid it, I agree that it is\na possibility.\n\nI'm not sure what you mean by quoted paths, you mean adding literal\ndouble quotes before printing them to the hook stdin? That means the\nhook needs to handle de-quoting after reading, right?\n\n> > diff --git a/git-send-email.perl b/git-send-email.perl\n> > index 07f2a0cbeaad..bec4d0f4ab47 100755\n> > --- a/git-send-email.perl\n> > +++ b/git-send-email.perl\n> > @@ -800,6 +800,7 @@ sub is_format_patch_arg {\n> >   \t\t\tvalidate_patch($f, $target_xfer_encoding);\n> >   \t\t}\n> >   \t}\n> > +\tvalidate_patch_series(@files)\n>\n> This happens fairly early, before the user has had a chance to edit the \n> patches and before we have added all the recipient and in-reply-to \n> headers to the patch files. Would it be more useful to validate what \n> will actually be sent?\n\nI agree that it would be better. I added the check here to be in line\nwith the existing sendemail-validate hook. I could move it after edition\nand header finalization but then we would need to move\nsendemail-validate as well for consistency. What do you think?\n\n> > +\t# The hook needs a correct cwd and GIT_DIR.\n> > +\trequire Cwd;\n> > +\tmy $cwd_save = Cwd::getcwd();\n> > +\tchdir($repo->wc_path() or $repo->repo_path()) or die(\"chdir: $!\");\n> > +\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n>\n> This looks like it is copied from the existing code but why do we need \n> to do this? I'm struggling to come up with a scenario where \"git \n> send-email\" can find the repository but the hook cannot.\n\nAgain, for consistency I assumed it would be best to keep the code\nsimilar in both hooks. If you think it is safe to skip that check, I'll\nremove it gladly.\n\n> > +\t# cannot use git hook run, it closes stdin before forking the hook\n> > +\topen(my $stdin, \"|-\", $validate_hook) or die(\"fork: $!\");\n>\n> This passes $validate_hook to the shell to execute which is not what we \n> want as it will split the hook path on whitespace etc. I think it would \n> be better to use \"git hook run --to-stdin\" (see 0414b3891c (hook: \n> support a --to-stdin=<path> option, 2023-02-08))\n\nAh that's a nice addition. I'll add that in v2.\n\nThanks for reviewing!\n"},{"id":"474683","messageId":"9429a246-4921-c9f6-0318-834c86b35898@dunelm.org.uk","threadId":"59525","inReplyTo":"CRN7096DENCQ.1HF4OQ0ZD4HFP@ringo","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-03T15:20:30Z","receivedAt":"2023-04-03T15:20:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Robin\n\nOn 03/04/2023 15:32, Robin Jarry wrote:\n> Hi Phillip,\n> \n> Phillip Wood, Apr 03, 2023 at 16:09:\n>>> +  <patch-file> LF\n>>\n>> Usually git commands that produce or consume paths either use quoted\n>> paths terminated by LF or unquoted paths terminated by NUL. That way\n>> there is no ambiguity when a path contains LF.\n> \n> I had never imagined that some twisted mind would insert LF in path\n> names but since nothing will forbid it, I agree that it is\n> a possibility.\n> \n> I'm not sure what you mean by quoted paths, you mean adding literal\n> double quotes before printing them to the hook stdin? That means the\n> hook needs to handle de-quoting after reading, right?\n\nI meant quoted in the same way that diff and ls-files etc quote paths \nthat contain control characters - see quote_c_style() in quote.c if \nyou're interested in the details. It looks like Git.pm can unquote paths \nbut has no code to quote them. It is probably easiest to use NUL \ntermination here - bash and zsh can read NUL terminated lines and so can \nany scripting language.\n\n>>> diff --git a/git-send-email.perl b/git-send-email.perl\n>>> index 07f2a0cbeaad..bec4d0f4ab47 100755\n>>> --- a/git-send-email.perl\n>>> +++ b/git-send-email.perl\n>>> @@ -800,6 +800,7 @@ sub is_format_patch_arg {\n>>>    \t\t\tvalidate_patch($f, $target_xfer_encoding);\n>>>    \t\t}\n>>>    \t}\n>>> +\tvalidate_patch_series(@files)\n>>\n>> This happens fairly early, before the user has had a chance to edit the\n>> patches and before we have added all the recipient and in-reply-to\n>> headers to the patch files. Would it be more useful to validate what\n>> will actually be sent?\n> \n> I agree that it would be better. I added the check here to be in line\n> with the existing sendemail-validate hook. I could move it after edition\n> and header finalization but then we would need to move\n> sendemail-validate as well for consistency. What do you think?\n\nThat would be my inclination but I'm only an occasional send-email user. \nThe downside is that the user may edit a patch only for it to be \nrejected but we could offer for them to edit it again rather than just \nthrowing their work away.\n\n>>> +\t# The hook needs a correct cwd and GIT_DIR.\n>>> +\trequire Cwd;\n>>> +\tmy $cwd_save = Cwd::getcwd();\n>>> +\tchdir($repo->wc_path() or $repo->repo_path()) or die(\"chdir: $!\");\n>>> +\tlocal $ENV{\"GIT_DIR\"} = $repo->repo_path();\n>>\n>> This looks like it is copied from the existing code but why do we need\n>> to do this? I'm struggling to come up with a scenario where \"git\n>> send-email\" can find the repository but the hook cannot.\n> \n> Again, for consistency I assumed it would be best to keep the code\n> similar in both hooks. If you think it is safe to skip that check, I'll\n> remove it gladly.\n\nI suspect it is safe, hopefully someone will speak up if I'm mistaken. A \nwhile ago rebase stopped setting these variables when running an \"exec\" \ncommand as they were causing problems (see 434e0636db (sequencer: do not \nexport GIT_DIR and GIT_WORK_TREE for 'exec', 2021-12-04))\n\n>>> +\t# cannot use git hook run, it closes stdin before forking the hook\n>>> +\topen(my $stdin, \"|-\", $validate_hook) or die(\"fork: $!\");\n>>\n>> This passes $validate_hook to the shell to execute which is not what we\n>> want as it will split the hook path on whitespace etc. I think it would\n>> be better to use \"git hook run --to-stdin\" (see 0414b3891c (hook:\n>> support a --to-stdin=<path> option, 2023-02-08))\n> \n> Ah that's a nice addition. I'll add that in v2.\n\nThat's great, Best Wishes\n\nPhillip\n\n> Thanks for reviewing!\n"},{"id":"474684","messageId":"xmqqo7o59dlz.fsf@gitster.g","threadId":"59525","inReplyTo":"66099367-4ea0-7d2a-a089-7a88e27f695e@dunelm.org.uk","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-03T15:42:16Z","receivedAt":"2023-04-03T15:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>>   diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 07f2a0cbeaad..bec4d0f4ab47 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -800,6 +800,7 @@ sub is_format_patch_arg {\n>>   \t\t\tvalidate_patch($f, $target_xfer_encoding);\n>>   \t\t}\n>>   \t}\n>> +\tvalidate_patch_series(@files)\n>\n> This happens fairly early, before the user has had a chance to edit\n> the patches and before we have added all the recipient and in-reply-to\n> headers to the patch files. Would it be more useful to validate what\n> will actually be sent?\n\nI actually think the original intent was to catch errors in the part\nof the file that can mechanically be created before letting the user\nspend time on editing, without realizing that a later stage will be\nrejected due to the auto-generated (e.g. came from a commit object)\nstuff.  I do not know why we need another hook to do pretty much the\nsame thing as the existing one (which could be taught to spool and\nthen the last round to validate, in addition to each step rejecting\nincoming one as needed), but at least calling it there would be very\nmuch in line with the existing one, I would say.\n\nThanks for a careful review.\n"},{"id":"474696","messageId":"CRNAOLZTJKEN.3G96UM2HO763B@ringo","threadId":"59525","inReplyTo":"xmqqo7o59dlz.fsf@gitster.g","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-03T17:25:42Z","receivedAt":"2023-04-03T17:25:50Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Junio C Hamano, Apr 03, 2023 at 17:42:\n> I do not know why we need another hook to do pretty much the same\n> thing as the existing one (which could be taught to spool and then the\n> last round to validate, in addition to each step rejecting incoming\n> one as needed), but at least calling it there would be very much in\n> line with the existing one, I would say.\n\nIf for example the validation would require trying to apply patches on\ntop of another branch in a temp repository, you would need to know the\nnumber of patches and be able to determine whether you need to reset the\nbranch (patch 1/N) before applying. For that you would need to parse the\ncontents of the patches. This is not the end of the world but I assumed\nthat it would be easier to handle with a hook that fires once with all\npatch files.\n\nAnother option would be to change sendemail-validate to be called only\nonce with all patches. That would be the ideal solution since the\nexisting hook is not always usable with series. But that would be\na breaking change. I personally don't mind a small breakage like this\nbut I don't know what is the project's policy.\n"},{"id":"474718","messageId":"CRNH5FOB91JE.14CZEA494X002@ringo","threadId":"59525","inReplyTo":"66099367-4ea0-7d2a-a089-7a88e27f695e@dunelm.org.uk","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-03T22:29:47Z","receivedAt":"2023-04-03T22:30:18Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Phillip Wood, Apr 03, 2023 at 16:09:\n> Usually git commands that produce or consume paths either use quoted \n> paths terminated by LF or unquoted paths terminated by NUL. That way \n> there is no ambiguity when a path contains LF.\n\nThinking again about that. The probability that a file path name\ngenerated by git-format-patch would contain LF is close to zero.\nHowever, reading per line is more natural and more in line with other\nhooks that read from stdin. Having that single hook separating stuff in\nstdin with NUL bytes is weird from a user point of view. Don't you think\nit would be an acceptable limitation?\n"},{"id":"474726","messageId":"xmqq7cus4m0b.fsf@gitster.g","threadId":"59525","inReplyTo":"CRNH5FOB91JE.14CZEA494X002@ringo","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-03T22:52:04Z","receivedAt":"2023-04-03T22:52:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robin Jarry\" <robin@jarry.cc> writes:\n\n> Thinking again about that. The probability that a file path name\n> generated by git-format-patch would contain LF is close to zero.\n\nClose to zero is very different from absolutely zero, and in the\ncase of format-patch generated patches, I think it is absolutely\nzero.  At least, that was the case back when I designed and\nimplemented it, and I do not think I accepted a patch to break it\nover the years.\n\nBut \"git send-email\" can be fed a list of files and even a directory\n(and enumerate files in it).  The filenames are under end-users'\ncontrol in this case, so \"close to zero\" has absolutely no relevance.\nIf the end user means to feed you such a file, they can do so 100%\nof the time.\n\nIf we support such a file is a different issue.  A good rule of\nthumb to decide if it is reasonable is to see if the main command\nalready works with such filenames, e.g.\n\n    $ git format-patch -2\n    0001-foo.txt\n    0002-bar.txt\n    $ mv 0001-foo.txt '0001-fo\n    > o.txt'\n    $ mkdir dir\n    $ mv 000[12]*.txt dir/.\n\nmay prepare two patch files that can be sent via send-email.  One\nfile (the first one) is deliberately given a filename with LF in\nit.  Does send-email work on it correctly if you did e.g.\n\n    $ git send-email dir/000[12]*.txt\n\nor something silly like\n\n    $ git send-email dir\n\nor does it already choke on the first file because of the filename?\n\n\n"},{"id":"474728","messageId":"CRNHSC3H2B6C.UCSDE4Y6ET4A@ringo","threadId":"59525","inReplyTo":"xmqq7cus4m0b.fsf@gitster.g","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-03T22:59:42Z","receivedAt":"2023-04-03T22:59:54Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Junio C Hamano, Apr 04, 2023 at 00:52:\n> Close to zero is very different from absolutely zero, and in the\n> case of format-patch generated patches, I think it is absolutely\n> zero.  At least, that was the case back when I designed and\n> implemented it, and I do not think I accepted a patch to break it\n> over the years.\n>\n> But \"git send-email\" can be fed a list of files and even a directory\n> (and enumerate files in it).  The filenames are under end-users'\n> control in this case, so \"close to zero\" has absolutely no relevance.\n> If the end user means to feed you such a file, they can do so 100%\n> of the time.\n\nOk that's a fair point. Even though I am having a hard time believing\nsomeone would do such a thing :D\n\n> If we support such a file is a different issue.  A good rule of\n> thumb to decide if it is reasonable is to see if the main command\n> already works with such filenames, e.g.\n>\n>     $ git format-patch -2\n>     0001-foo.txt\n>     0002-bar.txt\n>     $ mv 0001-foo.txt '0001-fo\n>     > o.txt'\n>     $ mkdir dir\n>     $ mv 000[12]*.txt dir/.\n>\n> may prepare two patch files that can be sent via send-email.  One\n> file (the first one) is deliberately given a filename with LF in\n> it.  Does send-email work on it correctly if you did e.g.\n>\n>     $ git send-email dir/000[12]*.txt\n>\n> or something silly like\n>\n>     $ git send-email dir\n>\n> or does it already choke on the first file because of the filename?\n\nIt seems to work with both. I guess, NUL bytes separation it is then...\n"},{"id":"474805","messageId":"xmqqbkk3z9p9.fsf@gitster.g","threadId":"59525","inReplyTo":"CRNHSC3H2B6C.UCSDE4Y6ET4A@ringo","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-04T20:14:26Z","receivedAt":"2023-04-04T20:14:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robin Jarry\" <robin@jarry.cc> writes:\n\n>> it.  Does send-email work on it correctly if you did e.g.\n>>\n>>     $ git send-email dir/000[12]*.txt\n>>\n>> or something silly like\n>>\n>>     $ git send-email dir\n>>\n>> or does it already choke on the first file because of the filename?\n>\n> It seems to work with both. I guess, NUL bytes separation it is then...\n\nFeeding the filenames as the command line arguments would have been\nmuch simpler X-<, but either NUL termination or c-quoting the\nfilenames would be needed _if_ we want to support crazy folks who\nfeed us such garbage filenames.  Letting hook scripts understand NUL\ntermination is a chore and it still is debatable if it is reasonable\nto support, though.  I'd say it would be sufficient to just declare\n\"files whose name has LF in it is not given to the hook, ever\" and\nusers would avoid such a filename if they care.\n\nThanks.\n\n\n"},{"id":"474828","messageId":"CROOKNR29PDV.1WIGA6219L1C6@ringo","threadId":"59525","inReplyTo":"xmqqbkk3z9p9.fsf@gitster.g","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-05T08:31:28Z","receivedAt":"2023-04-05T08:31:38Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Junio C Hamano, Apr 04, 2023 at 22:14:\n> Feeding the filenames as the command line arguments would have been\n> much simpler X-<,\n\nJust a thought. Instead of adding another hook, wouldn't it be better to\nadd an option --validate-series (git config sendemail.validateSeries)\nthat would change the behaviour of the sendemail-validate hook? Calling\nit with all patch files as command line arguments only once instead of\nonce per file.\n\nI know I was concerned with the max size of the command line args but is\nthere really a chance that we hit that maximum? On my system, it is\n2097152 bytes. Even with a 1000 patches series with 1000 bytes\nfilenames, we wouldn't hit the limit.\n\nThis way, we support any crap filename that the user may send and we\ndon't add a new hook which basically does the same thing than the\nexisting one.\n\nThoughts?\n"},{"id":"474891","messageId":"xmqqwn2qt2x3.fsf@gitster.g","threadId":"59525","inReplyTo":"CROOKNR29PDV.1WIGA6219L1C6@ringo","subject":"Re: [PATCH RESEND] hooks: add sendemail-validate-series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T21:49:44Z","receivedAt":"2023-04-05T21:49:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robin Jarry\" <robin@jarry.cc> writes:\n\n> Just a thought. Instead of adding another hook, wouldn't it be better to\n> add an option --validate-series (git config sendemail.validateSeries)\n> that would change the behaviour of the sendemail-validate hook? Calling\n> it with all patch files as command line arguments only once instead of\n> once per file.\n\nI do not see much upside in doing so.  Instead of having to write a\nnew validate-series hook script to store it under a new name, users\ncan update an existing validate-patch hook script to take a batch of\npatches in one go.  Then the user now has to set a new configuration\nvariable.  Because the expected way to feed the hook script is *not*\nper invocation but depends solely on how the script is written, a\ncommand line option would not make much sense.  Not having to rename\nthe updated validate-patch script to validate-series might be a\nsmall win, but I do not think it is a compelling reason to take that\napproach.\n\n> I know I was concerned with the max size of the command line args but is\n> there really a chance that we hit that maximum? On my system, it is\n> 2097152 bytes. Even with a 1000 patches series with 1000 bytes\n> filenames, we wouldn't hit the limit.\n>\n> This way, we support any crap filename that the user may send and we\n> don't add a new hook which basically does the same thing than the\n> existing one.\n\nBetween \"we may exceed command line argument limit\" (which by the\nway is way lower on certain systems than what you expect, IIRC) and\n\"the user may throw us a file with LF in its name\", I'd find it\nsimpler to punt on the latter and tell them \"if it hurts, don't do\nit\".  As long as the limitation is clearly documented, I'd say it is\nOK.\n\nThanks.\n"},{"id":"474895","messageId":"20230405231305.96996-1-robin@jarry.cc","threadId":"59525","inReplyTo":"20230402185635.302653-1-robin@jarry.cc","subject":"[PATCH v2] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-05T23:13:05Z","receivedAt":"2023-04-05T23:13:25Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"When sending patch series (with a cover-letter or not)\nsendemail-validate is called with every email/patch file independently\nfrom the others. When one of the patches depends on a previous one, it\nmay not be possible to use this hook in a meaningful way.\n\nChanging sendemail-validate to take all patches as arguments would break\nbackward compatibility.\n\nAdd a new hook to allow validating patch series instead of patch by\npatch. The patch files are provided in the hook script standard input,\none per line. Patch file names that contain LF characters are *not*\nvalidated.\n\nSigned-off-by: Robin Jarry <robin@jarry.cc>\n---\n\nNotes:\n    v1 -> v2:\n    \n    - Use `git hook run --to-stdin` with a temp file.\n    - Skip validation (with an explicit warning) for patch file names that\n      contain newline characters.\n    - Updated docs.\n\n Documentation/git-send-email.txt |  1 +\n Documentation/githooks.txt       | 19 +++++++++++++++++\n git-send-email.perl              | 36 ++++++++++++++++++++++++++++++++\n 3 files changed, 56 insertions(+)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 765b2df8530d..45113b928593 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -438,6 +438,7 @@ have been specified, in which case default to 'compose'.\n +\n --\n \t\t*\tInvoke the sendemail-validate hook if present (see linkgit:githooks[5]).\n+\t\t*\tInvoke the sendemail-validate-series hook if present (see linkgit:githooks[5]).\n \t\t*\tWarn of patches that contain lines longer than\n \t\t\t998 characters unless a suitable transfer encoding\n \t\t\t('auto', 'base64', or 'quoted-printable') is used;\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 62908602e7be..0e8573c6c116 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -600,6 +600,25 @@ 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+sendemail-validate-series\n+~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by linkgit:git-send-email[1].  It allows performing\n+validation on a complete patch series at once, instead of patch by patch with\n+`sendemail-validate`.\n+\n+`sendemail-validate-series` takes no arguments.  For each e-mail to be sent,\n+it receives on standard input a line of the format:\n+\n+  <patch-file> LF\n+\n+where '<patch-file>' is an absolute path to a file that holds an e-mail to be\n+sent.  Any '<patch-file>' that contains a 'LF' character will *not* be fed to\n+the hook and an explicit warning will be printed instead.\n+\n+If the hook exits with non-zero status, `git send-email` will abort before\n+sending any e-mails.\n+\n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbeaad..b29050e14c06 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -800,6 +800,7 @@ sub is_format_patch_arg {\n \t\t\tvalidate_patch($f, $target_xfer_encoding);\n \t\t}\n \t}\n+\tvalidate_patch_series(@files)\n }\n \n if (@files) {\n@@ -2125,6 +2126,41 @@ sub validate_patch {\n \treturn;\n }\n \n+sub validate_patch_series {\n+\tmy @files = @_;\n+\n+\tunless ($repo) {\n+\t\treturn;\n+\t}\n+\trequire File::Temp;\n+\tmy $tmp = File::Temp->new(\n+\t\tTEMPLATE => \"sendemail-series.XXXXXXXX\",\n+\t\tUNLINK => 1,\n+\t);\n+\tfor my $fn (@files) {\n+\t\tunless (-p $fn) {\n+\t\t\t$fn = Cwd::abs_path($fn);\n+\t\t\tif ($fn =~ /\\n/) {\n+\t\t\t\t$fn =~ s/\\n/'\\\\n'/g;\n+\t\t\t\tprintf STDERR __(\"warning: file name contains '\\\\n': %s. Skipping validation.\\n\"), $fn;\n+\t\t\t} else {\n+\t\t\t\t$tmp->print(\"$fn\\n\");\n+\t\t\t}\n+\t\t}\n+\t}\n+\tmy $hook_name = \"sendemail-validate-series\";\n+\tmy @cmd = (\"git\", \"hook\", \"run\", \"--ignore-missing\",\n+\t\t   \"--to-stdin\", $tmp->filename, $hook_name, \"--\");\n+\tmy $hook_error = system_or_msg(\\@cmd, undef, \"@cmd\");\n+\tif ($hook_error) {\n+\t\t$hook_error = sprintf(\n+\t\t    __(\"fatal: series rejected by %s hook\\n%s\\nwarning: no patches were sent\\n\"),\n+\t\t    $hook_name, $hook_error);\n+\t\tdie $hook_error;\n+\t}\n+\treturn;\n+}\n+\n sub handle_backup {\n \tmy ($last, $lastlen, $file, $known_suffix) = @_;\n \tmy ($suffix, $skip);\n-- \n2.40.0\n\n"},{"id":"474924","messageId":"230406.868rf5tkzs.gmgdl@evledraar.gmail.com","threadId":"59525","inReplyTo":"20230405231305.96996-1-robin@jarry.cc","subject":"Re: [PATCH v2] hooks: add sendemail-validate-series","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:56:30Z","receivedAt":"2023-04-06T09:32:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 06 2023, Robin Jarry wrote:\n\n> When sending patch series (with a cover-letter or not)\n> sendemail-validate is called with every email/patch file independently\n> from the others. When one of the patches depends on a previous one, it\n> may not be possible to use this hook in a meaningful way.\n>\n> Changing sendemail-validate to take all patches as arguments would break\n> backward compatibility.\n>\n> Add a new hook to allow validating patch series instead of patch by\n> patch. The patch files are provided in the hook script standard input,\n> one per line. Patch file names that contain LF characters are *not*\n> validated.\n>\n> Signed-off-by: Robin Jarry <robin@jarry.cc>\n> ---\n>\n> Notes:\n>     v1 -> v2:\n>     \n>     - Use `git hook run --to-stdin` with a temp file.\n>     - Skip validation (with an explicit warning) for patch file names that\n>       contain newline characters.\n>     - Updated docs.\n\nAt first glance I thought this was a re-roll of the patches to include\nthe headers in the output, but I see that's a *different* series:\nhttps://lore.kernel.org/git/5758ffc7-eb8c-4c16-d226-dd882cb2406b@amd.com/\n\nBut that was waiting on the --to-stdin I recently added, which you're\nusing here (good!).\n\nBut it seems to me that if that's integrated we'd end up with yet\nanother interface, or not? Is this proposing that we use this interface\ninstead for that use-case?\n\n>  Documentation/git-send-email.txt |  1 +\n>  Documentation/githooks.txt       | 19 +++++++++++++++++\n>  git-send-email.perl              | 36 ++++++++++++++++++++++++++++++++\n>  3 files changed, 56 insertions(+)\n>\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index 765b2df8530d..45113b928593 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -438,6 +438,7 @@ have been specified, in which case default to 'compose'.\n>  +\n>  --\n>  \t\t*\tInvoke the sendemail-validate hook if present (see linkgit:githooks[5]).\n> +\t\t*\tInvoke the sendemail-validate-series hook if present (see linkgit:githooks[5]).\n>  \t\t*\tWarn of patches that contain lines longer than\n>  \t\t\t998 characters unless a suitable transfer encoding\n>  \t\t\t('auto', 'base64', or 'quoted-printable') is used;\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 62908602e7be..0e8573c6c116 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -600,6 +600,25 @@ 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> +sendemail-validate-series\n> +~~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by linkgit:git-send-email[1].  It allows performing\n> +validation on a complete patch series at once, instead of patch by patch with\n> +`sendemail-validate`.\n> +\n> +`sendemail-validate-series` takes no arguments.  For each e-mail to be sent,\n> +it receives on standard input a line of the format:\n> +\n> +  <patch-file> LF\n> +\n> +where '<patch-file>' is an absolute path to a file that holds an e-mail to be\n> +sent.  Any '<patch-file>' that contains a 'LF' character will *not* be fed to\n> +the hook and an explicit warning will be printed instead.\n> +\n> +If the hook exits with non-zero status, `git send-email` will abort before\n> +sending any e-mails.\n> +\n>  fsmonitor-watchman\n>  ~~~~~~~~~~~~~~~~~~\n>  \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 07f2a0cbeaad..b29050e14c06 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -800,6 +800,7 @@ sub is_format_patch_arg {\n>  \t\t\tvalidate_patch($f, $target_xfer_encoding);\n>  \t\t}\n>  \t}\n> +\tvalidate_patch_series(@files)\n>  }\n>  \n>  if (@files) {\n> @@ -2125,6 +2126,41 @@ sub validate_patch {\n>  \treturn;\n>  }\n>  \n> +sub validate_patch_series {\n> +\tmy @files = @_;\n> +\n> +\tunless ($repo) {\n> +\t\treturn;\n> +\t}\n> +\trequire File::Temp;\n> +\tmy $tmp = File::Temp->new(\n> +\t\tTEMPLATE => \"sendemail-series.XXXXXXXX\",\n> +\t\tUNLINK => 1,\n> +\t);\n> +\tfor my $fn (@files) {\n> +\t\tunless (-p $fn) {\n> +\t\t\t$fn = Cwd::abs_path($fn);\n\nThe code you mostly copy/pasted does \"require Cwd\", but can't we just\ncombine this with the existing function somehow?\n\n> +\t\t\tif ($fn =~ /\\n/) {\n> +\t\t\t\t$fn =~ s/\\n/'\\\\n'/g;\n> +\t\t\t\tprintf STDERR __(\"warning: file name contains '\\\\n': %s. Skipping validation.\\n\"), $fn;\n> +\t\t\t} else {\n> +\t\t\t\t$tmp->print(\"$fn\\n\");\n> +\t\t\t}\n\nRe feedback from others, I think *if* we keep this we should pass this\n\\0-delimited, that delimiter can be safley used with POSIX filenames.\n\n> +\t\t}\n> +\t}\n> +\tmy $hook_name = \"sendemail-validate-series\";\n> +\tmy @cmd = (\"git\", \"hook\", \"run\", \"--ignore-missing\",\n> +\t\t   \"--to-stdin\", $tmp->filename, $hook_name, \"--\");\n> +\tmy $hook_error = system_or_msg(\\@cmd, undef, \"@cmd\");\n> +\tif ($hook_error) {\n> +\t\t$hook_error = sprintf(\n> +\t\t    __(\"fatal: series rejected by %s hook\\n%s\\nwarning: no patches were sent\\n\"),\n> +\t\t    $hook_name, $hook_error);\n> +\t\tdie $hook_error;\n> +\t}\n> +\treturn;\n> +}\n> +\n>  sub handle_backup {\n>  \tmy ($last, $lastlen, $file, $known_suffix) = @_;\n>  \tmy ($suffix, $skip);\n\nHonestly, I don't really get the use-case. If your 02/N depends on 01/N\ncouldn't your hook just maintain its own state, e.g. in some file\ncreated in the passed $GIT_DIR?\n\nWith the upcoming parallel hooks, I'm also skeptical of a an interface\nthat would preclude validating these in parallel.\n\nI also don't understand the reason for the stdin interface. The\n\"git-send-email\" program itself takes a <file|directory>, so concerns\nabout the files exceeding argument list seem out the window, i.e. we\ncould just pass the dir/files, and as we'd have the same limitations\nhere we should be able to pass the full set of files, no?\n\nI.e. why not a sendemail-validate-all that just takes a dir or file(s)?\n"},{"id":"475138","messageId":"9b8d6cc4-741a-5081-d5de-df0972efec37@gmail.com","threadId":"59525","inReplyTo":"230406.868rf5tkzs.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] hooks: add sendemail-validate-series","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-11T09:58:05Z","receivedAt":"2023-04-11T09:58:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/04/2023 09:56, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Apr 06 2023, Robin Jarry wrote:\n> \n> Honestly, I don't really get the use-case. If your 02/N depends on 01/N\n> couldn't your hook just maintain its own state, e.g. in some file\n> created in the passed $GIT_DIR?\n\nA hook that wants to check some property of the whole series needs to \nknow which patch is the final one. We could pass that via the \nenvironment as we do for external diff commands with \nGIT_DIFF_PATH_COUNTER and GIT_DIFF_PATH_TOTAL.\n\n> With the upcoming parallel hooks, I'm also skeptical of a an interface\n> that would preclude validating these in parallel.\n\nI'd not thought of that, I thought the idea of parallel hooks was to run \ndifferent scripts for the same hook in parallel, not have multiple \ninstances of the same script running simultaneously.\n\n> I also don't understand the reason for the stdin interface. The\n> \"git-send-email\" program itself takes a <file|directory>, so concerns\n> about the files exceeding argument list seem out the window, i.e. we\n> could just pass the dir/files, and as we'd have the same limitations\n> here we should be able to pass the full set of files, no?\n\nNo, not if the user passes something like \"HEAD~1000..\" instead of a \nlist of paths.\n\nBest Wishes\n\nPhillip\n\n> I.e. why not a sendemail-validate-all that just takes a dir or file(s)?\n\n"},{"id":"475140","messageId":"CRTV2BVL0265.1H9OALXHPDZF1@ringo","threadId":"59525","inReplyTo":"9b8d6cc4-741a-5081-d5de-df0972efec37@gmail.com","subject":"Re: [PATCH v2] hooks: add sendemail-validate-series","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-11T10:39:59Z","receivedAt":"2023-04-11T10:40:08Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Phillip Wood, Apr 11, 2023 at 11:58:\n> A hook that wants to check some property of the whole series needs to \n> know which patch is the final one. We could pass that via the \n> environment as we do for external diff commands with \n> GIT_DIFF_PATH_COUNTER and GIT_DIFF_PATH_TOTAL.\n\nThat may be an appropriate solution and it would avoid adding another\nhook. And it would solve the issue of \"\\n\" in filenames.\n\nThe only downside is that you would need to store state in an external\nfile (maybe in GIT_DIR) so that successive calls of the hook script can\npick up where the previous invocation ended.\n\nIt all comes down to ergonomics at this point. I don't mind either\nsolutions as long as validating whole series is possible before sending\nemails.\n"},{"id":"475149","messageId":"xmqqo7nubeby.fsf@gitster.g","threadId":"59525","inReplyTo":"9b8d6cc4-741a-5081-d5de-df0972efec37@gmail.com","subject":"Re: [PATCH v2] hooks: add sendemail-validate-series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-11T15:58:41Z","receivedAt":"2023-04-11T15:58:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> A hook that wants to check some property of the whole series needs to\n> know which patch is the final one. We could pass that via the\n> environment as we do for external diff commands with\n> GIT_DIFF_PATH_COUNTER and GIT_DIFF_PATH_TOTAL.\n\nAhh, I forgot that we added them to deal with \"I am called\nrepeatedly, where is the end of the series of calls?\" question,\nwhich exactly is the same issue.  Glad that you brought it up.\n\nThanks.\n"}]}