| Message ID | 20200424233800.200439-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bugreport: collect list of populated hooks | expand |
Emily Shaffer wrote: > Occasionally a failure a user is seeing may be related to a specific > hook which is being run, perhaps without the user realizing. While the > contents of hooks can be sensitive - containing user data or process > information specific to the user's organization - simply knowing that a > hook is being run at a certain stage can help us to understand whether > something is going wrong. Nice. [...] > Documentation/git-bugreport.txt | 1 + > bugreport.c | 52 +++++++++++++++++++++++++++++++++ > 2 files changed, 53 insertions(+) Can this functionality be demonstrated in a test? > diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt > index 643d1b2884..7fe9aef34e 100644 > --- a/Documentation/git-bugreport.txt > +++ b/Documentation/git-bugreport.txt > @@ -28,6 +28,7 @@ The following information is captured automatically: > - 'git version --build-options' > - uname sysname, release, version, and machine strings > - Compiler-specific info string > + - A list of enabled hooks > > This tool is invoked via the typical Git setup process, which means that in some > cases, it might not be able to launch - for example, if a relevant config file > diff --git a/bugreport.c b/bugreport.c > index 089b939a87..ce32145bce 100644 > --- a/bugreport.c > +++ b/bugreport.c > @@ -5,6 +5,8 @@ > #include "time.h" Not about this patch: this is for the system header <time.h>, right? Git includes the same system headers in (almost) all source files, via cache.h or git-compat-util.h, so it should be possible to leave this #include out. (Handling it in one place gives us a chance to get portability gotchas around names of headers, macros like _POSIX_SOURCE, and so on right). [...] > + /* > + * NEEDSWORK: Doesn't look like there is a list of all possible hooks; > + * so below is a transcription of `git help hooks`. Later, this should > + * be replaced with some programmatically generated list (generated from > + * doc or else taken from some library which tells us about all the > + * hooks) > + */ > + const char *hook[] = { > + "applypatch-msg", > + "pre-applypatch", > + "post-applypatch", > + "pre-commit", > + "pre-merge-commit", > + "prepare-commit-msg", > + "commit-msg", > + "post-commit", > + "pre-rebase", > + "post-checkout", > + "post-merge", > + "pre-push", > + "pre-receive", > + "update", > + "post-receive", > + "post-update", > + "push-to-checkout", > + "pre-auto-gc", > + "post-rewrite", > + "sendemail-validate", > + "fsmonitor-watchman", > + "p4-pre-submit", > + "post-index-change", > + }; Interesting. It would be possible to do some gettext-style trickery involving scanning for run_hook calls, but converting to an enum as you've suggested previously sounds simpler. Thanks, Jonathan
Emily Shaffer <emilyshaffer@google.com> writes: > Without a definitive list of hook names within the code, we compile our > own list from the documentation. This is likely prone to bitrot. To > reduce the amount of code humans need to read, we turn the list into a > string_list and iterate over it (as we are calling the same find_hook > operation on each string). However, since bugreport should primarily be > called by the user, the performance loss from massaging the string > seems acceptable. In this iteration we no longer are collecting the hook names into string list, but just formating the findings in a strbuf, no? > @@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info) > get_libc_info(sys_info); > } > > +static void get_populated_hooks(struct strbuf *hook_info, int nongit) > +{ > + /* > + * NEEDSWORK: Doesn't look like there is a list of all possible hooks; > + * so below is a transcription of `git help hooks`. Later, this should > + * be replaced with some programmatically generated list (generated from > + * doc or else taken from some library which tells us about all the > + * hooks) > + */ Yes, I recall that we discussed adding some annotation to documentation and extracting this automatically. > + const char *hook[] = { Is it worth making this "static const"? > + if (nongit) { > + strbuf_addstr(hook_info, > + "not run from a git repository - no hooks to show\n"); > + return; > + } > + > + for (i=0; i < ARRAY_SIZE(hook); i++) Style: SP around = in "i=0". > + get_header(&buffer, "Enabled Hooks"); > + get_populated_hooks(&buffer, nongit_ok); > + Sounds good, otherwise.
On Fri, Apr 24, 2020 at 09:52:07PM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > Without a definitive list of hook names within the code, we compile our > > own list from the documentation. This is likely prone to bitrot. To > > reduce the amount of code humans need to read, we turn the list into a > > string_list and iterate over it (as we are calling the same find_hook > > operation on each string). However, since bugreport should primarily be > > called by the user, the performance loss from massaging the string > > seems acceptable. > > In this iteration we no longer are collecting the hook names into > string list, but just formating the findings in a strbuf, no? Right - I'll fix the commit message for round 2. > > > @@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info) > > get_libc_info(sys_info); > > } > > > > +static void get_populated_hooks(struct strbuf *hook_info, int nongit) > > +{ > > + /* > > + * NEEDSWORK: Doesn't look like there is a list of all possible hooks; > > + * so below is a transcription of `git help hooks`. Later, this should > > + * be replaced with some programmatically generated list (generated from > > + * doc or else taken from some library which tells us about all the > > + * hooks) > > + */ > > Yes, I recall that we discussed adding some annotation to > documentation and extracting this automatically. Right, we did. I think I was hesitant to move on it because I had https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com on my back burner and wasn't sure how the hook architecture would look afterwards. I'm not sure I know the right way to move forward (which is why I left a NEEDSWORK) - my strongest preference is to leave it as is and wait for the linked RFC and related work to land, at which point this code will need a rework anyways. But if we're very interested in generating an enum or something from the docs rather than letting this gross char** land, I can do that too. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: >> > + /* >> > + * NEEDSWORK: Doesn't look like there is a list of all possible hooks; >> > + * so below is a transcription of `git help hooks`. Later, this should >> > + * be replaced with some programmatically generated list (generated from >> > + * doc or else taken from some library which tells us about all the >> > + * hooks) >> > + */ >> >> Yes, I recall that we discussed adding some annotation to >> documentation and extracting this automatically. > > Right, we did. I think I was hesitant to move on it because I had > https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com > on my back burner and wasn't sure how the hook architecture would look > afterwards. I thought we already agreed back then during that discussion I recalled that we'd leave that "single source of truth" unification out of the current topic. Unless anything has changed recently to bring us closer to be ready to start designing, I think it still is fine to keep the needs-work comment above and punting. Thanks.
On Mon, Apr 27, 2020 at 01:46:38PM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > >> > + /* > >> > + * NEEDSWORK: Doesn't look like there is a list of all possible hooks; > >> > + * so below is a transcription of `git help hooks`. Later, this should > >> > + * be replaced with some programmatically generated list (generated from > >> > + * doc or else taken from some library which tells us about all the > >> > + * hooks) > >> > + */ > >> > >> Yes, I recall that we discussed adding some annotation to > >> documentation and extracting this automatically. > > > > Right, we did. I think I was hesitant to move on it because I had > > https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com > > on my back burner and wasn't sure how the hook architecture would look > > afterwards. > > I thought we already agreed back then during that discussion I > recalled that we'd leave that "single source of truth" unification > out of the current topic. Unless anything has changed recently to > bring us closer to be ready to start designing, I think it still is > fine to keep the needs-work comment above and punting. Sure. Thanks.
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index 643d1b2884..7fe9aef34e 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -28,6 +28,7 @@ The following information is captured automatically: - 'git version --build-options' - uname sysname, release, version, and machine strings - Compiler-specific info string + - A list of enabled hooks This tool is invoked via the typical Git setup process, which means that in some cases, it might not be able to launch - for example, if a relevant config file diff --git a/bugreport.c b/bugreport.c index 089b939a87..ce32145bce 100644 --- a/bugreport.c +++ b/bugreport.c @@ -5,6 +5,8 @@ #include "time.h" #include "help.h" #include "compat/compiler.h" +#include "run-command.h" + static void get_system_info(struct strbuf *sys_info) { @@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info) get_libc_info(sys_info); } +static void get_populated_hooks(struct strbuf *hook_info, int nongit) +{ + /* + * NEEDSWORK: Doesn't look like there is a list of all possible hooks; + * so below is a transcription of `git help hooks`. Later, this should + * be replaced with some programmatically generated list (generated from + * doc or else taken from some library which tells us about all the + * hooks) + */ + const char *hook[] = { + "applypatch-msg", + "pre-applypatch", + "post-applypatch", + "pre-commit", + "pre-merge-commit", + "prepare-commit-msg", + "commit-msg", + "post-commit", + "pre-rebase", + "post-checkout", + "post-merge", + "pre-push", + "pre-receive", + "update", + "post-receive", + "post-update", + "push-to-checkout", + "pre-auto-gc", + "post-rewrite", + "sendemail-validate", + "fsmonitor-watchman", + "p4-pre-submit", + "post-index-change", + }; + int i; + + if (nongit) { + strbuf_addstr(hook_info, + "not run from a git repository - no hooks to show\n"); + return; + } + + for (i=0; i < ARRAY_SIZE(hook); i++) + if (find_hook(hook[i])) + strbuf_addf(hook_info, "%s\n", hook[i]); +} + static const char * const bugreport_usage[] = { N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), NULL @@ -116,6 +165,9 @@ int cmd_main(int argc, const char **argv) get_header(&buffer, _("System Info")); get_system_info(&buffer); + get_header(&buffer, "Enabled Hooks"); + get_populated_hooks(&buffer, nongit_ok); + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
Occasionally a failure a user is seeing may be related to a specific hook which is being run, perhaps without the user realizing. While the contents of hooks can be sensitive - containing user data or process information specific to the user's organization - simply knowing that a hook is being run at a certain stage can help us to understand whether something is going wrong. Without a definitive list of hook names within the code, we compile our own list from the documentation. This is likely prone to bitrot. To reduce the amount of code humans need to read, we turn the list into a string_list and iterate over it (as we are calling the same find_hook operation on each string). However, since bugreport should primarily be called by the user, the performance loss from massaging the string seems acceptable. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Based on es/bugreport. Documentation/git-bugreport.txt | 1 + bugreport.c | 52 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+)