| Message ID | 20200427233820.179891-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] bugreport: collect list of populated hooks | expand |
Emily Shaffer wrote: > --- a/t/t0091-bugreport.sh > +++ b/t/t0091-bugreport.sh > @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' ' > nongit git bugreport -o foo/bar/baz > ' > > +test_expect_success 'indicates populated hooks' ' > + test_when_finished rm git-bugreport-hooks.txt && > + test_when_finished rm -fr .git/hooks && > + mkdir .git/hooks && > + touch .git/hooks/applypatch-msg && > + chmod +x .git/hooks/applypatch-msg && optional: could use write_script for this > + git bugreport -s hooks && > + test_i18ngrep applypatch-msg git-bugreport-hooks.txt This should work even when translated, so it can use "grep" instead of test_i18ngrep. A few more things to test: - that it doesn't include hooks we *haven't* installed. :) - that it isn't confused by the default *.sample hooks - what happens when outside a git repository? Even without such tests, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Thanks.
Jonathan Nieder <jrnieder@gmail.com> writes: > Emily Shaffer wrote: > >> --- a/t/t0091-bugreport.sh >> +++ b/t/t0091-bugreport.sh >> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' ' >> nongit git bugreport -o foo/bar/baz >> ' >> >> +test_expect_success 'indicates populated hooks' ' >> + test_when_finished rm git-bugreport-hooks.txt && >> + test_when_finished rm -fr .git/hooks && >> + mkdir .git/hooks && >> + touch .git/hooks/applypatch-msg && >> + chmod +x .git/hooks/applypatch-msg && > > optional: could use write_script for this Yup, it is a good practice to do so. Use of touch is especially bad here, as it is quite plausible for us to later change the "is the hook there?" check to exclude a completely empty file. >> + git bugreport -s hooks && >> + test_i18ngrep applypatch-msg git-bugreport-hooks.txt > > This should work even when translated, so it can use "grep" instead of > test_i18ngrep. Nicely spotted. Thanks.
On Mon, Apr 27, 2020 at 04:45:10PM -0700, Jonathan Nieder wrote: > > Emily Shaffer wrote: > > > --- a/t/t0091-bugreport.sh > > +++ b/t/t0091-bugreport.sh > > @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' ' > > nongit git bugreport -o foo/bar/baz > > ' > > > > +test_expect_success 'indicates populated hooks' ' > > + test_when_finished rm git-bugreport-hooks.txt && > > + test_when_finished rm -fr .git/hooks && > > + mkdir .git/hooks && > > + touch .git/hooks/applypatch-msg && > > + chmod +x .git/hooks/applypatch-msg && > > optional: could use write_script for this ACK, especially given Junio's comment in reply here. > > > + git bugreport -s hooks && > > + test_i18ngrep applypatch-msg git-bugreport-hooks.txt > > This should work even when translated, so it can use "grep" instead of > test_i18ngrep. > > A few more things to test: > - that it doesn't include hooks we *haven't* installed. :) > - that it isn't confused by the default *.sample hooks I'll lump these together by adding a .sample hook and ensuring that hook was excluded. > - what happens when outside a git repository? The earlier test to make sure it doesn't crash seems satisfactory to me - is there some other behavior you'd like to ensure? I'm not keen to check the lines below the header are empty, since the spacing there could change and break an unrelated test. Reroll coming today. These changes are straightforward and passing for me locally. - Emily
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..81904b508e 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) + */ + static 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); diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh index 2e73658a5c..387fa46c3f 100755 --- a/t/t0091-bugreport.sh +++ b/t/t0091-bugreport.sh @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' ' nongit git bugreport -o foo/bar/baz ' +test_expect_success 'indicates populated hooks' ' + test_when_finished rm git-bugreport-hooks.txt && + test_when_finished rm -fr .git/hooks && + mkdir .git/hooks && + touch .git/hooks/applypatch-msg && + chmod +x .git/hooks/applypatch-msg && + git bugreport -s hooks && + test_i18ngrep applypatch-msg git-bugreport-hooks.txt +' + test_done
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, but designing a single source of truth for acceptable hooks is too much overhead for this small change to the bugreport tool. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Since v1: - turned array of hooknames into a static const - added a test - i18n-ified the outputs added in this section - style changes Thanks, all. - Emily Documentation/git-bugreport.txt | 1 + bugreport.c | 52 +++++++++++++++++++++++++++++++++ t/t0091-bugreport.sh | 10 +++++++ 3 files changed, 63 insertions(+)