| Message ID | 20200220015858.181086-12-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > +static void get_populated_hooks(struct strbuf *hook_info, int nongit) > +{ > + /* > + * Doesn't look like there is a list of all possible hooks; so below is > + * a transcription of `git help hooks`. > + */ It may want to become a NEEDSWORK comment. > + const char *hooks = "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-changex"; Typo here? > + struct string_list hooks_list = STRING_LIST_INIT_DUP; > + struct string_list_item *iter = NULL; > + > + > + if (nongit) { > + strbuf_addstr(hook_info, > + "not run from a git repository - no hooks to show\n"); > + return; > + } > + > + string_list_split(&hooks_list, hooks, ',', -1); > + > + for_each_string_list_item(iter, &hooks_list) { I do not get why you want to use string_list for this, especially if you need to use string_list_split. To me, int i; const char *hook[] = { "applypatch-msg", "pre-applypatch", ... "post-index-change", }; for (i = 0; i < ARRAY_SIZE(hook); i++) { if (hook[i] is enabled) strbuf_addf(hook_info, "%s\n", hook[i]); } would be far easier to understand. Do you have an external source that can feed you a single long string of comma separated hook names in mind, so that the initialization of *hooks will become simpler that way, or something? > + if (find_hook(iter->string)) { > + strbuf_addstr(hook_info, iter->string); > + strbuf_complete_line(hook_info); > + } > + } > +} > + > static const char * const bugreport_usage[] = { > N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), > NULL > @@ -166,6 +216,9 @@ int cmd_main(int argc, const char **argv) > get_header(&buffer, "Safelisted Config Info"); > get_safelisted_config(&buffer); > > + get_header(&buffer, "Enabled Hooks"); > + get_populated_hooks(&buffer, nongit_ok); > + > report = fopen_for_writing(report_path.buf); > > if (report == NULL) {
On Thu, Feb 20, 2020 at 12:58:32PM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > +static void get_populated_hooks(struct strbuf *hook_info, int nongit) > > +{ > > + /* > > + * Doesn't look like there is a list of all possible hooks; so below is > > + * a transcription of `git help hooks`. > > + */ > > It may want to become a NEEDSWORK comment. Oh, good point. I'll rephrase it. > > > + const char *hooks = "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-changex"; > > Typo here? Yep, I imagine from trying to press 'x' to remove the ',' from my list. > > > + struct string_list hooks_list = STRING_LIST_INIT_DUP; > > + struct string_list_item *iter = NULL; > > + > > + > > + if (nongit) { > > + strbuf_addstr(hook_info, > > + "not run from a git repository - no hooks to show\n"); > > + return; > > + } > > + > > + string_list_split(&hooks_list, hooks, ',', -1); > > + > > + for_each_string_list_item(iter, &hooks_list) { > > I do not get why you want to use string_list for this, especially if > you need to use string_list_split. > > To me, > > int i; > const char *hook[] = { > "applypatch-msg", > "pre-applypatch", > ... > "post-index-change", > }; > > for (i = 0; i < ARRAY_SIZE(hook); i++) { > if (hook[i] is enabled) > strbuf_addf(hook_info, "%s\n", hook[i]); > } > > would be far easier to understand. Do you have an external source > that can feed you a single long string of comma separated hook names > in mind, so that the initialization of *hooks will become simpler > that way, or something? Huh. I'm having a hard time remembering why I did it this way. I think I wanted to use the string_list foreach? But you're right that it is pretty hard to understand, plus expensive; I'll just use an array like you suggest. > > > + if (find_hook(iter->string)) { > > + strbuf_addstr(hook_info, iter->string); > > + strbuf_complete_line(hook_info); > > + } > > + } > > +} > > + > > static const char * const bugreport_usage[] = { > > N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), > > NULL > > @@ -166,6 +216,9 @@ int cmd_main(int argc, const char **argv) > > get_header(&buffer, "Safelisted Config Info"); > > get_safelisted_config(&buffer); > > > > + get_header(&buffer, "Enabled Hooks"); > > + get_populated_hooks(&buffer, nongit_ok); > > + > > report = fopen_for_writing(report_path.buf); > > > > if (report == NULL) {
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index efe9719ea4..4d01528540 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -31,6 +31,7 @@ The following information is captured automatically: - 'git remote-https --build-info' - $SHELL - Selected config values + - 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 70fb3b8fa6..b5a0714a7f 100644 --- a/bugreport.c +++ b/bugreport.c @@ -9,6 +9,7 @@ #include "config.h" #include "bugreport-config-safelist.h" #include "khash.h" +#include "run-command.h" static void get_git_remote_https_version_info(struct strbuf *version_info) { @@ -78,6 +79,55 @@ static void get_safelisted_config(struct strbuf *config_info) } } +static void get_populated_hooks(struct strbuf *hook_info, int nongit) +{ + /* + * Doesn't look like there is a list of all possible hooks; so below is + * a transcription of `git help hooks`. + */ + const char *hooks = "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-changex"; + struct string_list hooks_list = STRING_LIST_INIT_DUP; + struct string_list_item *iter = NULL; + + + if (nongit) { + strbuf_addstr(hook_info, + "not run from a git repository - no hooks to show\n"); + return; + } + + string_list_split(&hooks_list, hooks, ',', -1); + + for_each_string_list_item(iter, &hooks_list) { + if (find_hook(iter->string)) { + strbuf_addstr(hook_info, iter->string); + strbuf_complete_line(hook_info); + } + } +} + static const char * const bugreport_usage[] = { N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), NULL @@ -166,6 +216,9 @@ int cmd_main(int argc, const char **argv) get_header(&buffer, "Safelisted Config Info"); get_safelisted_config(&buffer); + get_header(&buffer, "Enabled Hooks"); + get_populated_hooks(&buffer, nongit_ok); + report = fopen_for_writing(report_path.buf); if (report == NULL) {
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> --- Documentation/git-bugreport.txt | 1 + bugreport.c | 53 +++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)