{"thread":{"id":"59575","subject":"[PATCH] send-email: export patch counters in validate environment","startedAt":"2023-04-11T11:48:20Z","lastAt":"2023-04-20T19:25:38Z","messageCount":21,"participants":["Robin Jarry","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"475141","messageId":"20230411114723.89029-1-robin@jarry.cc","threadId":"59575","inReplyTo":null,"subject":"[PATCH] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-11T11:47:23Z","receivedAt":"2023-04-11T11:48:20Z","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. A hook that\nwants to check some property of the whole series needs to know which\npatch is the final one.\n\nExpose the current and total number of patches to the hook via the\nGIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\nvariables so that both incremental and global validation is possible.\n\nSharing any other state between successive invocations of the validate\nhook must be done via external means. For example, by storing it in\na GIT_DIR/SENDEMAIL_VALIDATE file.\n\nSuggested-by: Phillip Wood <phillip.wood123@gmail.com>\nSigned-off-by: Robin Jarry <robin@jarry.cc>\n---\n\nNotes:\n    Follow up on:\n    https://lore.kernel.org/git/9b8d6cc4-741a-5081-d5de-df0972efec37@gmail.com/\n    \n    As suggested by Phillip, this is a less intrusive change which allows\n    validating whole series. Let me know what you think.\n\n Documentation/githooks.txt | 10 ++++++++++\n git-send-email.perl        |  7 +++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 62908602e7be..55f00e0f6f8c 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -600,6 +600,16 @@ 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+The following environment variables are set when executing the hook.\n+\n+`GIT_SENDEMAIL_PATCH_COUNTER`::\n+\tA 1-based counter incremented by one for every file.\n+\n+`GIT_SENDEMAIL_PATCH_TOTAL`::\n+\tThe total number of files.\n+\n+These variables can be used to validate patch series.\n+\n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbeaad..e962d5e15983 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -795,10 +795,17 @@ sub is_format_patch_arg {\n @files = handle_backup_files(@files);\n \n if ($validate) {\n+\tmy $num = 1;\n+\tmy $num_patches = @files;\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n+\t\t\t$ENV{GIT_SENDEMAIL_PATCH_COUNTER} = \"$num\";\n+\t\t\t$ENV{GIT_SENDEMAIL_PATCH_TOTAL} = \"$num_patches\";\n \t\t\tvalidate_patch($f, $target_xfer_encoding);\n+\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_COUNTER};\n+\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_TOTAL};\n \t\t}\n+\t\t$num += 1;\n \t}\n }\n \n-- \n2.40.0\n\n"},{"id":"475142","messageId":"79a7c59f-6644-1dad-3b85-fe0ca8beb968@gmail.com","threadId":"59575","inReplyTo":"20230411114723.89029-1-robin@jarry.cc","subject":"Re: [PATCH] send-email: export patch counters in validate environment","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-11T13:23:14Z","receivedAt":"2023-04-11T13:23:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Robin\n\nOn 11/04/2023 12:47, 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. A hook that\n> wants to check some property of the whole series needs to know which\n> patch is the final one.\n> \n> Expose the current and total number of patches to the hook via the\n> GIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\n> variables so that both incremental and global validation is possible.\n> \n> Sharing any other state between successive invocations of the validate\n> hook must be done via external means. For example, by storing it in\n> a GIT_DIR/SENDEMAIL_VALIDATE file.\n> \n> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>\n> Signed-off-by: Robin Jarry <robin@jarry.cc>\n> ---\n> \n> Notes:\n>      Follow up on:\n>      https://lore.kernel.org/git/9b8d6cc4-741a-5081-d5de-df0972efec37@gmail.com/\n>      \n>      As suggested by Phillip, this is a less intrusive change which allows\n>      validating whole series. Let me know what you think.\n\nThis is certainly less intrusive, if it does what you need and is \nefficient enough for your needs then I'd be inclined to go with this \napproach.\n\n>   Documentation/githooks.txt | 10 ++++++++++\n>   git-send-email.perl        |  7 +++++++\n>   2 files changed, 17 insertions(+)\n> \n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 62908602e7be..55f00e0f6f8c 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -600,6 +600,16 @@ 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> +The following environment variables are set when executing the hook.\n> +\n> +`GIT_SENDEMAIL_PATCH_COUNTER`::\n> +\tA 1-based counter incremented by one for every file.\n> +\n> +`GIT_SENDEMAIL_PATCH_TOTAL`::\n> +\tThe total number of files.\n> +\n> +These variables can be used to validate patch series.\n> +\n>   fsmonitor-watchman\n>   ~~~~~~~~~~~~~~~~~~\n>   \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 07f2a0cbeaad..e962d5e15983 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -795,10 +795,17 @@ sub is_format_patch_arg {\n>   @files = handle_backup_files(@files);\n>   \n>   if ($validate) {\n> +\tmy $num = 1;\n> +\tmy $num_patches = @files;\n>   \tforeach my $f (@files) {\n>   \t\tunless (-p $f) {\n> +\t\t\t$ENV{GIT_SENDEMAIL_PATCH_COUNTER} = \"$num\";\n> +\t\t\t$ENV{GIT_SENDEMAIL_PATCH_TOTAL} = \"$num_patches\";\n\nWe only need to set this once outside the loop\n\n>   \t\t\tvalidate_patch($f, $target_xfer_encoding);\n> +\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_COUNTER};\n> +\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_TOTAL};\n\nDo we really need to clear these? Certainly not in each iteration of the \nloop I would think.\n\nBest Wishes\n\nPhillip\n>   \t\t}\n> +\t\t$num += 1;\n>   \t}\n>   }\n>   \n\n"},{"id":"475150","messageId":"xmqqbkjubcyc.fsf@gitster.g","threadId":"59575","inReplyTo":"79a7c59f-6644-1dad-3b85-fe0ca8beb968@gmail.com","subject":"Re: [PATCH] send-email: export patch counters in validate environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-11T16:28:27Z","receivedAt":"2023-04-11T16:28:38Z","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> Hi Robin\n>\n> On 11/04/2023 12:47, 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. A hook that\n>> wants to check some property of the whole series needs to know which\n>> patch is the final one.\n>> Expose the current and total number of patches to the hook via the\n>> GIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\n>> variables so that both incremental and global validation is possible.\n\nThe above mentions \"cover letter\" and naturally the readers would\nwonder how it is treated.  When we have 5-patch series with a\nseparate cover letter, do we get TOTAL=6, COUNTER=1 for the cover,\nCOUNTER=2 for [PATCH 1/5], and so on, or do we see TOTAL=5,\nCOUNTER=0 for the cover, counter=1 for [PATCH 1/5], and so on?\n\nThe latter is certainly richer (with the former, the validator that\nwants to act differently on the cover has to somehow figure out if\nthe invocation with COUNTER=1 is seeing the cover or the first\npatch).  The usual and recommended workflow being \"git format-patch\n-o outdir --cover-letter <range>\" followed by \"edit outdir/*\" to\nproofread and edit the cover and the patches, followed by \"git\nsend-email outdir/*.patch\", git-send-email has to guess before\ninvoking the hook.\n\nBut it may be better than forcing the hook to guess, I dunno?\n\nWhichever way we choose, we should\n\n * explain the choice in the proposed log message.  If we choose the\n   \"TOTAL is the number of patches and COUNTER=0 is used for the\n   optional cover letter\" interpretation, we should also explain\n   that we cannot reliably do so and sometimes can guess wrong.  If\n   we choose the \"TOTAL is the number of input files and COUNTER\n   just counts, regardless of the payload\" interpretation, we should\n   also explain that even though we hinted that a series with cover\n   letter can be validated, it is a slight lie, because the hook has\n   to guess if the series has cover and it can guess wrong.\n\n * document what TOTAL and COUNTER means.\n\n>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n>> index 62908602e7be..55f00e0f6f8c 100644\n>> --- a/Documentation/githooks.txt\n>> +++ b/Documentation/githooks.txt\n>> @@ -600,6 +600,16 @@ 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>>   +The following environment variables are set when executing the\n>> hook.\n>> +\n>> +`GIT_SENDEMAIL_PATCH_COUNTER`::\n>> +\tA 1-based counter incremented by one for every file.\n>> +\n>> +`GIT_SENDEMAIL_PATCH_TOTAL`::\n>> +\tThe total number of files.\n>> +\n>> +These variables can be used to validate patch series.\n\nThis may be sufficient documentation to imply we are not treating\ncover letter any differently, by not saying \"patch\" or \"cover\nletter\" but just saying \"file\".  It may be more helpful to be a bit\nmore explicit, though (e.g. \"files\" -> \"input files\", perhaps).\n\n>>   diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 07f2a0cbeaad..e962d5e15983 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -795,10 +795,17 @@ sub is_format_patch_arg {\n>>   @files = handle_backup_files(@files);\n>>     if ($validate) {\n>> +\tmy $num = 1;\n>> +\tmy $num_patches = @files;\n>>   \tforeach my $f (@files) {\n>>   \t\tunless (-p $f) {\n>> +\t\t\t$ENV{GIT_SENDEMAIL_PATCH_COUNTER} = \"$num\";\n>> +\t\t\t$ENV{GIT_SENDEMAIL_PATCH_TOTAL} = \"$num_patches\";\n>\n> We only need to set this once outside the loop\n\nIndeed.\n\n>>   \t\t\tvalidate_patch($f, $target_xfer_encoding);\n>> +\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_COUNTER};\n>> +\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_TOTAL};\n>\n> Do we really need to clear these? Certainly not in each iteration of\n> the loop I would think.\n\nIf we set TOTAL outside, we should clear it outside.  We have to set\nCOUNTER inside, and we could clear it outside, but it probably is\neasier to see the correspondence of set/clear if it is done inside.\n\n>>   \t\t}\n>> +\t\t$num += 1;\n\nWhen you have 3 files to send, and if the last one satisfies \"-p\",\nthe hook will be told \"You are called for 1/3\" and then \"2/3\", and\nwill never hear about \"3/3\", so in practice it will spool the first\ntwo and finish without getting a chance to flush what has been\nspooled.  When you have 3 files to send, and if the first one\nsatisfies \"-p', the hook will be told \"You are called for 2/3\", but\nit is understandable if anybody is tempted to write a hook this way:\n\n\tif COUNTER==1:\n\t\tinitialize the spool area\n\t\trecord TOTAL there\n\telse:\n\t\tread TOTAL recorded in the spool area\n\t\tmake sure TOTAL matches\n\n\tprocess [PATCH COUNTER/TOTAL] individually\n\tif COUNTER==TOTAL:\n\t\tprocess the series as a whole\n\nand for such an invocation of \"git send-email\", the hook will try to\nprocess the second file without having its state fully initialzied\nbecause it never saw the first.\n\nWould these be problems?  I dunno.\n\nThanks for working on the patch, and thanks for a careful review.\n\n"},{"id":"475153","messageId":"CRU2VKKMECFZ.2GSICU4EKKBDR@ringo","threadId":"59575","inReplyTo":"79a7c59f-6644-1dad-3b85-fe0ca8beb968@gmail.com","subject":"Re: [PATCH] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-11T16:47:18Z","receivedAt":"2023-04-11T16:47:39Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Phillip Wood, Apr 11, 2023 at 15:23:\n> This is certainly less intrusive, if it does what you need and is \n> efficient enough for your needs then I'd be inclined to go with this \n> approach.\n\nYes, that is perfectly suitable to validate series. The missing pieces\nof information (e.g. the place where all patches are spooled) can be\neither hard coded or stored in git config entries.\n\n> >   \tforeach my $f (@files) {\n> >   \t\tunless (-p $f) {\n> > +\t\t\t$ENV{GIT_SENDEMAIL_PATCH_COUNTER} = \"$num\";\n> > +\t\t\t$ENV{GIT_SENDEMAIL_PATCH_TOTAL} = \"$num_patches\";\n>\n> We only need to set this once outside the loop\n>\n> >   \t\t\tvalidate_patch($f, $target_xfer_encoding);\n> > +\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_COUNTER};\n> > +\t\t\tdelete $ENV{GIT_SENDEMAIL_PATCH_TOTAL};\n>\n> Do we really need to clear these? Certainly not in each iteration of the \n> loop I would think.\n\nI wanted to keep everything collocated. The time spent setting/unsetting\nthese variables is completely negligible. I don't mind making this more\nstreamlined in a v2.\n\nThanks for the review :)\n"},{"id":"475158","messageId":"CRU3FHOZIRVM.3N8I4FAZ2RGO5@ringo","threadId":"59575","inReplyTo":"xmqqbkjubcyc.fsf@gitster.g","subject":"Re: [PATCH] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-11T17:13:19Z","receivedAt":"2023-04-11T17:13:30Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Hi Junio,\n\nJunio C Hamano, Apr 11, 2023 at 18:28:\n> The above mentions \"cover letter\" and naturally the readers would\n> wonder how it is treated.  When we have 5-patch series with a\n> separate cover letter, do we get TOTAL=6, COUNTER=1 for the cover,\n> COUNTER=2 for [PATCH 1/5], and so on, or do we see TOTAL=5,\n> COUNTER=0 for the cover, counter=1 for [PATCH 1/5], and so on?\n>\n> The latter is certainly richer (with the former, the validator that\n> wants to act differently on the cover has to somehow figure out if\n> the invocation with COUNTER=1 is seeing the cover or the first\n> patch).  The usual and recommended workflow being \"git format-patch\n> -o outdir --cover-letter <range>\" followed by \"edit outdir/*\" to\n> proofread and edit the cover and the patches, followed by \"git\n> send-email outdir/*.patch\", git-send-email has to guess before\n> invoking the hook.\n>\n> But it may be better than forcing the hook to guess, I dunno?\n>\n> Whichever way we choose, we should\n>\n>  * explain the choice in the proposed log message.  If we choose the\n>    \"TOTAL is the number of patches and COUNTER=0 is used for the\n>    optional cover letter\" interpretation, we should also explain\n>    that we cannot reliably do so and sometimes can guess wrong.  If\n>    we choose the \"TOTAL is the number of input files and COUNTER\n>    just counts, regardless of the payload\" interpretation, we should\n>    also explain that even though we hinted that a series with cover\n>    letter can be validated, it is a slight lie, because the hook has\n>    to guess if the series has cover and it can guess wrong.\n>\n>  * document what TOTAL and COUNTER means.\n\nIt is easy enough to differentiate a cover letter from an actual patch\nwith a simple shell test:\n\n    if grep -q \"^diff --git \" \"$1\"; then\n        # patch file\n    else\n        # cover letter\n    fi\n\nIt is probably best to let git-send-email out of the picture. Since\nnothing prevents from sending multiple patch series at once, it may not\nbe possible to determine the proper ordering of all these files. A dumb\n1-based counter will be perfectly suitable.\n\nI will add more details about these two variables, what they mean and\nhow they should be used.\n\n> This may be sufficient documentation to imply we are not treating\n> cover letter any differently, by not saying \"patch\" or \"cover\n> letter\" but just saying \"file\".  It may be more helpful to be a bit\n> more explicit, though (e.g. \"files\" -> \"input files\", perhaps).\n\nIt makes sense to use the \"files\" terminology instead of \"patches\".\nI will update for v2.\n\n> > Do we really need to clear these? Certainly not in each iteration of\n> > the loop I would think.\n>\n> If we set TOTAL outside, we should clear it outside.  We have to set\n> COUNTER inside, and we could clear it outside, but it probably is\n> easier to see the correspondence of set/clear if it is done inside.\n\nGiven the small cost of setting these variables in a perl script, it was\nmy intention to have a clear correspondence between the set/clear\noperations.\n\n> When you have 3 files to send, and if the last one satisfies \"-p\",\n> the hook will be told \"You are called for 1/3\" and then \"2/3\", and\n> will never hear about \"3/3\", so in practice it will spool the first\n> two and finish without getting a chance to flush what has been\n> spooled.  When you have 3 files to send, and if the first one\n> satisfies \"-p', the hook will be told \"You are called for 2/3\", but\n> it is understandable if anybody is tempted to write a hook this way:\n>\n> \tif COUNTER==1:\n> \t\tinitialize the spool area\n> \t\trecord TOTAL there\n> \telse:\n> \t\tread TOTAL recorded in the spool area\n> \t\tmake sure TOTAL matches\n>\n> \tprocess [PATCH COUNTER/TOTAL] individually\n> \tif COUNTER==TOTAL:\n> \t\tprocess the series as a whole\n>\n> and for such an invocation of \"git send-email\", the hook will try to\n> process the second file without having its state fully initialzied\n> because it never saw the first.\n>\n> Would these be problems?  I dunno.\n\nI had thought of this. From perl docs:\n\n    -p  File is a named pipe (FIFO), or Filehandle is a pipe.\n    https://perldoc.perl.org/functions/-p\n\nWhile there is very little chance that users will run git send-email on\nFIFOs, it is a possibility. Reference commit is:\n\n    300913bd448de (\"git-send-email: Accept fifos as well as files\")\n    https://github.com/git/git/commit/300913bd448de\n\nI can run the loop twice to determine the count of non-FIFOs and adjust\nGIT_SENDEMAIL_FILE_TOTAL accordingly.\n\nThanks for the review.\n\nPS: What would you think if I also added a sendemail-validate.sample\nscript in the templates folder? Should I add it in the same commit?\n"},{"id":"475166","messageId":"xmqqile29qpw.fsf@gitster.g","threadId":"59575","inReplyTo":"CRU3FHOZIRVM.3N8I4FAZ2RGO5@ringo","subject":"Re: [PATCH] send-email: export patch counters in validate environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-11T19:14:03Z","receivedAt":"2023-04-11T19:14:21Z","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 is probably best to let git-send-email out of the picture. Since\n> nothing prevents from sending multiple patch series at once, it may not\n> be possible to determine the proper ordering of all these files. A dumb\n> 1-based counter will be perfectly suitable.\n\nAs long as the design decision is clearly documented, I am more than\nfine to make it the user's problem ;-) It is a better design between\nthe two, as the user knows better what their payload is.\n\n> I can run the loop twice to determine the count of non-FIFOs and adjust\n> GIT_SENDEMAIL_FILE_TOTAL accordingly.\n\nIt sounds like a reasonable way out.\n\nThanks.\n\n"},{"id":"475205","messageId":"20230412095434.140754-1-robin@jarry.cc","threadId":"59575","inReplyTo":"20230411114723.89029-1-robin@jarry.cc","subject":"[PATCH v2] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-12T09:54:34Z","receivedAt":"2023-04-12T09:54:47Z","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. A hook that\nwants to check some property of the whole series needs to know which\npatch is the final one.\n\nExpose the current and total number of patches to the hook via the\nGIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\nvariables so that both incremental and global validation is possible.\n\nSharing any other state between successive invocations of the validate\nhook must be done via external means. For example, by storing it in\na git config sendemail.validateWorkdir entry.\n\nAdd a sample script with placeholder validations.\n\nSuggested-by: Phillip Wood <phillip.wood123@gmail.com>\nSigned-off-by: Robin Jarry <robin@jarry.cc>\n---\n\nNotes:\n    v1 -> v2:\n    \n    * Added more details in documentation.\n    * Exclude FIFOs from COUNT/TOTAL\n    * Only set TOTAL once.\n    * Only unset COUNT/TOTAL once.\n    * Add sample hook script.\n\n Documentation/githooks.txt                 | 22 ++++++\n git-send-email.perl                        | 17 ++++-\n templates/hooks--sendemail-validate.sample | 84 ++++++++++++++++++++++\n 3 files changed, 122 insertions(+), 1 deletion(-)\n create mode 100755 templates/hooks--sendemail-validate.sample\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 62908602e7be..c8e55b2613f5 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -600,6 +600,28 @@ 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+The following environment variables are set when executing the hook.\n+\n+`GIT_SENDEMAIL_FILE_COUNTER`::\n+\tA 1-based counter incremented by one for every file holding an e-mail\n+\tto be sent (excluding any FIFOs). This counter does not follow the\n+\tpatch series counter scheme. It will always start at 1 and will end at\n+\tGIT_SENDEMAIL_FILE_TOTAL.\n+\n+`GIT_SENDEMAIL_FILE_TOTAL`::\n+\tThe total number of files that will be sent (excluding any FIFOs). This\n+\tcounter does not follow the patch series counter scheme. It will always\n+\tbe equal to the number of files being sent, whether there is a cover\n+\tletter or not.\n+\n+These variables may for instance be used to validate patch series.\n+\n+The sample `sendemail-validate` hook that comes with Git checks that all sent\n+patches (excluding the cover letter) can be applied on top of the upstream\n+repository default branch without conflicts. Some placeholders are left for\n+additional validation steps to be performed after all patches of a given series\n+have been applied.\n+\n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbeaad..497ec0354790 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -795,11 +795,26 @@ sub is_format_patch_arg {\n @files = handle_backup_files(@files);\n \n if ($validate) {\n+\t# FIFOs can only be read once, exclude them from validation.\n+\tmy @real_files = ();\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n-\t\t\tvalidate_patch($f, $target_xfer_encoding);\n+\t\t\tpush(@real_files, $f);\n \t\t}\n \t}\n+\n+\t# Run the loop once again to avoid gaps in the counter due to FIFO\n+\t# arguments provided by the user.\n+\tmy $num = 1;\n+\tmy $num_files = scalar @real_files;\n+\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n+\tforeach my $r (@real_files) {\n+\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n+\t\tvalidate_patch($r, $target_xfer_encoding);\n+\t\t$num += 1;\n+\t}\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n }\n \n if (@files) {\ndiff --git a/templates/hooks--sendemail-validate.sample b/templates/hooks--sendemail-validate.sample\nnew file mode 100755\nindex 000000000000..c898ee3ab167\n--- /dev/null\n+++ b/templates/hooks--sendemail-validate.sample\n@@ -0,0 +1,84 @@\n+#!/bin/sh\n+\n+# An example hook script to validate a patch (and/or patch series) before\n+# sending it via email.\n+#\n+# The hook should exit with non-zero status after issuing an appropriate\n+# message if it wants to prevent the email(s) from being sent.\n+#\n+# To enable this hook, rename this file to \"sendemail-validate\".\n+#\n+# By default, it will only check that the patch(es) can be applied on top of\n+# the default upstream branch without conflicts. Replace the XXX placeholders\n+# with appropriate checks according to your needs.\n+\n+set -e\n+\n+validate_cover_letter()\n+{\n+\tfile=\"$1\"\n+\t# XXX: Add appropriate checks here (e.g. spell checking).\n+}\n+\n+validate_patch()\n+{\n+\tfile=\"$1\"\n+\t# Ensure that the patch applies without conflicts to the latest\n+\t# upstream version.\n+\tgit am -3 \"$file\" || die \"failed to apply patch on upstream repo\"\n+\t# XXX: Add appropriate checks here (e.g. checkpatch.pl).\n+}\n+\n+validate_series()\n+{\n+\t# XXX: Add appropriate checks here (e.g. quick build, etc.).\n+}\n+\n+die()\n+{\n+\techo \"sendemail-validate: error: $*\" >&2\n+\texit 1\n+}\n+\n+get_work_dir()\n+{\n+\tgit config --get sendemail.validateWorkdir || {\n+\t\t# Initialize it to a temp dir, if unset.\n+\t\tgit config --add sendemail.validateWorkdir \"$(mktemp -d)\"\n+\t\tgit config --get sendemail.validateWorkdir\n+\t}\n+}\n+\n+get_upstream_url()\n+{\n+\tgit config --get remote.origin.url ||\n+\t\tdie \"cannot get remote.origin.url\"\n+}\n+\n+clone_upstream()\n+{\n+\tworkdir=\"$1\"\n+\turl=\"$(get_upstream_url)\"\n+\trm -rf -- \"$workdir\"\n+\tgit clone --depth=1 \"$url\" \"$workdir\" ||\n+\t\tdie \"failed to clone upstream repository\"\n+}\n+\n+# main -------------------------------------------------------------------------\n+\n+workdir=$(get_work_dir)\n+if [ \"$GIT_SENDEMAIL_FILE_COUNTER\" = 1 ]; then\n+\tclone_upstream \"$workdir\"\n+fi\n+cd \"$workdir\"\n+export GIT_DIR=\"$workdir/.git\"\n+\n+if grep -q \"^diff --git \" \"$1\"; then\n+\tvalidate_patch \"$1\"\n+else\n+\tvalidate_cover_letter \"$1\"\n+fi\n+\n+if [ \"$GIT_SENDEMAIL_FILE_COUNTER\" = \"$GIT_SENDEMAIL_FILE_TOTAL\" ]; then\n+\tvalidate_series || die \"patch series was rejected\"\n+fi\n-- \n2.40.0\n\n"},{"id":"475222","messageId":"xmqqfs957zs4.fsf@gitster.g","threadId":"59575","inReplyTo":"20230412095434.140754-1-robin@jarry.cc","subject":"Re: [PATCH v2] send-email: export patch counters in validate environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-12T17:53:31Z","receivedAt":"2023-04-12T17:53:36Z","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>  if ($validate) {\n> +\t# FIFOs can only be read once, exclude them from validation.\n\nIt is very good to see this comment here, as it may not be obvious\nto everybody why we exclude them.\n\n> +validate_cover_letter()\n> +{\n\nSee Documentation/CodingGuidelines, look for \"For shell scripts\nspecifically\" and follow what is in the section.  There may be style\nviolations of other kinds in the file.\n\n> +validate_patch()\n> +{\n> +\tfile=\"$1\"\n> +\t# Ensure that the patch applies without conflicts to the latest\n> +\t# upstream version.\n\nThat comment is true only for the first one.  The second patch needs\nto apply to the upstream plus the first patch, and so on.\n\n> +\tgit am -3 \"$file\" || die \"failed to apply patch on upstream repo\"\n> +\t# XXX: Add appropriate checks here (e.g. checkpatch.pl).\n> +}\n> +\n> +validate_series()\n> +{\n> +\t# XXX: Add appropriate checks here (e.g. quick build, etc.).\n> +}\n> +\n> +die()\n> +{\n> +\techo \"sendemail-validate: error: $*\" >&2\n> +\texit 1\n> +}\n> +\n> +get_work_dir()\n> +{\n> +\tgit config --get sendemail.validateWorkdir || {\n> +\t\t# Initialize it to a temp dir, if unset.\n> +\t\tgit config --add sendemail.validateWorkdir \"$(mktemp -d)\"\n> +\t\tgit config --get sendemail.validateWorkdir\n> +\t}\n> +}\n> +\n> +get_upstream_url()\n> +{\n> +\tgit config --get remote.origin.url ||\n> +\t\tdie \"cannot get remote.origin.url\"\n> +}\n> +\n> +clone_upstream()\n> +{\n> +\tworkdir=\"$1\"\n> +\turl=\"$(get_upstream_url)\"\n> +\trm -rf -- \"$workdir\"\n> +\tgit clone --depth=1 \"$url\" \"$workdir\" ||\n> +\t\tdie \"failed to clone upstream repository\"\n> +}\n\nStyle-wise, it is better to get rid of get_upstream_url and write\nthe above more like\n\n\tworkdir=$1 &&\n\turl=$(git config remote.originurl) &&\n\trm -r -- \"$workdir\" &&\n\tgit clone ... ||\n\tdie \"failed to ...\"\n\nand that would be less error prone (e.g. you will catch failure from\n\"rm\" yourself, instead of relying on \"git clone\" to catch it for\nyou).\n\nIn any case, I would avoid network traffic and extra disk usage if I\nwere showing an example for readers to follow, and would not\nrecommend you to use \"clone\" here, even if it were a shallow one.\n\nIt would make much more sense to create a secondary worktree based\non this repository, with its HEAD detached at the copy of the target\nbranch (e.g. refs/remotes/origin/HEAD), and use that secondary\nworktree, as the necessary objects for \"am -3\" to fall back on are\nmore likely to be found in such a setting, compared to a shallow\nclone that only can have the blobs at the tip.\n\n> +\n> +# main -------------------------------------------------------------------------\n\n> +workdir=$(get_work_dir)\n> +if [ \"$GIT_SENDEMAIL_FILE_COUNTER\" = 1 ]; then\n> +\tclone_upstream \"$workdir\"\n> +fi\n> +cd \"$workdir\"\n> +export GIT_DIR=\"$workdir/.git\"\n\nIt is a good discipline to always set GIT_DIR and GIT_WORK_TREE as a\npair.  Working in a subdirectory of a working tree becomes awkward,\nbecause the presence of the former without the latter signals that\nyour $(cwd) is at the top of the working tree.\n\nBut that is more or less moot, because I am suggesting not to use\n\"git clone\" to prepare the playground and instead use a secondary\nworktree that is attached to the same current repository, so GIT_DIR\nwould be the same as the current one.\n\nAnd because you are \"cd\"ing there anyway, it probably is much\nsimpler to just \n\n    unset GIT_DIR GIT_WORK_TREE\n\nto let the repository discovery mechanism take care of it.\n\n> +if grep -q \"^diff --git \" \"$1\"; then\n> +\tvalidate_patch \"$1\"\n> +else\n> +\tvalidate_cover_letter \"$1\"\n> +fi\n> +\n> +if [ \"$GIT_SENDEMAIL_FILE_COUNTER\" = \"$GIT_SENDEMAIL_FILE_TOTAL\" ]; then\n> +\tvalidate_series || die \"patch series was rejected\"\n> +fi\n\nIt is uneven that validate_patch and validate_cover_letter are\nresponsible for dying when problem is found, but validate_series is\nnot and the caller is made responsible for that.\n\nI would make the caller responsible for dying with message for all\nthree by removing the calls to \"die\" or \"exit\" from the former two,\nif I were showing an example for readers to follow.\n\nOverall, a very well crafted patch, even though little details and\nsome design choices can be improved.\n\nThanks.\n\n"},{"id":"475224","messageId":"CRUZR9IO75B2.3DTTR2N12SQRL@ringo","threadId":"59575","inReplyTo":"xmqqfs957zs4.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-12T18:33:18Z","receivedAt":"2023-04-12T18:33:26Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Hi Junio,\n\nJunio C Hamano, Apr 12, 2023 at 19:53:\n> See Documentation/CodingGuidelines, look for \"For shell scripts\n> specifically\" and follow what is in the section.  There may be style\n> violations of other kinds in the file.\n\nI had missed that one. I'll have a look.\n\n> > +\t# Ensure that the patch applies without conflicts to the latest\n> > +\t# upstream version.\n>\n> That comment is true only for the first one.  The second patch needs\n> to apply to the upstream plus the first patch, and so on.\n\nWill adjust this as well.\n\n> Style-wise, it is better to get rid of get_upstream_url and write\n> the above more like\n>\n> \tworkdir=$1 &&\n> \turl=$(git config remote.originurl) &&\n> \trm -r -- \"$workdir\" &&\n> \tgit clone ... ||\n> \tdie \"failed to ...\"\n>\n> and that would be less error prone (e.g. you will catch failure from\n> \"rm\" yourself, instead of relying on \"git clone\" to catch it for\n> you).\n\nI have added set -e at the beginning of the script, specifically to\navoid the chained && commands which make the code hard to read. If any\ncommand returns/exits with a non-zero status which is not handled by an\nif or by a ||, the shell script will exit.\n\nI can probably get rid of the explicit die statements because of this.\n\n> In any case, I would avoid network traffic and extra disk usage if I\n> were showing an example for readers to follow, and would not\n> recommend you to use \"clone\" here, even if it were a shallow one.\n>\n> It would make much more sense to create a secondary worktree based\n> on this repository, with its HEAD detached at the copy of the target\n> branch (e.g. refs/remotes/origin/HEAD), and use that secondary\n> worktree, as the necessary objects for \"am -3\" to fall back on are\n> more likely to be found in such a setting, compared to a shallow\n> clone that only can have the blobs at the tip.\n\nI have never used secondary worktrees. My original thinking was that the\nlocal repository may not be up to date compared to the upstream and\nrunning git fetch on the local repo seemed like a bad idea. Would there\nbe a proper way to do this with secondary worktree?\n\nThere may not be an elegant generic solution here. $(git config\nremote.origin.url) may not even contain the proper upstream url...\n\nAlso, if I understand how worktrees function, applying patches in\na detached HEAD will create blobs in the current git dir. These will\neventually be garbage collected but I wonder if that could be a problem.\n\n> It is a good discipline to always set GIT_DIR and GIT_WORK_TREE as a\n> pair.  Working in a subdirectory of a working tree becomes awkward,\n> because the presence of the former without the latter signals that\n> your $(cwd) is at the top of the working tree.\n>\n> But that is more or less moot, because I am suggesting not to use\n> \"git clone\" to prepare the playground and instead use a secondary\n> worktree that is attached to the same current repository, so GIT_DIR\n> would be the same as the current one.\n>\n> And because you are \"cd\"ing there anyway, it probably is much\n> simpler to just\n>\n>     unset GIT_DIR GIT_WORK_TREE\n>\n> to let the repository discovery mechanism take care of it.\n\nDepending on whether I use a worktree or not, I will unset these\nvariables.\n\n> It is uneven that validate_patch and validate_cover_letter are\n> responsible for dying when problem is found, but validate_series is\n> not and the caller is made responsible for that.\n>\n> I would make the caller responsible for dying with message for all\n> three by removing the calls to \"die\" or \"exit\" from the former two,\n> if I were showing an example for readers to follow.\n\nAgreed, this is inconsistent. My original intent was to provide more\nexplicit error messages but it is probably not necessary.\n\nAs explained above, `set -e` will force early exit if any command fails\nwithout being explicitly handled. I will remove die/exit calls.\n\n> Overall, a very well crafted patch, even though little details and\n> some design choices can be improved.\n\nThanks for the careful review!\n"},{"id":"475235","messageId":"xmqq7cug96qv.fsf@gitster.g","threadId":"59575","inReplyTo":"CRUZR9IO75B2.3DTTR2N12SQRL@ringo","subject":"Re: [PATCH v2] send-email: export patch counters in validate environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-12T20:37:44Z","receivedAt":"2023-04-12T20:37: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> Also, if I understand how worktrees function, applying patches in\n> a detached HEAD will create blobs in the current git dir. These will\n> eventually be garbage collected but I wonder if that could be a problem.\n\nYou are the user who just ran format-patch to prepare sending out\nthe patches, and you are checking your patches.  Wouldn't you have\nthe blobs already anyways?\n\n> As explained above, `set -e` will force early exit if any command fails\n> without being explicitly handled. I will remove die/exit calls.\n\nI'd rather not to see anybody go in that direction.  \"set -e\" is a\npoor substitute for a properly designed error handling.  Between\n\n\tset -e\n\tcommand A\n\tcommand B\n\tcommand C\n\nand\n\n\tcommand A &&\n\tcommand B &&\n\tcommand C || die message\n\nthe former can only say \"command B\" failed because command B was run\nunder some condition that it did not like, but that is too low level\nan error that is close to the implementation.  As opposed to the\nlatter that can talk about what it _means_ that any one of these\nthree commands did not succeed in the end-user's terms.\n\nThanks.\n"},{"id":"475237","messageId":"CRV2G5WD329G.3ATH750WRKPIF@ringo","threadId":"59575","inReplyTo":"xmqq7cug96qv.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-12T20:39:51Z","receivedAt":"2023-04-12T20:39:58Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Junio C Hamano, Apr 12, 2023 at 22:37:\n> You are the user who just ran format-patch to prepare sending out\n> the patches, and you are checking your patches.  Wouldn't you have\n> the blobs already anyways?\n\nBut these will be new blobs since we are applying the patches from files\ninto another detached branch.\n\n> I'd rather not to see anybody go in that direction.  \"set -e\" is a\n> poor substitute for a properly designed error handling.  Between\n>\n> \tset -e\n> \tcommand A\n> \tcommand B\n> \tcommand C\n>\n> and\n>\n> \tcommand A &&\n> \tcommand B &&\n> \tcommand C || die message\n>\n> the former can only say \"command B\" failed because command B was run\n> under some condition that it did not like, but that is too low level\n> an error that is close to the implementation.  As opposed to the\n> latter that can talk about what it _means_ that any one of these\n> three commands did not succeed in the end-user's terms.\n\nOk.\n"},{"id":"475243","messageId":"20230412214502.90174-1-robin@jarry.cc","threadId":"59575","inReplyTo":"20230412095434.140754-1-robin@jarry.cc","subject":"[PATCH v3] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-12T21:45:02Z","receivedAt":"2023-04-12T21:45:22Z","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. A hook that\nwants to check some property of the whole series needs to know which\npatch is the final one.\n\nExpose the current and total number of patches to the hook via the\nGIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\nvariables so that both incremental and global validation is possible.\n\nSharing any other state between successive invocations of the validate\nhook must be done via external means. For example, by storing it in\na git config sendemail.validateWorktree entry.\n\nAdd a sample script with placeholders for validation.\n\nSuggested-by: Phillip Wood <phillip.wood123@gmail.com>\nSigned-off-by: Robin Jarry <robin@jarry.cc>\n---\n\nNotes:\n    v2 -> v3:\n    \n    * Fixed style in sample script following Documentation/CodingGuidelines\n    * Used git worktree instead of a shallow clone.\n    * Removed set -e and added explicit error handling.\n    * Reworded some comments.\n\n Documentation/githooks.txt                 | 22 +++++++\n git-send-email.perl                        | 17 +++++-\n templates/hooks--sendemail-validate.sample | 71 ++++++++++++++++++++++\n 3 files changed, 109 insertions(+), 1 deletion(-)\n create mode 100755 templates/hooks--sendemail-validate.sample\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 62908602e7be..c8e55b2613f5 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -600,6 +600,28 @@ 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+The following environment variables are set when executing the hook.\n+\n+`GIT_SENDEMAIL_FILE_COUNTER`::\n+\tA 1-based counter incremented by one for every file holding an e-mail\n+\tto be sent (excluding any FIFOs). This counter does not follow the\n+\tpatch series counter scheme. It will always start at 1 and will end at\n+\tGIT_SENDEMAIL_FILE_TOTAL.\n+\n+`GIT_SENDEMAIL_FILE_TOTAL`::\n+\tThe total number of files that will be sent (excluding any FIFOs). This\n+\tcounter does not follow the patch series counter scheme. It will always\n+\tbe equal to the number of files being sent, whether there is a cover\n+\tletter or not.\n+\n+These variables may for instance be used to validate patch series.\n+\n+The sample `sendemail-validate` hook that comes with Git checks that all sent\n+patches (excluding the cover letter) can be applied on top of the upstream\n+repository default branch without conflicts. Some placeholders are left for\n+additional validation steps to be performed after all patches of a given series\n+have been applied.\n+\n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbeaad..497ec0354790 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -795,11 +795,26 @@ sub is_format_patch_arg {\n @files = handle_backup_files(@files);\n \n if ($validate) {\n+\t# FIFOs can only be read once, exclude them from validation.\n+\tmy @real_files = ();\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n-\t\t\tvalidate_patch($f, $target_xfer_encoding);\n+\t\t\tpush(@real_files, $f);\n \t\t}\n \t}\n+\n+\t# Run the loop once again to avoid gaps in the counter due to FIFO\n+\t# arguments provided by the user.\n+\tmy $num = 1;\n+\tmy $num_files = scalar @real_files;\n+\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n+\tforeach my $r (@real_files) {\n+\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n+\t\tvalidate_patch($r, $target_xfer_encoding);\n+\t\t$num += 1;\n+\t}\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n }\n \n if (@files) {\ndiff --git a/templates/hooks--sendemail-validate.sample b/templates/hooks--sendemail-validate.sample\nnew file mode 100755\nindex 000000000000..f6dbaa24ad57\n--- /dev/null\n+++ b/templates/hooks--sendemail-validate.sample\n@@ -0,0 +1,71 @@\n+#!/bin/sh\n+\n+# An example hook script to validate a patch (and/or patch series) before\n+# sending it via email.\n+#\n+# The hook should exit with non-zero status after issuing an appropriate\n+# message if it wants to prevent the email(s) from being sent.\n+#\n+# To enable this hook, rename this file to \"sendemail-validate\".\n+#\n+# By default, it will only check that the patch(es) can be applied on top of\n+# the default upstream branch without conflicts. Replace the XXX placeholders\n+# with appropriate checks according to your needs.\n+\n+validate_cover_letter() {\n+\tfile=\"$1\"\n+\t# XXX: Add appropriate checks (e.g. spell checking).\n+}\n+\n+validate_patch() {\n+\tfile=\"$1\"\n+\t# Ensure that the patch applies without conflicts.\n+\tgit am -3 \"$file\" || return\n+\t# XXX: Add appropriate checks for this patch (e.g. checkpatch.pl).\n+}\n+\n+validate_series() {\n+\t# XXX: Add appropriate checks for the whole series\n+\t# (e.g. quick build, coding style checks, etc.).\n+}\n+\n+get_worktree() {\n+\tif ! git config --get sendemail.validateWorktree\n+\tthen\n+\t\t# Initialize it to a temp dir, if unset.\n+\t\tworktree=$(mktemp --tmpdir -d sendemail-validate.XXXXXXX) &&\n+\t\tgit config --add sendemail.validateWorktree \"$worktree\" &&\n+\t\techo \"$worktree\"\n+\tfi\n+}\n+\n+die() {\n+\techo \"sendemail-validate: error: $*\" >&2\n+\texit 1\n+}\n+\n+# main -------------------------------------------------------------------------\n+\n+worktree=$(get_worktree) &&\n+if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = 1\n+then\n+\t# ignore error if not a worktree\n+\tgit worktree remove -f \"$worktree\" 2>/dev/null || :\n+\techo \"sendemail-validate: worktree $worktree\"\n+\tgit worktree add -fd --checkout \"$worktree\" refs/remotes/origin/HEAD\n+fi || die \"failed to prepare worktree for validation\"\n+\n+unset GIT_DIR GIT_WORK_TREE\n+cd \"$worktree\" &&\n+\n+if grep -q \"^diff --git \" \"$1\"\n+then\n+\tvalidate_patch \"$1\"\n+else\n+\tvalidate_cover_letter \"$1\"\n+fi &&\n+\n+if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = \"$GIT_SENDEMAIL_FILE_TOTAL\"\n+then\n+\tvalidate_series\n+fi\n-- \n2.40.0\n\n"},{"id":"475244","messageId":"xmqqsfd46aca.fsf@gitster.g","threadId":"59575","inReplyTo":"xmqqfs957zs4.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: export patch counters in validate environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-12T21:48:21Z","receivedAt":"2023-04-12T21:48:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Overall, a very well crafted patch, even though little details and\n> some design choices can be improved.\n\nOne thing I forgot to mention.  We probably want some test, perhaps\nadding something like the following to t9001 after we already test\nfor --validate.\n\nThanks.\n\n----------- >8 ---------------------- >8 ---------------------- >8 -----------\nexpected_file_counter_output () {\n\ttotal=$1\n\tcount=0\n\twhile test $count -ne $total\n\tdo\n\t\tcount=$((count + 1)) &&\n\t\techo \"$count/$total\" || return\n\tdone\n}\n\ntest_expect_success $PREREQ '--validate hook allows counting of messages' '\n\ttest_when_finished \"rm my-hooks.log\" &&\n\ttest_config core.hooksPath \"my-hooks\" &&\n\n\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n\t\tnum=$GIT_SENDEMAIL_FILE_COUNTER &&\n\t\ttot=$GIT_SENDEMAIL_FILE_TOTAL &&\n\t\techo \"$num/$tot\" >>my-hooks.log || exit 1\n\tEOF\n\n\t>my-hooks.log &&\n\texpected_file_counter_output 1 >expect &&\n\tgit send-email \\\n\t\t--from=\"Example <from@example.com>\" \\\n\t\t--to=nobody@example.com \\\n\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n\t\t--validate $patches &&\n\ttest_cmp expect my-hooks.log\n'\n"},{"id":"475301","messageId":"240577d5-3412-5a80-c7d9-e3d277869add@gmail.com","threadId":"59575","inReplyTo":"20230412214502.90174-1-robin@jarry.cc","subject":"Re: [PATCH v3] send-email: export patch counters in validate environment","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-13T13:52:46Z","receivedAt":"2023-04-13T13:52:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Robin\n\nOn 12/04/2023 22:45, 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. A hook that\n> wants to check some property of the whole series needs to know which\n> patch is the final one.\n> \n> Expose the current and total number of patches to the hook via the\n> GIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\n> variables so that both incremental and global validation is possible.\n> \n> Sharing any other state between successive invocations of the validate\n> hook must be done via external means. For example, by storing it in\n> a git config sendemail.validateWorktree entry.\n> \n> Add a sample script with placeholders for validation.\n> \n> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>\n> Signed-off-by: Robin Jarry <robin@jarry.cc>\n> ---\n> \n> Notes:\n>      v2 -> v3:\n>      \n>      * Fixed style in sample script following Documentation/CodingGuidelines\n>      * Used git worktree instead of a shallow clone.\n>      * Removed set -e and added explicit error handling.\n>      * Reworded some comments.\n\nI think the documentation and implementation look good, I've left a \ncomment about the example hook below. As Junio has previously mentioned, \nit would be nice to have a test with this patch.\n\n> diff --git a/templates/hooks--sendemail-validate.sample b/templates/hooks--sendemail-validate.sample\n> new file mode 100755\n> index 000000000000..f6dbaa24ad57\n> --- /dev/null\n> +++ b/templates/hooks--sendemail-validate.sample\n> @@ -0,0 +1,71 @@\n> +#!/bin/sh\n> +\n> +# An example hook script to validate a patch (and/or patch series) before\n> +# sending it via email.\n> +#\n> +# The hook should exit with non-zero status after issuing an appropriate\n> +# message if it wants to prevent the email(s) from being sent.\n> +#\n> +# To enable this hook, rename this file to \"sendemail-validate\".\n> +#\n> +# By default, it will only check that the patch(es) can be applied on top of\n> +# the default upstream branch without conflicts. Replace the XXX placeholders\n> +# with appropriate checks according to your needs.\n> +\n> +validate_cover_letter() {\n> +\tfile=\"$1\"\n> +\t# XXX: Add appropriate checks (e.g. spell checking).\n> +}\n> +\n> +validate_patch() {\n> +\tfile=\"$1\"\n> +\t# Ensure that the patch applies without conflicts.\n> +\tgit am -3 \"$file\" || return\n> +\t# XXX: Add appropriate checks for this patch (e.g. checkpatch.pl).\n> +}\n> +\n> +validate_series() {\n> +\t# XXX: Add appropriate checks for the whole series\n> +\t# (e.g. quick build, coding style checks, etc.).\n> +}\n> +\n> +get_worktree() {\n> +\tif ! git config --get sendemail.validateWorktree\n> +\tthen\n> +\t\t# Initialize it to a temp dir, if unset.\n> +\t\tworktree=$(mktemp --tmpdir -d sendemail-validate.XXXXXXX) &&\n> +\t\tgit config --add sendemail.validateWorktree \"$worktree\" &&\n> +\t\techo \"$worktree\"\n> +\tfi\n> +}\n> +\n> +die() {\n> +\techo \"sendemail-validate: error: $*\" >&2\n> +\texit 1\n> +}\n> +\n> +# main -------------------------------------------------------------------------\n> +\n> +worktree=$(get_worktree) &&\n> +if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = 1\n> +then\n> +\t# ignore error if not a worktree\n> +\tgit worktree remove -f \"$worktree\" 2>/dev/null || :\n\nNow that you've got rid of \"set -e\" I don't think we need \"|| :\". I had \nexpected that we'd always create a new worktree on the first patch in a \nseries and remove it after processing the the last patch in the series, \nbut this seems to leave it in place until the next time send-email is \nrun or /tmp gets cleaned up. Also if I've understood it correctly the \nname is set the first time this hook is run, rather than generating a \nnew name for each set of files that is validated.\n\nBest Wishes\n\nPhillip\n\n> +\techo \"sendemail-validate: worktree $worktree\"\n> +\tgit worktree add -fd --checkout \"$worktree\" refs/remotes/origin/HEAD\n> +fi || die \"failed to prepare worktree for validation\"\n> +\n> +unset GIT_DIR GIT_WORK_TREE\n> +cd \"$worktree\" &&\n> +\n> +if grep -q \"^diff --git \" \"$1\"\n> +then\n> +\tvalidate_patch \"$1\"\n> +else\n> +\tvalidate_cover_letter \"$1\"\n> +fi &&\n> +\n> +if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = \"$GIT_SENDEMAIL_FILE_TOTAL\"\n> +then\n> +\tvalidate_series\n> +fi\n"},{"id":"475304","messageId":"CRVOLVOVSVO4.UJJ8JLP8Y69T@ringo","threadId":"59575","inReplyTo":"240577d5-3412-5a80-c7d9-e3d277869add@gmail.com","subject":"Re: [PATCH v3] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-13T14:01:43Z","receivedAt":"2023-04-13T14:01:59Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Hi Phillip,\n\nPhillip Wood, Apr 13, 2023 at 15:52:\n> I think the documentation and implementation look good, I've left a \n> comment about the example hook below. As Junio has previously mentioned, \n> it would be nice to have a test with this patch.\n\nYes, I only got Junio's email after sending v3 :)\n\nThe test case is ready. I was waiting for more comments before sending\na v4.\n\n> > +\tgit worktree remove -f \"$worktree\" 2>/dev/null || :\n>\n> Now that you've got rid of \"set -e\" I don't think we need \"|| :\".\n\nRight.\n\n> I had expected that we'd always create a new worktree on the first\n> patch in a series and remove it after processing the the last patch in\n> the series, but this seems to leave it in place until the next time\n> send-email is run or /tmp gets cleaned up. Also if I've understood it\n> correctly the name is set the first time this hook is run, rather than\n> generating a new name for each set of files that is validated.\n\nI had thought that it would be useful to keep it in case the user wants\nto inspect and resolve issues. I you think it is a problem to leave it,\nI can deleted it after the last patch. In any case, if the user\ninterrupts send-email before it has time to validate all patches, the\nworktree will be left in place.\n\nThanks for the review!\n"},{"id":"475376","messageId":"d40ad39a-7598-02f3-7a5c-46f0d75f34fc@gmail.com","threadId":"59575","inReplyTo":"CRVOLVOVSVO4.UJJ8JLP8Y69T@ringo","subject":"Re: [PATCH v3] send-email: export patch counters in validate environment","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-14T12:58:49Z","receivedAt":"2023-04-14T12:59:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Robin\n\nOn 13/04/2023 15:01, Robin Jarry wrote:\n> Hi Phillip,\n> \n> Phillip Wood, Apr 13, 2023 at 15:52:\n>> I think the documentation and implementation look good, I've left a\n>> comment about the example hook below. As Junio has previously mentioned,\n>> it would be nice to have a test with this patch.\n> \n> Yes, I only got Junio's email after sending v3 :)\n> \n> The test case is ready. I was waiting for more comments before sending\n> a v4.\n\nThat's great, thank you for doing that.\n\n>>> +\tgit worktree remove -f \"$worktree\" 2>/dev/null || :\n>>\n>> Now that you've got rid of \"set -e\" I don't think we need \"|| :\".\n> \n> Right.\n> \n>> I had expected that we'd always create a new worktree on the first\n>> patch in a series and remove it after processing the the last patch in\n>> the series, but this seems to leave it in place until the next time\n>> send-email is run or /tmp gets cleaned up. Also if I've understood it\n>> correctly the name is set the first time this hook is run, rather than\n>> generating a new name for each set of files that is validated.\n> \n> I had thought that it would be useful to keep it in case the user wants\n> to inspect and resolve issues. I you think it is a problem to leave it,\n> I can deleted it after the last patch. In any case, if the user\n> interrupts send-email before it has time to validate all patches, the\n> worktree will be left in place.\n\nI think leaving it in place if there is an error is fine, but it ought \nto clean up after itself if there isn't an error. More serious is that \nwe use mktemp to create the worktree path the first time the hook is run \nand then just keep using that same path forever. It should be creating a \nnew temporary directory with mktemp for each series to avoid clashes \nwith existing entries in /tmp.\n\nBest Wishes\n\nPhillip\n\n> Thanks for the review!\n\n"},{"id":"475384","messageId":"20230414152843.659667-1-robin@jarry.cc","threadId":"59575","inReplyTo":"20230412214502.90174-1-robin@jarry.cc","subject":"[PATCH v4] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-14T15:28:43Z","receivedAt":"2023-04-14T15:30: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. A hook that\nwants to check some property of the whole series needs to know which\npatch is the final one.\n\nExpose the current and total number of patches to the hook via the\nGIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\nvariables so that both incremental and global validation is possible.\n\nSharing any other state between successive invocations of the validate\nhook must be done via external means. For example, by storing it in\na git config sendemail.validateWorktree entry.\n\nAdd a sample script with placeholder validations and update tests to\ncheck that the counters are properly exported.\n\nSuggested-by: Phillip Wood <phillip.wood123@gmail.com>\nSigned-off-by: Robin Jarry <robin@jarry.cc>\n---\n\nNotes:\n    v3 -> v4:\n    \n    * Added test case.\n    * Make sure to always cleanup the temp worktree.\n    * Add configuration knobs to tweak the remote and ref on which to check\n      if the patches apply without conflicts.\n\n Documentation/githooks.txt                 | 22 +++++++\n git-send-email.perl                        | 17 ++++-\n t/t9001-send-email.sh                      | 31 +++++++++\n templates/hooks--sendemail-validate.sample | 77 ++++++++++++++++++++++\n 4 files changed, 146 insertions(+), 1 deletion(-)\n create mode 100755 templates/hooks--sendemail-validate.sample\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 62908602e7be..c8e55b2613f5 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -600,6 +600,28 @@ 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+The following environment variables are set when executing the hook.\n+\n+`GIT_SENDEMAIL_FILE_COUNTER`::\n+\tA 1-based counter incremented by one for every file holding an e-mail\n+\tto be sent (excluding any FIFOs). This counter does not follow the\n+\tpatch series counter scheme. It will always start at 1 and will end at\n+\tGIT_SENDEMAIL_FILE_TOTAL.\n+\n+`GIT_SENDEMAIL_FILE_TOTAL`::\n+\tThe total number of files that will be sent (excluding any FIFOs). This\n+\tcounter does not follow the patch series counter scheme. It will always\n+\tbe equal to the number of files being sent, whether there is a cover\n+\tletter or not.\n+\n+These variables may for instance be used to validate patch series.\n+\n+The sample `sendemail-validate` hook that comes with Git checks that all sent\n+patches (excluding the cover letter) can be applied on top of the upstream\n+repository default branch without conflicts. Some placeholders are left for\n+additional validation steps to be performed after all patches of a given series\n+have been applied.\n+\n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbeaad..497ec0354790 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -795,11 +795,26 @@ sub is_format_patch_arg {\n @files = handle_backup_files(@files);\n \n if ($validate) {\n+\t# FIFOs can only be read once, exclude them from validation.\n+\tmy @real_files = ();\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n-\t\t\tvalidate_patch($f, $target_xfer_encoding);\n+\t\t\tpush(@real_files, $f);\n \t\t}\n \t}\n+\n+\t# Run the loop once again to avoid gaps in the counter due to FIFO\n+\t# arguments provided by the user.\n+\tmy $num = 1;\n+\tmy $num_files = scalar @real_files;\n+\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n+\tforeach my $r (@real_files) {\n+\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n+\t\tvalidate_patch($r, $target_xfer_encoding);\n+\t\t$num += 1;\n+\t}\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n }\n \n if (@files) {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 323952a572d6..7c7625759883 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -2326,6 +2326,37 @@ test_expect_success $PREREQ 'invoke hook' '\n \t)\n '\n \n+expected_file_counter_output () {\n+\ttotal=$1\n+\tcount=0\n+\twhile test $count -ne $total\n+\tdo\n+\t\tcount=$((count + 1)) &&\n+\t\techo \"$count/$total\" || return\n+\tdone\n+}\n+\n+test_expect_success $PREREQ '--validate hook allows counting of messages' '\n+\ttest_when_finished \"rm -rf my-hooks.log\" &&\n+\ttest_config core.hooksPath \"my-hooks\" &&\n+\tmkdir -p my-hooks &&\n+\n+\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n+\t\tnum=$GIT_SENDEMAIL_FILE_COUNTER &&\n+\t\ttot=$GIT_SENDEMAIL_FILE_TOTAL &&\n+\t\techo \"$num/$tot\" >>my-hooks.log || exit 1\n+\tEOF\n+\n+\t>my-hooks.log &&\n+\texpected_file_counter_output 4 >expect &&\n+\tgit send-email \\\n+\t\t--from=\"Example <from@example.com>\" \\\n+\t\t--to=nobody@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--validate -3 --cover-letter --force &&\n+\ttest_cmp expect my-hooks.log\n+'\n+\n test_expect_success $PREREQ 'test that send-email works outside a repo' '\n \tnongit git send-email \\\n \t\t--from=\"Example <nobody@example.com>\" \\\ndiff --git a/templates/hooks--sendemail-validate.sample b/templates/hooks--sendemail-validate.sample\nnew file mode 100755\nindex 000000000000..dcefcc1f992f\n--- /dev/null\n+++ b/templates/hooks--sendemail-validate.sample\n@@ -0,0 +1,77 @@\n+#!/bin/sh\n+\n+# An example hook script to validate a patch (and/or patch series) before\n+# sending it via email.\n+#\n+# The hook should exit with non-zero status after issuing an appropriate\n+# message if it wants to prevent the email(s) from being sent.\n+#\n+# To enable this hook, rename this file to \"sendemail-validate\".\n+#\n+# By default, it will only check that the patch(es) can be applied on top of\n+# the default upstream branch without conflicts in a secondary worktree. After\n+# validation (successful or not) of the last patch of a series, the worktree\n+# will be deleted.\n+#\n+# The following config variables can be set to change the default remote and\n+# remote ref that are used to apply the patches against:\n+#\n+#   sendemail.validateRemote (default: origin)\n+#   sendemail.validateRemoteRef (default: HEAD)\n+#\n+# Replace the TODO placeholders with appropriate checks according to your\n+# needs.\n+\n+validate_cover_letter() {\n+\tfile=\"$1\"\n+\t# TODO: Replace with appropriate checks (e.g. spell checking).\n+\ttrue\n+}\n+\n+validate_patch() {\n+\tfile=\"$1\"\n+\t# Ensure that the patch applies without conflicts.\n+\tgit am -3 \"$file\" || return\n+\t# TODO: Replace with appropriate checks for this patch\n+\t# (e.g. checkpatch.pl).\n+\ttrue\n+}\n+\n+validate_series() {\n+\t# TODO: Replace with appropriate checks for the whole series\n+\t# (e.g. quick build, coding style checks, etc.).\n+\ttrue\n+}\n+\n+# main -------------------------------------------------------------------------\n+\n+if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = 1\n+then\n+\tremote=$(git config --default origin --get sendemail.validateRemote) &&\n+\tref=$(git config --default HEAD --get sendemail.validateRemoteRef) &&\n+\tworktree=$(mktemp --tmpdir -d sendemail-validate.XXXXXXX) &&\n+\tgit worktree add -fd --checkout \"$worktree\" \"refs/remotes/$remote/$ref\" &&\n+\tgit config --replace-all sendemail.validateWorktree \"$worktree\" &&\n+else\n+\tworktree=$(git config --get sendemail.validateWorktree)\n+fi || {\n+\techo \"sendemail-validate: error: failed to prepare worktree\" >&2\n+\texit 1\n+}\n+\n+unset GIT_DIR GIT_WORK_TREE\n+cd \"$worktree\" &&\n+\n+if grep -q \"^diff --git \" \"$1\"\n+then\n+\tvalidate_patch \"$1\"\n+else\n+\tvalidate_cover_letter \"$1\"\n+fi &&\n+\n+if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = \"$GIT_SENDEMAIL_FILE_TOTAL\"\n+then\n+\tgit config --unset-all sendemail.validateWorktree &&\n+\ttrap 'git worktree remove -ff \"$worktree\"' EXIT &&\n+\tvalidate_series\n+fi\n-- \n2.40.0\n\n"},{"id":"475385","messageId":"CRWLJMJDT7JY.1XITLUSCD39O@ringo","threadId":"59575","inReplyTo":"20230414152843.659667-1-robin@jarry.cc","subject":"Re: [PATCH v4] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-14T15:50:23Z","receivedAt":"2023-04-14T15:50:58Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Robin Jarry, Apr 14, 2023 at 17:28:\n> +\tgit config --replace-all sendemail.validateWorktree \"$worktree\" &&\n> +else\n\nDamn, I removed one echo line before sending and didn't test again...\nSorry for the brain fart. I'll send a v5.\n"},{"id":"475386","messageId":"20230414155249.667180-1-robin@jarry.cc","threadId":"59575","inReplyTo":"20230414152843.659667-1-robin@jarry.cc","subject":"[PATCH v5] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-14T15:52:49Z","receivedAt":"2023-04-14T15:53:01Z","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. A hook that\nwants to check some property of the whole series needs to know which\npatch is the final one.\n\nExpose the current and total number of patches to the hook via the\nGIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment\nvariables so that both incremental and global validation is possible.\n\nSharing any other state between successive invocations of the validate\nhook must be done via external means. For example, by storing it in\na git config sendemail.validateWorktree entry.\n\nAdd a sample script with placeholder validations and update tests to\ncheck that the counters are properly exported.\n\nSuggested-by: Phillip Wood <phillip.wood123@gmail.com>\nSigned-off-by: Robin Jarry <robin@jarry.cc>\n---\n\nNotes:\n    v4 -> v5:\n    \n    * Fixed shell syntax error introduced by last minute change.\n    \n    v3 -> v4:\n    \n    * Added test case.\n    * Make sure to always cleanup the temp worktree.\n    * Add configuration knobs to tweak the remote and ref on which to check\n      if the patches apply without conflicts.\n\n Documentation/githooks.txt                 | 22 +++++++\n git-send-email.perl                        | 17 ++++-\n t/t9001-send-email.sh                      | 31 +++++++++\n templates/hooks--sendemail-validate.sample | 77 ++++++++++++++++++++++\n 4 files changed, 146 insertions(+), 1 deletion(-)\n create mode 100755 templates/hooks--sendemail-validate.sample\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 62908602e7be..c8e55b2613f5 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -600,6 +600,28 @@ 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+The following environment variables are set when executing the hook.\n+\n+`GIT_SENDEMAIL_FILE_COUNTER`::\n+\tA 1-based counter incremented by one for every file holding an e-mail\n+\tto be sent (excluding any FIFOs). This counter does not follow the\n+\tpatch series counter scheme. It will always start at 1 and will end at\n+\tGIT_SENDEMAIL_FILE_TOTAL.\n+\n+`GIT_SENDEMAIL_FILE_TOTAL`::\n+\tThe total number of files that will be sent (excluding any FIFOs). This\n+\tcounter does not follow the patch series counter scheme. It will always\n+\tbe equal to the number of files being sent, whether there is a cover\n+\tletter or not.\n+\n+These variables may for instance be used to validate patch series.\n+\n+The sample `sendemail-validate` hook that comes with Git checks that all sent\n+patches (excluding the cover letter) can be applied on top of the upstream\n+repository default branch without conflicts. Some placeholders are left for\n+additional validation steps to be performed after all patches of a given series\n+have been applied.\n+\n fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 07f2a0cbeaad..497ec0354790 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -795,11 +795,26 @@ sub is_format_patch_arg {\n @files = handle_backup_files(@files);\n \n if ($validate) {\n+\t# FIFOs can only be read once, exclude them from validation.\n+\tmy @real_files = ();\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n-\t\t\tvalidate_patch($f, $target_xfer_encoding);\n+\t\t\tpush(@real_files, $f);\n \t\t}\n \t}\n+\n+\t# Run the loop once again to avoid gaps in the counter due to FIFO\n+\t# arguments provided by the user.\n+\tmy $num = 1;\n+\tmy $num_files = scalar @real_files;\n+\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n+\tforeach my $r (@real_files) {\n+\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n+\t\tvalidate_patch($r, $target_xfer_encoding);\n+\t\t$num += 1;\n+\t}\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n }\n \n if (@files) {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 323952a572d6..7c7625759883 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -2326,6 +2326,37 @@ test_expect_success $PREREQ 'invoke hook' '\n \t)\n '\n \n+expected_file_counter_output () {\n+\ttotal=$1\n+\tcount=0\n+\twhile test $count -ne $total\n+\tdo\n+\t\tcount=$((count + 1)) &&\n+\t\techo \"$count/$total\" || return\n+\tdone\n+}\n+\n+test_expect_success $PREREQ '--validate hook allows counting of messages' '\n+\ttest_when_finished \"rm -rf my-hooks.log\" &&\n+\ttest_config core.hooksPath \"my-hooks\" &&\n+\tmkdir -p my-hooks &&\n+\n+\twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n+\t\tnum=$GIT_SENDEMAIL_FILE_COUNTER &&\n+\t\ttot=$GIT_SENDEMAIL_FILE_TOTAL &&\n+\t\techo \"$num/$tot\" >>my-hooks.log || exit 1\n+\tEOF\n+\n+\t>my-hooks.log &&\n+\texpected_file_counter_output 4 >expect &&\n+\tgit send-email \\\n+\t\t--from=\"Example <from@example.com>\" \\\n+\t\t--to=nobody@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--validate -3 --cover-letter --force &&\n+\ttest_cmp expect my-hooks.log\n+'\n+\n test_expect_success $PREREQ 'test that send-email works outside a repo' '\n \tnongit git send-email \\\n \t\t--from=\"Example <nobody@example.com>\" \\\ndiff --git a/templates/hooks--sendemail-validate.sample b/templates/hooks--sendemail-validate.sample\nnew file mode 100755\nindex 000000000000..ad2f9a86473d\n--- /dev/null\n+++ b/templates/hooks--sendemail-validate.sample\n@@ -0,0 +1,77 @@\n+#!/bin/sh\n+\n+# An example hook script to validate a patch (and/or patch series) before\n+# sending it via email.\n+#\n+# The hook should exit with non-zero status after issuing an appropriate\n+# message if it wants to prevent the email(s) from being sent.\n+#\n+# To enable this hook, rename this file to \"sendemail-validate\".\n+#\n+# By default, it will only check that the patch(es) can be applied on top of\n+# the default upstream branch without conflicts in a secondary worktree. After\n+# validation (successful or not) of the last patch of a series, the worktree\n+# will be deleted.\n+#\n+# The following config variables can be set to change the default remote and\n+# remote ref that are used to apply the patches against:\n+#\n+#   sendemail.validateRemote (default: origin)\n+#   sendemail.validateRemoteRef (default: HEAD)\n+#\n+# Replace the TODO placeholders with appropriate checks according to your\n+# needs.\n+\n+validate_cover_letter() {\n+\tfile=\"$1\"\n+\t# TODO: Replace with appropriate checks (e.g. spell checking).\n+\ttrue\n+}\n+\n+validate_patch() {\n+\tfile=\"$1\"\n+\t# Ensure that the patch applies without conflicts.\n+\tgit am -3 \"$file\" || return\n+\t# TODO: Replace with appropriate checks for this patch\n+\t# (e.g. checkpatch.pl).\n+\ttrue\n+}\n+\n+validate_series() {\n+\t# TODO: Replace with appropriate checks for the whole series\n+\t# (e.g. quick build, coding style checks, etc.).\n+\ttrue\n+}\n+\n+# main -------------------------------------------------------------------------\n+\n+if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = 1\n+then\n+\tremote=$(git config --default origin --get sendemail.validateRemote) &&\n+\tref=$(git config --default HEAD --get sendemail.validateRemoteRef) &&\n+\tworktree=$(mktemp --tmpdir -d sendemail-validate.XXXXXXX) &&\n+\tgit worktree add -fd --checkout \"$worktree\" \"refs/remotes/$remote/$ref\" &&\n+\tgit config --replace-all sendemail.validateWorktree \"$worktree\"\n+else\n+\tworktree=$(git config --get sendemail.validateWorktree)\n+fi || {\n+\techo \"sendemail-validate: error: failed to prepare worktree\" >&2\n+\texit 1\n+}\n+\n+unset GIT_DIR GIT_WORK_TREE\n+cd \"$worktree\" &&\n+\n+if grep -q \"^diff --git \" \"$1\"\n+then\n+\tvalidate_patch \"$1\"\n+else\n+\tvalidate_cover_letter \"$1\"\n+fi &&\n+\n+if test \"$GIT_SENDEMAIL_FILE_COUNTER\" = \"$GIT_SENDEMAIL_FILE_TOTAL\"\n+then\n+\tgit config --unset-all sendemail.validateWorktree &&\n+\ttrap 'git worktree remove -ff \"$worktree\"' EXIT &&\n+\tvalidate_series\n+fi\n-- \n2.40.0\n\n"},{"id":"475753","messageId":"CS1TOE1MCMH0.2OMA9UHSDG7RC@ringo","threadId":"59575","inReplyTo":"20230414155249.667180-1-robin@jarry.cc","subject":"Re: [PATCH v5] send-email: export patch counters in validate environment","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2023-04-20T19:16:05Z","receivedAt":"2023-04-20T19:16:15Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Robin Jarry, Apr 14, 2023 at 17:52:\n> diff --git a/templates/hooks--sendemail-validate.sample b/templates/hooks--sendemail-validate.sample\n> new file mode 100755\n> index 000000000000..ad2f9a86473d\n> --- /dev/null\n> +++ b/templates/hooks--sendemail-validate.sample\n> @@ -0,0 +1,77 @@\n> +#!/bin/sh\n> +\n> +# An example hook script to validate a patch (and/or patch series) before\n> +# sending it via email.\n> +#\n> +# The hook should exit with non-zero status after issuing an appropriate\n> +# message if it wants to prevent the email(s) from being sent.\n> +#\n> +# To enable this hook, rename this file to \"sendemail-validate\".\n> +#\n> +# By default, it will only check that the patch(es) can be applied on top of\n> +# the default upstream branch without conflicts in a secondary worktree. After\n> +# validation (successful or not) of the last patch of a series, the worktree\n> +# will be deleted.\n> +#\n> +# The following config variables can be set to change the default remote and\n> +# remote ref that are used to apply the patches against:\n> +#\n> +#   sendemail.validateRemote (default: origin)\n> +#   sendemail.validateRemoteRef (default: HEAD)\n> +#\n> +# Replace the TODO placeholders with appropriate checks according to your\n> +# needs.\n> +\n> +validate_cover_letter() {\n> +\tfile=\"$1\"\n> +\t# TODO: Replace with appropriate checks (e.g. spell checking).\n> +\ttrue\n> +}\n> +\n> +validate_patch() {\n> +\tfile=\"$1\"\n> +\t# Ensure that the patch applies without conflicts.\n> +\tgit am -3 \"$file\" || return\n> +\t# TODO: Replace with appropriate checks for this patch\n> +\t# (e.g. checkpatch.pl).\n> +\ttrue\n\nHey folks,\n\nI had an idea after sending v5. Instead of leaving TODO placeholders, it\nwould be nicer to introduce other git config sendemail.validate* options\nspecific to this hook template so that users can directly use it without\nany modifications simply by setting options in their local clone:\n\n    git config sendemail.validatePatchCmd 'tools/checkpatch.sh'\n    git config sendemail.validateSeriesCmd 'make tests lint'\n\nAnd reuse these commands if defined in the hook template.\n\nWhat do you think?\n"},{"id":"475755","messageId":"xmqqttxaz784.fsf@gitster.g","threadId":"59575","inReplyTo":"CS1TOE1MCMH0.2OMA9UHSDG7RC@ringo","subject":"Re: [PATCH v5] send-email: export patch counters in validate environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T19:25:31Z","receivedAt":"2023-04-20T19:25:38Z","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> I had an idea after sending v5. Instead of leaving TODO placeholders, it\n> would be nicer to introduce other git config sendemail.validate* options\n> specific to this hook template so that users can directly use it without\n> any modifications simply by setting options in their local clone:\n>\n>     git config sendemail.validatePatchCmd 'tools/checkpatch.sh'\n>     git config sendemail.validateSeriesCmd 'make tests lint'\n>\n> And reuse these commands if defined in the hook template.\n\nI do not think it is worth it, as they have to write the scripts or\ncopy them from elsewhere, *and* need to make the configuration\nvariables, *and* make the sample hook into a real one with such an\napproach.\n\nAfter all, it is merely a sample.  There is a value in keeping it\nsimple so that users can learn what the structure should look like.\nThen users will come up with something much better suited for their\nown use case themselves.\n\nThanks.\n"}]}