{"thread":{"id":"53313","subject":"[PATCH] bugreport: collect list of populated hooks","startedAt":"2020-04-24T23:38:07Z","lastAt":"2020-05-11T23:45:53Z","messageCount":33,"participants":["Emily Shaffer","Jonathan Nieder","Junio C Hamano","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396206","messageId":"20200424233800.200439-1-emilyshaffer@google.com","threadId":"53313","inReplyTo":null,"subject":"[PATCH] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-24T23:38:00Z","receivedAt":"2020-04-24T23:38:07Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Occasionally a failure a user is seeing may be related to a specific\nhook which is being run, perhaps without the user realizing. While the\ncontents of hooks can be sensitive - containing user data or process\ninformation specific to the user's organization - simply knowing that a\nhook is being run at a certain stage can help us to understand whether\nsomething is going wrong.\n\nWithout a definitive list of hook names within the code, we compile our\nown list from the documentation. This is likely prone to bitrot. To\nreduce the amount of code humans need to read, we turn the list into a\nstring_list and iterate over it (as we are calling the same find_hook\noperation on each string). However, since bugreport should primarily be\ncalled by the user, the performance loss from massaging the string\nseems acceptable.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nBased on es/bugreport.\n\n Documentation/git-bugreport.txt |  1 +\n bugreport.c                     | 52 +++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex 643d1b2884..7fe9aef34e 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -28,6 +28,7 @@ The following information is captured automatically:\n  - 'git version --build-options'\n  - uname sysname, release, version, and machine strings\n  - Compiler-specific info string\n+ - A list of enabled hooks\n \n This tool is invoked via the typical Git setup process, which means that in some\n cases, it might not be able to launch - for example, if a relevant config file\ndiff --git a/bugreport.c b/bugreport.c\nindex 089b939a87..ce32145bce 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -5,6 +5,8 @@\n #include \"time.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n+#include \"run-command.h\"\n+\n \n static void get_system_info(struct strbuf *sys_info)\n {\n@@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)\n \tget_libc_info(sys_info);\n }\n \n+static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n+{\n+\t/*\n+\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n+\t * so below is a transcription of `git help hooks`. Later, this should\n+\t * be replaced with some programmatically generated list (generated from\n+\t * doc or else taken from some library which tells us about all the\n+\t * hooks)\n+\t */\n+\tconst char *hook[] = {\n+\t\t\"applypatch-msg\",\n+\t\t\"pre-applypatch\",\n+\t\t\"post-applypatch\",\n+\t\t\"pre-commit\",\n+\t\t\"pre-merge-commit\",\n+\t\t\"prepare-commit-msg\",\n+\t\t\"commit-msg\",\n+\t\t\"post-commit\",\n+\t\t\"pre-rebase\",\n+\t\t\"post-checkout\",\n+\t\t\"post-merge\",\n+\t\t\"pre-push\",\n+\t\t\"pre-receive\",\n+\t\t\"update\",\n+\t\t\"post-receive\",\n+\t\t\"post-update\",\n+\t\t\"push-to-checkout\",\n+\t\t\"pre-auto-gc\",\n+\t\t\"post-rewrite\",\n+\t\t\"sendemail-validate\",\n+\t\t\"fsmonitor-watchman\",\n+\t\t\"p4-pre-submit\",\n+\t\t\"post-index-change\",\n+\t};\n+\tint i;\n+\n+\tif (nongit) {\n+\t\tstrbuf_addstr(hook_info,\n+\t\t\t\"not run from a git repository - no hooks to show\\n\");\n+\t\treturn;\n+\t}\n+\n+\tfor (i=0; i < ARRAY_SIZE(hook); i++)\n+\t\tif (find_hook(hook[i]))\n+\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n+}\n+\n static const char * const bugreport_usage[] = {\n \tN_(\"git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]\"),\n \tNULL\n@@ -116,6 +165,9 @@ int cmd_main(int argc, const char **argv)\n \tget_header(&buffer, _(\"System Info\"));\n \tget_system_info(&buffer);\n \n+\tget_header(&buffer, \"Enabled Hooks\");\n+\tget_populated_hooks(&buffer, nongit_ok);\n+\n \t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n \treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396207","messageId":"20200425003002.GC17217@google.com","threadId":"53313","inReplyTo":"20200424233800.200439-1-emilyshaffer@google.com","subject":"Re: [PATCH] bugreport: collect list of populated hooks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-25T00:30:02Z","receivedAt":"2020-04-25T00:30:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Emily Shaffer wrote:\n\n> Occasionally a failure a user is seeing may be related to a specific\n> hook which is being run, perhaps without the user realizing. While the\n> contents of hooks can be sensitive - containing user data or process\n> information specific to the user's organization - simply knowing that a\n> hook is being run at a certain stage can help us to understand whether\n> something is going wrong.\n\nNice.\n\n[...]\n>  Documentation/git-bugreport.txt |  1 +\n>  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++\n>  2 files changed, 53 insertions(+)\n\nCan this functionality be demonstrated in a test?\n\n> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\n> index 643d1b2884..7fe9aef34e 100644\n> --- a/Documentation/git-bugreport.txt\n> +++ b/Documentation/git-bugreport.txt\n> @@ -28,6 +28,7 @@ The following information is captured automatically:\n>   - 'git version --build-options'\n>   - uname sysname, release, version, and machine strings\n>   - Compiler-specific info string\n> + - A list of enabled hooks\n>  \n>  This tool is invoked via the typical Git setup process, which means that in some\n>  cases, it might not be able to launch - for example, if a relevant config file\n> diff --git a/bugreport.c b/bugreport.c\n> index 089b939a87..ce32145bce 100644\n> --- a/bugreport.c\n> +++ b/bugreport.c\n> @@ -5,6 +5,8 @@\n>  #include \"time.h\"\n\nNot about this patch: this is for the system header <time.h>, right?\nGit includes the same system headers in (almost) all source files, via\ncache.h or git-compat-util.h, so it should be possible to leave this\n#include out.  (Handling it in one place gives us a chance to get\nportability gotchas around names of headers, macros like _POSIX_SOURCE,\nand so on right).\n\n[...]\n> +\t/*\n> +\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n> +\t * so below is a transcription of `git help hooks`. Later, this should\n> +\t * be replaced with some programmatically generated list (generated from\n> +\t * doc or else taken from some library which tells us about all the\n> +\t * hooks)\n> +\t */\n> +\tconst char *hook[] = {\n> +\t\t\"applypatch-msg\",\n> +\t\t\"pre-applypatch\",\n> +\t\t\"post-applypatch\",\n> +\t\t\"pre-commit\",\n> +\t\t\"pre-merge-commit\",\n> +\t\t\"prepare-commit-msg\",\n> +\t\t\"commit-msg\",\n> +\t\t\"post-commit\",\n> +\t\t\"pre-rebase\",\n> +\t\t\"post-checkout\",\n> +\t\t\"post-merge\",\n> +\t\t\"pre-push\",\n> +\t\t\"pre-receive\",\n> +\t\t\"update\",\n> +\t\t\"post-receive\",\n> +\t\t\"post-update\",\n> +\t\t\"push-to-checkout\",\n> +\t\t\"pre-auto-gc\",\n> +\t\t\"post-rewrite\",\n> +\t\t\"sendemail-validate\",\n> +\t\t\"fsmonitor-watchman\",\n> +\t\t\"p4-pre-submit\",\n> +\t\t\"post-index-change\",\n> +\t};\n\nInteresting.   It would be possible to do some gettext-style trickery\ninvolving scanning for run_hook calls, but converting to an enum as\nyou've suggested previously sounds simpler.\n\nThanks,\nJonathan\n"},{"id":"396232","messageId":"xmqqsggs9ngo.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200424233800.200439-1-emilyshaffer@google.com","subject":"Re: [PATCH] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-25T04:52:07Z","receivedAt":"2020-04-25T04:52:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> Without a definitive list of hook names within the code, we compile our\n> own list from the documentation. This is likely prone to bitrot. To\n> reduce the amount of code humans need to read, we turn the list into a\n> string_list and iterate over it (as we are calling the same find_hook\n> operation on each string). However, since bugreport should primarily be\n> called by the user, the performance loss from massaging the string\n> seems acceptable.\n\nIn this iteration we no longer are collecting the hook names into\nstring list, but just formating the findings in a strbuf, no?\n\n> @@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)\n>  \tget_libc_info(sys_info);\n>  }\n>  \n> +static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n> +{\n> +\t/*\n> +\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n> +\t * so below is a transcription of `git help hooks`. Later, this should\n> +\t * be replaced with some programmatically generated list (generated from\n> +\t * doc or else taken from some library which tells us about all the\n> +\t * hooks)\n> +\t */\n\nYes, I recall that we discussed adding some annotation to\ndocumentation and extracting this automatically.\n\n> +\tconst char *hook[] = {\n\nIs it worth making this \"static const\"?\n\n> +\tif (nongit) {\n> +\t\tstrbuf_addstr(hook_info,\n> +\t\t\t\"not run from a git repository - no hooks to show\\n\");\n> +\t\treturn;\n> +\t}\n> +\n> +\tfor (i=0; i < ARRAY_SIZE(hook); i++)\n\nStyle: SP around = in \"i=0\".\n\n> +\tget_header(&buffer, \"Enabled Hooks\");\n> +\tget_populated_hooks(&buffer, nongit_ok);\n> +\n\nSounds good, otherwise.\n\n"},{"id":"396365","messageId":"20200427190257.GA77802@google.com","threadId":"53313","inReplyTo":"xmqqsggs9ngo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-27T19:02:57Z","receivedAt":"2020-04-27T19:03:06Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Apr 24, 2020 at 09:52:07PM -0700, Junio C Hamano wrote:\n> \n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> > Without a definitive list of hook names within the code, we compile our\n> > own list from the documentation. This is likely prone to bitrot. To\n> > reduce the amount of code humans need to read, we turn the list into a\n> > string_list and iterate over it (as we are calling the same find_hook\n> > operation on each string). However, since bugreport should primarily be\n> > called by the user, the performance loss from massaging the string\n> > seems acceptable.\n> \n> In this iteration we no longer are collecting the hook names into\n> string list, but just formating the findings in a strbuf, no?\n\nRight - I'll fix the commit message for round 2.\n\n> \n> > @@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)\n> >  \tget_libc_info(sys_info);\n> >  }\n> >  \n> > +static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n> > +{\n> > +\t/*\n> > +\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n> > +\t * so below is a transcription of `git help hooks`. Later, this should\n> > +\t * be replaced with some programmatically generated list (generated from\n> > +\t * doc or else taken from some library which tells us about all the\n> > +\t * hooks)\n> > +\t */\n> \n> Yes, I recall that we discussed adding some annotation to\n> documentation and extracting this automatically.\n\nRight, we did. I think I was hesitant to move on it because I had\nhttps://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com\non my back burner and wasn't sure how the hook architecture would look\nafterwards.\n\nI'm not sure I know the right way to move forward (which is why I left a\nNEEDSWORK) - my strongest preference is to leave it as is and wait for\nthe linked RFC and related work to land, at which point this code will\nneed a rework anyways. But if we're very interested in generating an\nenum or something from the docs rather than letting this gross char**\nland, I can do that too.\n\n - Emily\n"},{"id":"396387","messageId":"xmqqr1w87j2p.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200427190257.GA77802@google.com","subject":"Re: [PATCH] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T20:46:38Z","receivedAt":"2020-04-27T20:46:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n>> > +\t/*\n>> > +\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n>> > +\t * so below is a transcription of `git help hooks`. Later, this should\n>> > +\t * be replaced with some programmatically generated list (generated from\n>> > +\t * doc or else taken from some library which tells us about all the\n>> > +\t * hooks)\n>> > +\t */\n>> \n>> Yes, I recall that we discussed adding some annotation to\n>> documentation and extracting this automatically.\n>\n> Right, we did. I think I was hesitant to move on it because I had\n> https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com\n> on my back burner and wasn't sure how the hook architecture would look\n> afterwards.\n\nI thought we already agreed back then during that discussion I\nrecalled that we'd leave that \"single source of truth\" unification\nout of the current topic.  Unless anything has changed recently to\nbring us closer to be ready to start designing, I think it still is\nfine to keep the needs-work comment above and punting.\n\nThanks.\n\n"},{"id":"396388","messageId":"20200427204859.171084-1-emilyshaffer@google.com","threadId":"53313","inReplyTo":"20200425003002.GC17217@google.com","subject":"[PATCH] bugreport: drop time.h include","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-27T20:48:59Z","receivedAt":"2020-04-27T20:49:12Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"As pointed out in\nhttps://lore.kernel.org/git/20200425003002.GC17217@google.com, we don't\nneed to include time.h explicitly because it is included (and possibly\nmanaged for portability) by cache.h. Drop the include.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nThanks for the observation, Jonathan.\n\n - Emily\n\n bugreport.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/bugreport.c b/bugreport.c\nindex 089b939a87..e4a7ed3a23 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -2,7 +2,6 @@\n #include \"parse-options.h\"\n #include \"stdio.h\"\n #include \"strbuf.h\"\n-#include \"time.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n \n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396389","messageId":"20200427204946.GB77802@google.com","threadId":"53313","inReplyTo":"xmqqr1w87j2p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-27T20:49:46Z","receivedAt":"2020-04-27T20:49:53Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Apr 27, 2020 at 01:46:38PM -0700, Junio C Hamano wrote:\n> \n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> >> > +\t/*\n> >> > +\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n> >> > +\t * so below is a transcription of `git help hooks`. Later, this should\n> >> > +\t * be replaced with some programmatically generated list (generated from\n> >> > +\t * doc or else taken from some library which tells us about all the\n> >> > +\t * hooks)\n> >> > +\t */\n> >> \n> >> Yes, I recall that we discussed adding some annotation to\n> >> documentation and extracting this automatically.\n> >\n> > Right, we did. I think I was hesitant to move on it because I had\n> > https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com\n> > on my back burner and wasn't sure how the hook architecture would look\n> > afterwards.\n> \n> I thought we already agreed back then during that discussion I\n> recalled that we'd leave that \"single source of truth\" unification\n> out of the current topic.  Unless anything has changed recently to\n> bring us closer to be ready to start designing, I think it still is\n> fine to keep the needs-work comment above and punting.\n\nSure. Thanks.\n"},{"id":"396391","messageId":"20200427210311.GB36078@google.com","threadId":"53313","inReplyTo":"20200427204859.171084-1-emilyshaffer@google.com","subject":"Re: [PATCH] bugreport: drop time.h include","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-27T21:03:11Z","receivedAt":"2020-04-27T21:03:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Emily Shaffer wrote:\n\n> As pointed out in\n> https://lore.kernel.org/git/20200425003002.GC17217@google.com,\n\nThis breadcrumb shouldn't be needed, since the rest of the commit\nmessage already speaks for itself.  We can save the future \"git log\"\nreader some time by leaing it out.\n\n>                                                                we don't\n> need to include time.h explicitly because it is included (and possibly\n> managed for portability) by cache.h. Drop the include.\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n> Thanks for the observation, Jonathan.\n>\n>  - Emily\n>\n>  bugreport.c | 1 -\n>  1 file changed, 1 deletion(-)\n>\n> diff --git a/bugreport.c b/bugreport.c\n> index 089b939a87..e4a7ed3a23 100644\n> --- a/bugreport.c\n> +++ b/bugreport.c\n> @@ -2,7 +2,6 @@\n>  #include \"parse-options.h\"\n>  #include \"stdio.h\"\n>  #include \"strbuf.h\"\n> -#include \"time.h\"\n\nSame applies to stdio.h, I believe.  Forgive my laziness in not being\nexhaustive before.\n\nThanks,\nJonathan\n"},{"id":"396394","messageId":"xmqqh7x47har.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200427210311.GB36078@google.com","subject":"Re: [PATCH] bugreport: drop time.h include","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T21:25:00Z","receivedAt":"2020-04-27T21:25:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Emily Shaffer wrote:\n>\n>> As pointed out in\n>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,\n>\n> This breadcrumb shouldn't be needed, since the rest of the commit\n> message already speaks for itself.  We can save the future \"git log\"\n> reader some time by leaing it out.\n\nTrue.\n\nThanks.\n"},{"id":"396396","messageId":"xmqqd07s7gjr.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"xmqqh7x47har.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bugreport: drop time.h include","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T21:41:12Z","receivedAt":"2020-04-27T21:41:19Z","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> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>> Emily Shaffer wrote:\n>>\n>>> As pointed out in\n>>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,\n>>\n>> This breadcrumb shouldn't be needed, since the rest of the commit\n>> message already speaks for itself.  We can save the future \"git log\"\n>> reader some time by leaing it out.\n>\n> True.\n\nWell, removing these two lines made the rest non-sentence, so I had\nto rewrite the thing.  I am not sure if the educational value warrants\nthe mention of compat/ exemption, but it people find it too noisy,\nit can certainly be dropped.\n\nThanks.\n\n-- >8 --\nFrom: Emily Shaffer <emilyshaffer@google.com>\nDate: Mon, 27 Apr 2020 13:48:59 -0700\nSubject: [PATCH] bugreport: do not include <time.h>\n\nIn the generic parts of the source files, system headers like\n<time.h> are supposed to be included indirectly by including\n\"git-compat-util.h\", which manages portability issues (platform\nspecific compat/ sources are generally exempt from this rule).\n\nDrop the inclusion.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n bugreport.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/bugreport.c b/bugreport.c\nindex 089b939a87..e4a7ed3a23 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -2,7 +2,6 @@\n #include \"parse-options.h\"\n #include \"stdio.h\"\n #include \"strbuf.h\"\n-#include \"time.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n \n-- \n2.26.2-266-ge870325ee8\n\n"},{"id":"396398","messageId":"20200427215619.GC77802@google.com","threadId":"53313","inReplyTo":"xmqqd07s7gjr.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bugreport: drop time.h include","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-27T21:56:19Z","receivedAt":"2020-04-27T21:56:25Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Apr 27, 2020 at 02:41:12PM -0700, Junio C Hamano wrote:\n> \n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Jonathan Nieder <jrnieder@gmail.com> writes:\n> >\n> >> Emily Shaffer wrote:\n> >>\n> >>> As pointed out in\n> >>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,\n> >>\n> >> This breadcrumb shouldn't be needed, since the rest of the commit\n> >> message already speaks for itself.  We can save the future \"git log\"\n> >> reader some time by leaing it out.\n> >\n> > True.\n> \n> Well, removing these two lines made the rest non-sentence, so I had\n> to rewrite the thing.  I am not sure if the educational value warrants\n> the mention of compat/ exemption, but it people find it too noisy,\n> it can certainly be dropped.\n\nI've got a reroll to drop the \"stdio.h\" include too - do you want me to\nsend it? Your commit message is much nicer than what I came up with on\nmy end dropping the breadcrumb and generalizing to include stdio.h, so I\ncan adapt it if you're interested in the reroll.\n\n> \n> Thanks.\n> \n> -- >8 --\n> \n> In the generic parts of the source files, system headers like\n> <time.h> are supposed to be included indirectly by including\n> \"git-compat-util.h\", which manages portability issues (platform\n> specific compat/ sources are generally exempt from this rule).\n> \n> Drop the inclusion.\n> \n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  bugreport.c | 1 -\n>  1 file changed, 1 deletion(-)\n> \n> diff --git a/bugreport.c b/bugreport.c\n> index 089b939a87..e4a7ed3a23 100644\n> --- a/bugreport.c\n> +++ b/bugreport.c\n> @@ -2,7 +2,6 @@\n>  #include \"parse-options.h\"\n>  #include \"stdio.h\"\n>  #include \"strbuf.h\"\n> -#include \"time.h\"\n>  #include \"help.h\"\n>  #include \"compat/compiler.h\"\n>  \n> -- \n> 2.26.2-266-ge870325ee8\n> \n"},{"id":"396404","messageId":"xmqqzhaw5x2m.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200427215619.GC77802@google.com","subject":"Re: [PATCH] bugreport: drop time.h include","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T23:27:13Z","receivedAt":"2020-04-27T23:27:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> On Mon, Apr 27, 2020 at 02:41:12PM -0700, Junio C Hamano wrote:\n>> \n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> > Jonathan Nieder <jrnieder@gmail.com> writes:\n>> >\n>> >> Emily Shaffer wrote:\n>> >>\n>> >>> As pointed out in\n>> >>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,\n>> >>\n>> >> This breadcrumb shouldn't be needed, since the rest of the commit\n>> >> message already speaks for itself.  We can save the future \"git log\"\n>> >> reader some time by leaing it out.\n>> >\n>> > True.\n>> \n>> Well, removing these two lines made the rest non-sentence, so I had\n>> to rewrite the thing.  I am not sure if the educational value warrants\n>> the mention of compat/ exemption, but it people find it too noisy,\n>> it can certainly be dropped.\n>\n> I've got a reroll to drop the \"stdio.h\" include too - do you want me to\n> send it? Your commit message is much nicer than what I came up with on\n> my end dropping the breadcrumb and generalizing to include stdio.h, so I\n> can adapt it if you're interested in the reroll.\n\nSure.  I already queued it and merged it to 'next', but the result\nhasn't been pushed out and I am bisecting a test failure in some\nnewish topics that are merged to 'jch' recently, so I do not mind\ntaking a reroll and redoing 'next' before I can push it out today.\n\nThanks.\n"},{"id":"396406","messageId":"20200427233820.179891-1-emilyshaffer@google.com","threadId":"53313","inReplyTo":"20200424233800.200439-1-emilyshaffer@google.com","subject":"[PATCH v2] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-27T23:38:20Z","receivedAt":"2020-04-27T23:38:40Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Occasionally a failure a user is seeing may be related to a specific\nhook which is being run, perhaps without the user realizing. While the\ncontents of hooks can be sensitive - containing user data or process\ninformation specific to the user's organization - simply knowing that a\nhook is being run at a certain stage can help us to understand whether\nsomething is going wrong.\n\nWithout a definitive list of hook names within the code, we compile our\nown list from the documentation. This is likely prone to bitrot, but\ndesigning a single source of truth for acceptable hooks is too much\noverhead for this small change to the bugreport tool.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nSince v1:\n\n - turned array of hooknames into a static const\n - added a test\n - i18n-ified the outputs added in this section\n - style changes\n\nThanks, all.\n - Emily\n\n Documentation/git-bugreport.txt |  1 +\n bugreport.c                     | 52 +++++++++++++++++++++++++++++++++\n t/t0091-bugreport.sh            | 10 +++++++\n 3 files changed, 63 insertions(+)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex 643d1b2884..7fe9aef34e 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -28,6 +28,7 @@ The following information is captured automatically:\n  - 'git version --build-options'\n  - uname sysname, release, version, and machine strings\n  - Compiler-specific info string\n+ - A list of enabled hooks\n \n This tool is invoked via the typical Git setup process, which means that in some\n cases, it might not be able to launch - for example, if a relevant config file\ndiff --git a/bugreport.c b/bugreport.c\nindex 089b939a87..81904b508e 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -5,6 +5,8 @@\n #include \"time.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n+#include \"run-command.h\"\n+\n \n static void get_system_info(struct strbuf *sys_info)\n {\n@@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)\n \tget_libc_info(sys_info);\n }\n \n+static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n+{\n+\t/*\n+\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n+\t * so below is a transcription of `git help hooks`. Later, this should\n+\t * be replaced with some programmatically generated list (generated from\n+\t * doc or else taken from some library which tells us about all the\n+\t * hooks)\n+\t */\n+\tstatic const char *hook[] = {\n+\t\t\"applypatch-msg\",\n+\t\t\"pre-applypatch\",\n+\t\t\"post-applypatch\",\n+\t\t\"pre-commit\",\n+\t\t\"pre-merge-commit\",\n+\t\t\"prepare-commit-msg\",\n+\t\t\"commit-msg\",\n+\t\t\"post-commit\",\n+\t\t\"pre-rebase\",\n+\t\t\"post-checkout\",\n+\t\t\"post-merge\",\n+\t\t\"pre-push\",\n+\t\t\"pre-receive\",\n+\t\t\"update\",\n+\t\t\"post-receive\",\n+\t\t\"post-update\",\n+\t\t\"push-to-checkout\",\n+\t\t\"pre-auto-gc\",\n+\t\t\"post-rewrite\",\n+\t\t\"sendemail-validate\",\n+\t\t\"fsmonitor-watchman\",\n+\t\t\"p4-pre-submit\",\n+\t\t\"post-index-change\",\n+\t};\n+\tint i;\n+\n+\tif (nongit) {\n+\t\tstrbuf_addstr(hook_info,\n+\t\t\t_(\"not run from a git repository - no hooks to show\\n\"));\n+\t\treturn;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(hook); i++)\n+\t\tif (find_hook(hook[i]))\n+\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n+}\n+\n static const char * const bugreport_usage[] = {\n \tN_(\"git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]\"),\n \tNULL\n@@ -116,6 +165,9 @@ int cmd_main(int argc, const char **argv)\n \tget_header(&buffer, _(\"System Info\"));\n \tget_system_info(&buffer);\n \n+\tget_header(&buffer, _(\"Enabled Hooks\"));\n+\tget_populated_hooks(&buffer, nongit_ok);\n+\n \t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n \treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \ndiff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\nindex 2e73658a5c..387fa46c3f 100755\n--- a/t/t0091-bugreport.sh\n+++ b/t/t0091-bugreport.sh\n@@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '\n \tnongit git bugreport -o foo/bar/baz\n '\n \n+test_expect_success 'indicates populated hooks' '\n+\ttest_when_finished rm git-bugreport-hooks.txt &&\n+\ttest_when_finished rm -fr .git/hooks &&\n+\tmkdir .git/hooks &&\n+\ttouch .git/hooks/applypatch-msg &&\n+\tchmod +x .git/hooks/applypatch-msg &&\n+\tgit bugreport -s hooks &&\n+\ttest_i18ngrep applypatch-msg git-bugreport-hooks.txt\n+'\n+\n \n test_done\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396407","messageId":"20200427234231.194456-1-emilyshaffer@google.com","threadId":"53313","inReplyTo":"20200427204859.171084-1-emilyshaffer@google.com","subject":"[PATCH v2] bugreport: drop extraneous includes","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-27T23:42:31Z","receivedAt":"2020-04-27T23:42:46Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"In the generic parts of the source files, system headers like <time.h>\nand <stdio.h> are supposed to be included indirectly bt including\n\"git-compat-util.h\", which manages portability issues.\n\nDrop our explicit inclusions and rely on \"cache.h\", which includes\n\"git-compat-util.h\".\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\n bugreport.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/bugreport.c b/bugreport.c\nindex 089b939a87..acacca8fef 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -1,8 +1,6 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n-#include \"stdio.h\"\n #include \"strbuf.h\"\n-#include \"time.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n \n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396408","messageId":"20200427234510.GC36078@google.com","threadId":"53313","inReplyTo":"20200427233820.179891-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] bugreport: collect list of populated hooks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-27T23:45:10Z","receivedAt":"2020-04-27T23:45:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Emily Shaffer wrote:\n\n> --- a/t/t0091-bugreport.sh\n> +++ b/t/t0091-bugreport.sh\n> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '\n>  \tnongit git bugreport -o foo/bar/baz\n>  '\n>  \n> +test_expect_success 'indicates populated hooks' '\n> +\ttest_when_finished rm git-bugreport-hooks.txt &&\n> +\ttest_when_finished rm -fr .git/hooks &&\n> +\tmkdir .git/hooks &&\n> +\ttouch .git/hooks/applypatch-msg &&\n> +\tchmod +x .git/hooks/applypatch-msg &&\n\noptional: could use write_script for this\n\n> +\tgit bugreport -s hooks &&\n> +\ttest_i18ngrep applypatch-msg git-bugreport-hooks.txt\n\nThis should work even when translated, so it can use \"grep\" instead of\ntest_i18ngrep.\n\nA few more things to test:\n- that it doesn't include hooks we *haven't* installed. :)\n- that it isn't confused by the default *.sample hooks\n- what happens when outside a git repository?\n\nEven without such tests,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"396409","messageId":"20200427234625.GD36078@google.com","threadId":"53313","inReplyTo":"20200427234231.194456-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] bugreport: drop extraneous includes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-27T23:46:25Z","receivedAt":"2020-04-27T23:46:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Emily Shaffer wrote:\n\n> In the generic parts of the source files, system headers like <time.h>\n> and <stdio.h> are supposed to be included indirectly bt including\n\ns/bt/by/\n\n> \"git-compat-util.h\", which manages portability issues.\n>\n> Drop our explicit inclusions and rely on \"cache.h\", which includes\n> \"git-compat-util.h\".\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  bugreport.c | 2 --\n>  1 file changed, 2 deletions(-)\n\nWith that tweak,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"396417","messageId":"xmqqh7x45vd1.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200427234510.GC36078@google.com","subject":"Re: [PATCH v2] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T00:04:10Z","receivedAt":"2020-04-28T00:04:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Emily Shaffer wrote:\n>\n>> --- a/t/t0091-bugreport.sh\n>> +++ b/t/t0091-bugreport.sh\n>> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '\n>>  \tnongit git bugreport -o foo/bar/baz\n>>  '\n>>  \n>> +test_expect_success 'indicates populated hooks' '\n>> +\ttest_when_finished rm git-bugreport-hooks.txt &&\n>> +\ttest_when_finished rm -fr .git/hooks &&\n>> +\tmkdir .git/hooks &&\n>> +\ttouch .git/hooks/applypatch-msg &&\n>> +\tchmod +x .git/hooks/applypatch-msg &&\n>\n> optional: could use write_script for this\n\nYup, it is a good practice to do so.  Use of touch is especially bad\nhere, as it is quite plausible for us to later change the \"is the\nhook there?\" check to exclude a completely empty file.\n\n>> +\tgit bugreport -s hooks &&\n>> +\ttest_i18ngrep applypatch-msg git-bugreport-hooks.txt\n>\n> This should work even when translated, so it can use \"grep\" instead of\n> test_i18ngrep.\n\nNicely spotted.\n\nThanks.\n"},{"id":"396635","messageId":"20200430000133.GD77802@google.com","threadId":"53313","inReplyTo":"20200427234510.GC36078@google.com","subject":"Re: [PATCH v2] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-30T00:01:33Z","receivedAt":"2020-04-30T00:01:41Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Apr 27, 2020 at 04:45:10PM -0700, Jonathan Nieder wrote:\n> \n> Emily Shaffer wrote:\n> \n> > --- a/t/t0091-bugreport.sh\n> > +++ b/t/t0091-bugreport.sh\n> > @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '\n> >  \tnongit git bugreport -o foo/bar/baz\n> >  '\n> >  \n> > +test_expect_success 'indicates populated hooks' '\n> > +\ttest_when_finished rm git-bugreport-hooks.txt &&\n> > +\ttest_when_finished rm -fr .git/hooks &&\n> > +\tmkdir .git/hooks &&\n> > +\ttouch .git/hooks/applypatch-msg &&\n> > +\tchmod +x .git/hooks/applypatch-msg &&\n> \n> optional: could use write_script for this\n\nACK, especially given Junio's comment in reply here.\n\n> \n> > +\tgit bugreport -s hooks &&\n> > +\ttest_i18ngrep applypatch-msg git-bugreport-hooks.txt\n> \n> This should work even when translated, so it can use \"grep\" instead of\n> test_i18ngrep.\n> \n> A few more things to test:\n> - that it doesn't include hooks we *haven't* installed. :)\n> - that it isn't confused by the default *.sample hooks\n\nI'll lump these together by adding a .sample hook and ensuring that hook\nwas excluded.\n\n> - what happens when outside a git repository?\n\nThe earlier test to make sure it doesn't crash seems satisfactory to me\n- is there some other behavior you'd like to ensure? I'm not keen to\ncheck the lines below the header are empty, since the spacing there\ncould change and break an unrelated test.\n\nReroll coming today. These changes are straightforward and passing for\nme locally.\n\n - Emily\n"},{"id":"396642","messageId":"20200430012425.209122-1-emilyshaffer@google.com","threadId":"53313","inReplyTo":"20200427233820.179891-1-emilyshaffer@google.com","subject":"[PATCH v3] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-04-30T01:24:25Z","receivedAt":"2020-04-30T01:24:39Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Occasionally a failure a user is seeing may be related to a specific\nhook which is being run, perhaps without the user realizing. While the\ncontents of hooks can be sensitive - containing user data or process\ninformation specific to the user's organization - simply knowing that a\nhook is being run at a certain stage can help us to understand whether\nsomething is going wrong.\n\nWithout a definitive list of hook names within the code, we compile our\nown list from the documentation. This is likely prone to bitrot, but\ndesigning a single source of truth for acceptable hooks is too much\noverhead for this small change to the bugreport tool.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nSince v2, switched to write_script, and modified the test to check for\nuninstalled hooks and sample hooks having correct behavior.\n\n Documentation/git-bugreport.txt |  1 +\n bugreport.c                     | 52 +++++++++++++++++++++++++++++++++\n t/t0091-bugreport.sh            | 10 +++++++\n 3 files changed, 63 insertions(+)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex 643d1b2884..7fe9aef34e 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -28,6 +28,7 @@ The following information is captured automatically:\n  - 'git version --build-options'\n  - uname sysname, release, version, and machine strings\n  - Compiler-specific info string\n+ - A list of enabled hooks\n \n This tool is invoked via the typical Git setup process, which means that in some\n cases, it might not be able to launch - for example, if a relevant config file\ndiff --git a/bugreport.c b/bugreport.c\nindex acacca8fef..aa8a489c35 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -3,6 +3,8 @@\n #include \"strbuf.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n+#include \"run-command.h\"\n+\n \n static void get_system_info(struct strbuf *sys_info)\n {\n@@ -31,6 +33,53 @@ static void get_system_info(struct strbuf *sys_info)\n \tget_libc_info(sys_info);\n }\n \n+static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n+{\n+\t/*\n+\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n+\t * so below is a transcription of `git help hooks`. Later, this should\n+\t * be replaced with some programmatically generated list (generated from\n+\t * doc or else taken from some library which tells us about all the\n+\t * hooks)\n+\t */\n+\tstatic const char *hook[] = {\n+\t\t\"applypatch-msg\",\n+\t\t\"pre-applypatch\",\n+\t\t\"post-applypatch\",\n+\t\t\"pre-commit\",\n+\t\t\"pre-merge-commit\",\n+\t\t\"prepare-commit-msg\",\n+\t\t\"commit-msg\",\n+\t\t\"post-commit\",\n+\t\t\"pre-rebase\",\n+\t\t\"post-checkout\",\n+\t\t\"post-merge\",\n+\t\t\"pre-push\",\n+\t\t\"pre-receive\",\n+\t\t\"update\",\n+\t\t\"post-receive\",\n+\t\t\"post-update\",\n+\t\t\"push-to-checkout\",\n+\t\t\"pre-auto-gc\",\n+\t\t\"post-rewrite\",\n+\t\t\"sendemail-validate\",\n+\t\t\"fsmonitor-watchman\",\n+\t\t\"p4-pre-submit\",\n+\t\t\"post-index-change\",\n+\t};\n+\tint i;\n+\n+\tif (nongit) {\n+\t\tstrbuf_addstr(hook_info,\n+\t\t\t_(\"not run from a git repository - no hooks to show\\n\"));\n+\t\treturn;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(hook); i++)\n+\t\tif (find_hook(hook[i]))\n+\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n+}\n+\n static const char * const bugreport_usage[] = {\n \tN_(\"git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]\"),\n \tNULL\n@@ -114,6 +163,9 @@ int cmd_main(int argc, const char **argv)\n \tget_header(&buffer, _(\"System Info\"));\n \tget_system_info(&buffer);\n \n+\tget_header(&buffer, _(\"Enabled Hooks\"));\n+\tget_populated_hooks(&buffer, nongit_ok);\n+\n \t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n \treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \ndiff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\nindex 2e73658a5c..612c12a918 100755\n--- a/t/t0091-bugreport.sh\n+++ b/t/t0091-bugreport.sh\n@@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '\n \tnongit git bugreport -o foo/bar/baz\n '\n \n+test_expect_success 'indicates populated hooks' '\n+\ttest_when_finished rm git-bugreport-hooks.txt &&\n+\ttest_when_finished rm -fr .git/hooks &&\n+\tmkdir .git/hooks &&\n+\twrite_script .git/hooks/applypatch-msg &&\n+\twrite_script .git/hooks/prepare-commit-msg.sample &&\n+\tgit bugreport -s hooks &&\n+\tgrep applypatch-msg git-bugreport-hooks.txt &&\n+\t! grep prepare-commit-msg git-bugreport-hooks.txt\n+'\n \n test_done\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396643","messageId":"20200430015049.GA115238@google.com","threadId":"53313","inReplyTo":"20200430012425.209122-1-emilyshaffer@google.com","subject":"Re: [PATCH v3] bugreport: collect list of populated hooks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-30T01:50:49Z","receivedAt":"2020-04-30T01:50:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"\nEmily Shaffer wrote:\n\n> Since v2, switched to write_script, and modified the test to check for\n> uninstalled hooks and sample hooks having correct behavior.\n\nLooks almost ready.  Two nits.\n\n[...]\n> --- a/t/t0091-bugreport.sh\n> +++ b/t/t0091-bugreport.sh\n> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '\n>  \tnongit git bugreport -o foo/bar/baz\n>  '\n>  \n> +test_expect_success 'indicates populated hooks' '\n> +\ttest_when_finished rm git-bugreport-hooks.txt &&\n> +\ttest_when_finished rm -fr .git/hooks &&\n> +\tmkdir .git/hooks &&\n\nThis is subtle.  Since c09a69a83e3 (Disable hooks during tests,\n2005-10-16), the repository in which the test runs has its \"hooks\"\ndirectory renamed to \"hooks-disabled\", with rationale\n\n    Individual tests for hooks would want to have their own tests when\n    written.  Also we should not pick up from random templates the user\n    happens to have.\n\nThat rationale is a bit strange because we explicitly passed\n--template to \"git init\" even then, so we could be confident that the\nbuilt-in templates that do not enable any hooks would be in use.\n\nMailing list diving finds [1].  We didn't have f98f8cbac01 (Ship\nsample hooks with .sample suffix, 2008-06-24) yet, which means that on\nfilesystems that do not record an executable bit, the sample hooks\nwere all enabled by default.  Nowadays that has been fixed and we\nshould be able to get rid of the \"mv .git/hooks .git/hooks-disabled\"\nin the test setup.\n\nWhen we do that, this \"mkdir .git/hooks\" will fail because the\ndirectory already exists.  Ideas:\n\n A. Include a preparatory patch in this series that removes that \"mv\"\n    command.  That way, this test can do\n\n\ttest_when_finished \"\n\t\trm -fr .git/hooks &&\n\t\tmv .git/hooks-saved .git/hooks\n\t\" &&\n\tmv .git/hooks .git/hooks-saved &&\n\tmkdir .git/hooks &&\n\t...\n\n    or even better (because of increased realism)\n\n\ttest_when_finished rm .git/hooks/applypatch-msg &&\n\twrite_script .git/hooks/applypatch-msg <<-\\EOF &&\n\t...\n\n  B. Run \"git init\" ourselves so we know what we're getting:\n\n  \ttest_when_finished \"rm -fr .git && mv .git-saved .git\" &&\n\tmv .git .git-saved &&\n\tgit init &&\n\t...\n\n> +\twrite_script .git/hooks/applypatch-msg &&\n\nwrite_script looks for a script on its stdin.  test_eval_ redirects\nstdin to come from /dev/null, so we happen to get an empty script, but\nthis is subtle.  How about something like\n\n\twrite_script .git/hooks/applypatch-msg <<-\\EOF &&\n\techo >&2 \"rejecting message in $1\"\n\texit 1\n\tEOF\n\nor\n\n\twrite_script .git/hooks/applypatch-msg </dev/null &&\n\n?\n\n> +\twrite_script .git/hooks/prepare-commit-msg.sample &&\n\nLikewise.\n\n> +\tgit bugreport -s hooks &&\n> +\tgrep applypatch-msg git-bugreport-hooks.txt &&\n> +\t! grep prepare-commit-msg git-bugreport-hooks.txt\n> +'\n\nThanks,\nJonathan\n"},{"id":"396644","messageId":"20200430015320.GB115238@google.com","threadId":"53313","inReplyTo":"20200430015049.GA115238@google.com","subject":"Re: [PATCH v3] bugreport: collect list of populated hooks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-30T01:53:20Z","receivedAt":"2020-04-30T01:53:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Emily Shaffer wrote:\n\n>> +++ b/t/t0091-bugreport.sh\n>> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '\n>>  \tnongit git bugreport -o foo/bar/baz\n>>  '\n>>  \n>> +test_expect_success 'indicates populated hooks' '\n>> +\ttest_when_finished rm git-bugreport-hooks.txt &&\n>> +\ttest_when_finished rm -fr .git/hooks &&\n>> +\tmkdir .git/hooks &&\n>\n> This is subtle.  Since c09a69a83e3 (Disable hooks during tests,\n> 2005-10-16), the repository in which the test runs has its \"hooks\"\n> directory renamed to \"hooks-disabled\", with rationale\n>\n>     Individual tests for hooks would want to have their own tests when\n>     written.  Also we should not pick up from random templates the user\n>     happens to have.\n>\n> That rationale is a bit strange because we explicitly passed\n> --template to \"git init\" even then, so we could be confident that the\n> built-in templates that do not enable any hooks would be in use.\n>\n> Mailing list diving finds [1].  We didn't have f98f8cbac01 (Ship\n> sample hooks with .sample suffix, 2008-06-24) yet, which means that on\n> filesystems that do not record an executable bit, the sample hooks\n> were all enabled by default.  Nowadays that has been fixed and we\n> should be able to get rid of the \"mv .git/hooks .git/hooks-disabled\"\n> in the test setup.\n\nAnd of course I forget to include the footnote.  Sorry for the noise.\n\nJonathan\n\n[1] https://lore.kernel.org/git/81b0412b0510140546ya10bc8fg3dd5eaab429eba6f@mail.gmail.com/\n"},{"id":"396675","messageId":"xmqqv9lgswar.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200430015049.GA115238@google.com","subject":"Re: [PATCH v3] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-30T17:44:28Z","receivedAt":"2020-04-30T17:44:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> When we do that, this \"mkdir .git/hooks\" will fail because the\n> directory already exists.  Ideas:\n>\n>  A. Include a preparatory patch in this series that removes that \"mv\"\n>     command.  That way, this test can do\n\nWhile I do not think it is realistic to anticipate that the \"test\"\nrepository may someday come with a hooks/ directory, even if we did\nso, we would not enable any hook by default in there.  So \"move away\nand restore\" feels way overkill.\n\n>   B. Run \"git init\" ourselves so we know what we're getting:\n\nThat is certainly safer, and simpler.  But perhaps the simplest\nwould be\n\n    C. Use \"mkdir -p .git/hooks\" so we won't get affected.\n\n>> +\twrite_script .git/hooks/applypatch-msg &&\n>\n> write_script looks for a script on its stdin.  test_eval_ redirects\n> stdin to come from /dev/null, so we happen to get an empty script, but\n> this is subtle.  How about something like\n>\n> \twrite_script .git/hooks/applypatch-msg <<-\\EOF &&\n> \techo >&2 \"rejecting message in $1\"\n> \texit 1\n> \tEOF\n\nYes, that is good.\n\n> or\n>\n> \twrite_script .git/hooks/applypatch-msg </dev/null &&\n\nThis takes us back to the resuling \"empty\" hook we wanted to avoid\nby switching from \"use touch to create something\" to \"write some\nmeaningful contents\" approach, no?\n\n>> +\tgit bugreport -s hooks &&\n>> +\tgrep applypatch-msg git-bugreport-hooks.txt &&\n>> +\t! grep prepare-commit-msg git-bugreport-hooks.txt\n>> +'\n>\n> Thanks,\n\nThanks.\n"},{"id":"396703","messageId":"xmqqwo5wpqvg.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"xmqqv9lgswar.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-30T22:09:55Z","receivedAt":"2020-04-30T22:10:04Z","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>> When we do that, this \"mkdir .git/hooks\" will fail because the\n>> directory already exists.  Ideas:\n>>\n>>  A. Include a preparatory patch in this series that removes that \"mv\"\n>>     command.  That way, this test can do\n>\n> While I do not think it is realistic to anticipate that the \"test\"\n> repository may someday come with a hooks/ directory, even if we did\n> so, we would not enable any hook by default in there.  So \"move away\n> and restore\" feels way overkill.\n>\n>>   B. Run \"git init\" ourselves so we know what we're getting:\n>\n> That is certainly safer, and simpler.  But perhaps the simplest\n> would be\n>\n>     C. Use \"mkdir -p .git/hooks\" so we won't get affected.\n\nIn the meantime, I added this SQUASH on top.  I do not claim that\nthis is the best solution, but the idea is to refuse to be affected\nby what is left in .git/hooks either by the test framework or\nearlier tests in the same test script file.\n\n t/t0091-bugreport.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\nindex 612c12a918..9450cc02e3 100755\n--- a/t/t0091-bugreport.sh\n+++ b/t/t0091-bugreport.sh\n@@ -58,11 +58,14 @@ test_expect_success 'can create leading directories outside of a git dir' '\n '\n \n test_expect_success 'indicates populated hooks' '\n-\ttest_when_finished rm git-bugreport-hooks.txt &&\n-\ttest_when_finished rm -fr .git/hooks &&\n+\trm -fr .git/hooks &&\n \tmkdir .git/hooks &&\n-\twrite_script .git/hooks/applypatch-msg &&\n-\twrite_script .git/hooks/prepare-commit-msg.sample &&\n+\tfor hook in applypatch-msg prepare-commit-msg.sample\n+\tdo\n+\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n+\t\techo \"hook $hook exists\"\n+\t\tEOF\n+\tdone &&\n \tgit bugreport -s hooks &&\n \tgrep applypatch-msg git-bugreport-hooks.txt &&\n \t! grep prepare-commit-msg git-bugreport-hooks.txt\n-- \n2.26.2-447-gd61d20c9b4\n\n"},{"id":"397358","messageId":"20200507210849.GH77802@google.com","threadId":"53313","inReplyTo":"xmqqwo5wpqvg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-05-07T21:08:49Z","receivedAt":"2020-05-07T21:08:58Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Apr 30, 2020 at 03:09:55PM -0700, Junio C Hamano wrote:\n> \n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >> When we do that, this \"mkdir .git/hooks\" will fail because the\n> >> directory already exists.  Ideas:\n> >>\n> >>  A. Include a preparatory patch in this series that removes that \"mv\"\n> >>     command.  That way, this test can do\n> >\n> > While I do not think it is realistic to anticipate that the \"test\"\n> > repository may someday come with a hooks/ directory, even if we did\n> > so, we would not enable any hook by default in there.  So \"move away\n> > and restore\" feels way overkill.\n> >\n> >>   B. Run \"git init\" ourselves so we know what we're getting:\n> >\n> > That is certainly safer, and simpler.  But perhaps the simplest\n> > would be\n> >\n> >     C. Use \"mkdir -p .git/hooks\" so we won't get affected.\n> \n> In the meantime, I added this SQUASH on top.  I do not claim that\n> this is the best solution, but the idea is to refuse to be affected\n> by what is left in .git/hooks either by the test framework or\n> earlier tests in the same test script file.\n\nThanks for the patience with the reply - like many of us, my attention\nis being pulled many directions right now :)\n\nThis solution looks very neat to me, minus one comment. I can send a\nsquash with the change I mention below.\n\n - Emily\n\n> \n>  t/t0091-bugreport.sh | 11 +++++++----\n>  1 file changed, 7 insertions(+), 4 deletions(-)\n> \n> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\n> index 612c12a918..9450cc02e3 100755\n> --- a/t/t0091-bugreport.sh\n> +++ b/t/t0091-bugreport.sh\n> @@ -58,11 +58,14 @@ test_expect_success 'can create leading directories outside of a git dir' '\n>  '\n>  \n>  test_expect_success 'indicates populated hooks' '\n> -\ttest_when_finished rm git-bugreport-hooks.txt &&\n> -\ttest_when_finished rm -fr .git/hooks &&\n\nI'm not sure it's necessary to lose these two lines. Especially the\ngenerated bugreport I'd like to clean up.\n\n> +\trm -fr .git/hooks &&\n>  \tmkdir .git/hooks &&\n> -\twrite_script .git/hooks/applypatch-msg &&\n> -\twrite_script .git/hooks/prepare-commit-msg.sample &&\n> +\tfor hook in applypatch-msg prepare-commit-msg.sample\n> +\tdo\n> +\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n> +\t\techo \"hook $hook exists\"\n> +\t\tEOF\n> +\tdone &&\n\nI like this placeholder script a lot.\n\n>  \tgit bugreport -s hooks &&\n>  \tgrep applypatch-msg git-bugreport-hooks.txt &&\n>  \t! grep prepare-commit-msg git-bugreport-hooks.txt\n> -- \n> 2.26.2-447-gd61d20c9b4\n> \n"},{"id":"397368","messageId":"xmqqa72j2vmh.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200507210849.GH77802@google.com","subject":"Re: [PATCH v3] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-07T23:06:14Z","receivedAt":"2020-05-07T23:06:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n>>  test_expect_success 'indicates populated hooks' '\n>> -\ttest_when_finished rm git-bugreport-hooks.txt &&\n>> -\ttest_when_finished rm -fr .git/hooks &&\n>\n> I'm not sure it's necessary to lose these two lines. Especially the\n> generated bugreport I'd like to clean up.\n\nI do not care either way, actually.  I left it so that it would be\neasier to debug the test by looking at the output.\n\n>> +\trm -fr .git/hooks &&\n>>  \tmkdir .git/hooks &&\n>> -\twrite_script .git/hooks/applypatch-msg &&\n>> -\twrite_script .git/hooks/prepare-commit-msg.sample &&\n>> +\tfor hook in applypatch-msg prepare-commit-msg.sample\n>> +\tdo\n>> +\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n>> +\t\techo \"hook $hook exists\"\n>> +\t\tEOF\n>> +\tdone &&\n>\n> I like this placeholder script a lot.\n\nI actually don't.  At least the final version should not quote EOF\n(otherwise $hook will appear verbatim).\n\n>>  \tgit bugreport -s hooks &&\n>>  \tgrep applypatch-msg git-bugreport-hooks.txt &&\n>>  \t! grep prepare-commit-msg git-bugreport-hooks.txt\n>> -- \n>> 2.26.2-447-gd61d20c9b4\n>> \n\nThanks.\n"},{"id":"397383","messageId":"20200508005357.223058-1-emilyshaffer@google.com","threadId":"53313","inReplyTo":"20200430012425.209122-1-emilyshaffer@google.com","subject":"[PATCH v4] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-05-08T00:53:57Z","receivedAt":"2020-05-08T00:54:08Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Occasionally a failure a user is seeing may be related to a specific\nhook which is being run, perhaps without the user realizing. While the\ncontents of hooks can be sensitive - containing user data or process\ninformation specific to the user's organization - simply knowing that a\nhook is being run at a certain stage can help us to understand whether\nsomething is going wrong.\n\nWithout a definitive list of hook names within the code, we compile our\nown list from the documentation. This is likely prone to bitrot, but\ndesigning a single source of truth for acceptable hooks is too much\noverhead for this small change to the bugreport tool.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nSince v3, squashed Junio's suggestion\n(https://lore.kernel.org/git/xmqqwo5wpqvg.fsf@gitster.c.googlers.com)\nbut kept the test_when_finished cleanup lines to try to avoid leaving\njunk for later tests.\n\nCI: https://github.com/nasamuffin/git/actions/runs/98568890, which seems\nlike it might be flaky. Have failed a couple because of downloads timing\nout... but I'm on top of next from a while ago, so I might be missing\nsome extra Actions topics?\n\n - Emily\n\n Documentation/git-bugreport.txt |  1 +\n bugreport.c                     | 52 +++++++++++++++++++++++++++++++++\n t/t0091-bugreport.sh            | 15 ++++++++++\n 3 files changed, 68 insertions(+)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex 643d1b2884..7fe9aef34e 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -28,6 +28,7 @@ The following information is captured automatically:\n  - 'git version --build-options'\n  - uname sysname, release, version, and machine strings\n  - Compiler-specific info string\n+ - A list of enabled hooks\n \n This tool is invoked via the typical Git setup process, which means that in some\n cases, it might not be able to launch - for example, if a relevant config file\ndiff --git a/bugreport.c b/bugreport.c\nindex acacca8fef..aa8a489c35 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -3,6 +3,8 @@\n #include \"strbuf.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n+#include \"run-command.h\"\n+\n \n static void get_system_info(struct strbuf *sys_info)\n {\n@@ -31,6 +33,53 @@ static void get_system_info(struct strbuf *sys_info)\n \tget_libc_info(sys_info);\n }\n \n+static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n+{\n+\t/*\n+\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n+\t * so below is a transcription of `git help hooks`. Later, this should\n+\t * be replaced with some programmatically generated list (generated from\n+\t * doc or else taken from some library which tells us about all the\n+\t * hooks)\n+\t */\n+\tstatic const char *hook[] = {\n+\t\t\"applypatch-msg\",\n+\t\t\"pre-applypatch\",\n+\t\t\"post-applypatch\",\n+\t\t\"pre-commit\",\n+\t\t\"pre-merge-commit\",\n+\t\t\"prepare-commit-msg\",\n+\t\t\"commit-msg\",\n+\t\t\"post-commit\",\n+\t\t\"pre-rebase\",\n+\t\t\"post-checkout\",\n+\t\t\"post-merge\",\n+\t\t\"pre-push\",\n+\t\t\"pre-receive\",\n+\t\t\"update\",\n+\t\t\"post-receive\",\n+\t\t\"post-update\",\n+\t\t\"push-to-checkout\",\n+\t\t\"pre-auto-gc\",\n+\t\t\"post-rewrite\",\n+\t\t\"sendemail-validate\",\n+\t\t\"fsmonitor-watchman\",\n+\t\t\"p4-pre-submit\",\n+\t\t\"post-index-change\",\n+\t};\n+\tint i;\n+\n+\tif (nongit) {\n+\t\tstrbuf_addstr(hook_info,\n+\t\t\t_(\"not run from a git repository - no hooks to show\\n\"));\n+\t\treturn;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(hook); i++)\n+\t\tif (find_hook(hook[i]))\n+\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n+}\n+\n static const char * const bugreport_usage[] = {\n \tN_(\"git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]\"),\n \tNULL\n@@ -114,6 +163,9 @@ int cmd_main(int argc, const char **argv)\n \tget_header(&buffer, _(\"System Info\"));\n \tget_system_info(&buffer);\n \n+\tget_header(&buffer, _(\"Enabled Hooks\"));\n+\tget_populated_hooks(&buffer, nongit_ok);\n+\n \t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n \treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \ndiff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\nindex 2e73658a5c..ff317cce68 100755\n--- a/t/t0091-bugreport.sh\n+++ b/t/t0091-bugreport.sh\n@@ -57,5 +57,20 @@ test_expect_success 'can create leading directories outside of a git dir' '\n \tnongit git bugreport -o foo/bar/baz\n '\n \n+test_expect_success 'indicates populated hooks' '\n+\ttest_when_finished rm git-bugreport-hooks.txt &&\n+\ttest_when_finished rm -fr .git/hooks &&\n+\trm -fr .git/hooks &&\n+\tmkdir .git/hooks &&\n+\tfor hook in applypatch-msg prepare-commit-msg.sample\n+\tdo\n+\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n+\t\techo \"hook $hook exists\"\n+\t\tEOF\n+\tdone &&\n+\tgit bugreport -s hooks &&\n+\tgrep applypatch-msg git-bugreport-hooks.txt &&\n+\t! grep prepare-commit-msg git-bugreport-hooks.txt\n+'\n \n test_done\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397387","messageId":"xmqqlfm31atm.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200508005357.223058-1-emilyshaffer@google.com","subject":"Re: [PATCH v4] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T01:20:53Z","receivedAt":"2020-05-08T01:20:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\n> index 2e73658a5c..ff317cce68 100755\n> --- a/t/t0091-bugreport.sh\n> +++ b/t/t0091-bugreport.sh\n> @@ -57,5 +57,20 @@ test_expect_success 'can create leading directories outside of a git dir' '\n>  \tnongit git bugreport -o foo/bar/baz\n>  '\n>  \n> +test_expect_success 'indicates populated hooks' '\n> +\ttest_when_finished rm git-bugreport-hooks.txt &&\n> +\ttest_when_finished rm -fr .git/hooks &&\n> +\trm -fr .git/hooks &&\n> +\tmkdir .git/hooks &&\n> +\tfor hook in applypatch-msg prepare-commit-msg.sample\n> +\tdo\n> +\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n> +\t\techo \"hook $hook exists\"\n> +\t\tEOF\n\nProbably our mails crossed, but as I said in my response, this will\nmake the hook say \"hook  exists\" because $hook will be literally in\nthe script file.  EOF needs to be left unquoted, i.e.\n\n\t\twrite_script \".git/hooks/$hook\" <<-EOF || return 1\n\t\t...\n\n> +\tdone &&\n> +\t...\n\nThanks.\n\n\n"},{"id":"397389","messageId":"20200508013405.GA2111@danh.dev","threadId":"53313","inReplyTo":"20200508005357.223058-1-emilyshaffer@google.com","subject":"Re: [PATCH v4] bugreport: collect list of populated hooks","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-08T01:34:05Z","receivedAt":"2020-05-08T01:34:14Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-07 17:53:57-0700, Emily Shaffer <emilyshaffer@google.com> wrote:\n> +test_expect_success 'indicates populated hooks' '\n> +\ttest_when_finished rm git-bugreport-hooks.txt &&\n> +\ttest_when_finished rm -fr .git/hooks &&\n> +\trm -fr .git/hooks &&\n> +\tmkdir .git/hooks &&\n> +\tfor hook in applypatch-msg prepare-commit-msg.sample\n> +\tdo\n> +\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n> +\t\techo \"hook $hook exists\"\n> +\t\tEOF\n> +\tdone &&\n> +\tgit bugreport -s hooks &&\n> +\tgrep applypatch-msg git-bugreport-hooks.txt &&\n> +\t! grep prepare-commit-msg git-bugreport-hooks.txt\n\nHi Emily,\n\nI think this is a bit more correct test.\n---------8<----------\nFrom: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=\n <congdanhqx@gmail.com>\nDate: Fri, 8 May 2020 08:26:53 +0700\nSubject: [PATCH] fixup! bugreport: collect list of populated hooks\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nUse an exact match to check for populated hooks\n\nSigned-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n---\n t/t0091-bugreport.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\nindex 9450cc02e3..789e8f1ac7 100755\n--- a/t/t0091-bugreport.sh\n+++ b/t/t0091-bugreport.sh\n@@ -67,8 +67,13 @@ test_expect_success 'indicates populated hooks' '\n \t\tEOF\n \tdone &&\n \tgit bugreport -s hooks &&\n-\tgrep applypatch-msg git-bugreport-hooks.txt &&\n-\t! grep prepare-commit-msg git-bugreport-hooks.txt\n+\tcat <<-\\EOF >expected &&\n+\tapplypatch-msg\n+\n+\tEOF\n+\tawk -F \"]\\\\n\" -v RS=\"[\" \"/applypatch-msg/{print \\$2}\" \\\n+\t\tgit-bugreport-hooks.txt >actual &&\n+\ttest_cmp expected actual\n '\n \n test_done\n-- \n2.26.2.672.g232c24e857\n\n\n-- \nDanh\n"},{"id":"397582","messageId":"20200511212205.GI77802@google.com","threadId":"53313","inReplyTo":"20200508013405.GA2111@danh.dev","subject":"Re: [PATCH v4] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-05-11T21:22:05Z","receivedAt":"2020-05-11T21:22:15Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, May 08, 2020 at 08:34:05AM +0700, Đoàn Trần Công Danh wrote:\n> \n> On 2020-05-07 17:53:57-0700, Emily Shaffer <emilyshaffer@google.com> wrote:\n> > +test_expect_success 'indicates populated hooks' '\n> > +\ttest_when_finished rm git-bugreport-hooks.txt &&\n> > +\ttest_when_finished rm -fr .git/hooks &&\n> > +\trm -fr .git/hooks &&\n> > +\tmkdir .git/hooks &&\n> > +\tfor hook in applypatch-msg prepare-commit-msg.sample\n> > +\tdo\n> > +\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n> > +\t\techo \"hook $hook exists\"\n> > +\t\tEOF\n> > +\tdone &&\n> > +\tgit bugreport -s hooks &&\n> > +\tgrep applypatch-msg git-bugreport-hooks.txt &&\n> > +\t! grep prepare-commit-msg git-bugreport-hooks.txt\n> \n> Hi Emily,\n> \n> I think this is a bit more correct test.\n> ---------8<----------\n> MIME-Version: 1.0\n> Content-Type: text/plain; charset=UTF-8\n> Content-Transfer-Encoding: 8bit\n> \n> Use an exact match to check for populated hooks\n> \n> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n> ---\n>  t/t0091-bugreport.sh | 9 +++++++--\n>  1 file changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\n> index 9450cc02e3..789e8f1ac7 100755\n> --- a/t/t0091-bugreport.sh\n> +++ b/t/t0091-bugreport.sh\n> @@ -67,8 +67,13 @@ test_expect_success 'indicates populated hooks' '\n>  \t\tEOF\n>  \tdone &&\n>  \tgit bugreport -s hooks &&\n> -\tgrep applypatch-msg git-bugreport-hooks.txt &&\n> -\t! grep prepare-commit-msg git-bugreport-hooks.txt\n> +\tcat <<-\\EOF >expected &&\n> +\tapplypatch-msg\n> +\n> +\tEOF\n> +\tawk -F \"]\\\\n\" -v RS=\"[\" \"/applypatch-msg/{print \\$2}\" \\\n> +\t\tgit-bugreport-hooks.txt >actual &&\n\nIf I understand correctly, you are saying \"look for a line like [...]\nwhich is followed by a line that says 'applypatch-msg'\" - that is,\nmaking sure that you don't see some false positive should\n\"applypatch-msg\" exist in the rest of the bugreport.\n\nCould we compromise and grep for \"^applypatch-msg$\", e.g. \"there is a\nline calling out applypatch-msg in some way that isn't in the context of\nthe report template\"?\n\n- Even though regex magic is used, ^$ are beginner regex that many\n  people can understand easily.\n- Using grep for a single line means we are not allergic to header\n  format changes later on\n\nIt doesn't search for false positives, true, but I found your awk\nsuggestion hard to understand - my impression is that awk is less\ncommonly understood, even among Git contributors.\n\nIn sed, it's a little bit more readable:\n\n  cat <<-\\EOF >expected &&\n  [Enabled Hooks]\n  applypatch-msg\n\n  EOF\n\n  sed -n '/\\[Enabled Hooks\\]/, /^$/ p' git-bugreport-hooks.txt >actual\n  test_cmp expected actual\n\nBut, I don't like this because it relies on the name of the header, and\nthe newline spacing between sections - and would be nontrivial to change\nif we decided to underline headers instead, or add a newline between a\nheader and its contents. So I think it may be overkill.\n\nThanks for your suggestion, though.\n\n - Emily\n"},{"id":"397583","messageId":"20200511212649.GJ77802@google.com","threadId":"53313","inReplyTo":"xmqqa72j2vmh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-05-11T21:26:49Z","receivedAt":"2020-05-11T21:26:58Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, May 07, 2020 at 04:06:14PM -0700, Junio C Hamano wrote:\n> \n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> >>  test_expect_success 'indicates populated hooks' '\n> >> -\ttest_when_finished rm git-bugreport-hooks.txt &&\n> >> -\ttest_when_finished rm -fr .git/hooks &&\n> >\n> > I'm not sure it's necessary to lose these two lines. Especially the\n> > generated bugreport I'd like to clean up.\n> \n> I do not care either way, actually.  I left it so that it would be\n> easier to debug the test by looking at the output.\n\nThat's a good point, although I was worried about the junk left behind\nfor other tests. I think I'd rather delete the junk, as people should\nstill be able to debug the test with \"test_pause &&\" and see the\ncontents of the temporary hook directory (verified locally).\n\n> \n> >> +\trm -fr .git/hooks &&\n> >>  \tmkdir .git/hooks &&\n> >> -\twrite_script .git/hooks/applypatch-msg &&\n> >> -\twrite_script .git/hooks/prepare-commit-msg.sample &&\n> >> +\tfor hook in applypatch-msg prepare-commit-msg.sample\n> >> +\tdo\n> >> +\t\twrite_script \".git/hooks/$hook\" <<-\\EOF || return 1\n> >> +\t\techo \"hook $hook exists\"\n> >> +\t\tEOF\n> >> +\tdone &&\n> >\n> > I like this placeholder script a lot.\n> \n> I actually don't.  At least the final version should not quote EOF\n> (otherwise $hook will appear verbatim).\n\nACK.\n\nNew patch inbound as discussed in standup.\n - Emily\n"},{"id":"397584","messageId":"20200511221430.174122-1-emilyshaffer@google.com","threadId":"53313","inReplyTo":"20200508005357.223058-1-emilyshaffer@google.com","subject":"[PATCH v5] bugreport: collect list of populated hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-05-11T22:14:30Z","receivedAt":"2020-05-11T22:14:38Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Occasionally a failure a user is seeing may be related to a specific\nhook which is being run, perhaps without the user realizing. While the\ncontents of hooks can be sensitive - containing user data or process\ninformation specific to the user's organization - simply knowing that a\nhook is being run at a certain stage can help us to understand whether\nsomething is going wrong.\n\nWithout a definitive list of hook names within the code, we compile our\nown list from the documentation. This is likely prone to bitrot, but\ndesigning a single source of truth for acceptable hooks is too much\noverhead for this small change to the bugreport tool.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nThe only change since v4 is to uncomment the heredoc, meaning the hook\nnames will be correctly injected into the test hooks.\n\nI chose not to implement Danh's suggestion of multiline grep:\nhttps://lore.kernel.org/git/20200511212205.GI77802@google.com\n\nTests passed:\nhttps://github.com/nasamuffin/git/actions/runs/101874479\n\nThanks, all.\n - Emily\n\n Documentation/git-bugreport.txt |  1 +\n bugreport.c                     | 52 +++++++++++++++++++++++++++++++++\n t/t0091-bugreport.sh            | 15 ++++++++++\n 3 files changed, 68 insertions(+)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex 643d1b2884..7fe9aef34e 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -28,6 +28,7 @@ The following information is captured automatically:\n  - 'git version --build-options'\n  - uname sysname, release, version, and machine strings\n  - Compiler-specific info string\n+ - A list of enabled hooks\n \n This tool is invoked via the typical Git setup process, which means that in some\n cases, it might not be able to launch - for example, if a relevant config file\ndiff --git a/bugreport.c b/bugreport.c\nindex acacca8fef..aa8a489c35 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -3,6 +3,8 @@\n #include \"strbuf.h\"\n #include \"help.h\"\n #include \"compat/compiler.h\"\n+#include \"run-command.h\"\n+\n \n static void get_system_info(struct strbuf *sys_info)\n {\n@@ -31,6 +33,53 @@ static void get_system_info(struct strbuf *sys_info)\n \tget_libc_info(sys_info);\n }\n \n+static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n+{\n+\t/*\n+\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n+\t * so below is a transcription of `git help hooks`. Later, this should\n+\t * be replaced with some programmatically generated list (generated from\n+\t * doc or else taken from some library which tells us about all the\n+\t * hooks)\n+\t */\n+\tstatic const char *hook[] = {\n+\t\t\"applypatch-msg\",\n+\t\t\"pre-applypatch\",\n+\t\t\"post-applypatch\",\n+\t\t\"pre-commit\",\n+\t\t\"pre-merge-commit\",\n+\t\t\"prepare-commit-msg\",\n+\t\t\"commit-msg\",\n+\t\t\"post-commit\",\n+\t\t\"pre-rebase\",\n+\t\t\"post-checkout\",\n+\t\t\"post-merge\",\n+\t\t\"pre-push\",\n+\t\t\"pre-receive\",\n+\t\t\"update\",\n+\t\t\"post-receive\",\n+\t\t\"post-update\",\n+\t\t\"push-to-checkout\",\n+\t\t\"pre-auto-gc\",\n+\t\t\"post-rewrite\",\n+\t\t\"sendemail-validate\",\n+\t\t\"fsmonitor-watchman\",\n+\t\t\"p4-pre-submit\",\n+\t\t\"post-index-change\",\n+\t};\n+\tint i;\n+\n+\tif (nongit) {\n+\t\tstrbuf_addstr(hook_info,\n+\t\t\t_(\"not run from a git repository - no hooks to show\\n\"));\n+\t\treturn;\n+\t}\n+\n+\tfor (i = 0; i < ARRAY_SIZE(hook); i++)\n+\t\tif (find_hook(hook[i]))\n+\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n+}\n+\n static const char * const bugreport_usage[] = {\n \tN_(\"git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]\"),\n \tNULL\n@@ -114,6 +163,9 @@ int cmd_main(int argc, const char **argv)\n \tget_header(&buffer, _(\"System Info\"));\n \tget_system_info(&buffer);\n \n+\tget_header(&buffer, _(\"Enabled Hooks\"));\n+\tget_populated_hooks(&buffer, nongit_ok);\n+\n \t/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */\n \treport = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \ndiff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\nindex 2e73658a5c..526304ff95 100755\n--- a/t/t0091-bugreport.sh\n+++ b/t/t0091-bugreport.sh\n@@ -57,5 +57,20 @@ test_expect_success 'can create leading directories outside of a git dir' '\n \tnongit git bugreport -o foo/bar/baz\n '\n \n+test_expect_success 'indicates populated hooks' '\n+\ttest_when_finished rm git-bugreport-hooks.txt &&\n+\ttest_when_finished rm -fr .git/hooks &&\n+\trm -fr .git/hooks &&\n+\tmkdir .git/hooks &&\n+\tfor hook in applypatch-msg prepare-commit-msg.sample\n+\tdo\n+\t\twrite_script \".git/hooks/$hook\" <<-EOF || return 1\n+\t\techo \"hook $hook exists\"\n+\t\tEOF\n+\tdone &&\n+\tgit bugreport -s hooks &&\n+\tgrep applypatch-msg git-bugreport-hooks.txt &&\n+\t! grep prepare-commit-msg git-bugreport-hooks.txt\n+'\n \n test_done\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397585","messageId":"20200511232629.GB141481@google.com","threadId":"53313","inReplyTo":"20200511221430.174122-1-emilyshaffer@google.com","subject":"Re: [PATCH v5] bugreport: collect list of populated hooks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-05-11T23:26:29Z","receivedAt":"2020-05-11T23:26:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nEmily Shaffer wrote:\n\n> Occasionally a failure a user is seeing may be related to a specific\n> hook which is being run, perhaps without the user realizing. While the\n> contents of hooks can be sensitive - containing user data or process\n> information specific to the user's organization - simply knowing that a\n> hook is being run at a certain stage can help us to understand whether\n> something is going wrong.\n>\n> Without a definitive list of hook names within the code, we compile our\n> own list from the documentation. This is likely prone to bitrot, but\n> designing a single source of truth for acceptable hooks is too much\n> overhead for this small change to the bugreport tool.\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  Documentation/git-bugreport.txt |  1 +\n>  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++\n>  t/t0091-bugreport.sh            | 15 ++++++++++\n>  3 files changed, 68 insertions(+)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"397586","messageId":"xmqqd07aqbmb.fsf@gitster.c.googlers.com","threadId":"53313","inReplyTo":"20200511221430.174122-1-emilyshaffer@google.com","subject":"Re: [PATCH v5] bugreport: collect list of populated hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-11T23:45:48Z","receivedAt":"2020-05-11T23:45:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> The only change since v4 is to uncomment the heredoc, meaning the hook\n> names will be correctly injected into the test hooks.\n\nWhich means that this is identical to what we had in 'pu' for a\ncouple of days already ;-)\n\nLet's mark it as ready to be merged to 'next' and move on.\n\nThanks.\n"}]}