| Message ID | 20200430012425.209122-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] bugreport: collect list of populated hooks | expand |
Emily Shaffer wrote: > Since v2, switched to write_script, and modified the test to check for > uninstalled hooks and sample hooks having correct behavior. Looks almost ready. Two nits. [...] > --- 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 && This is subtle. Since c09a69a83e3 (Disable hooks during tests, 2005-10-16), the repository in which the test runs has its "hooks" directory renamed to "hooks-disabled", with rationale Individual tests for hooks would want to have their own tests when written. Also we should not pick up from random templates the user happens to have. That rationale is a bit strange because we explicitly passed --template to "git init" even then, so we could be confident that the built-in templates that do not enable any hooks would be in use. Mailing list diving finds [1]. We didn't have f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24) yet, which means that on filesystems that do not record an executable bit, the sample hooks were all enabled by default. Nowadays that has been fixed and we should be able to get rid of the "mv .git/hooks .git/hooks-disabled" in the test setup. When we do that, this "mkdir .git/hooks" will fail because the directory already exists. Ideas: A. Include a preparatory patch in this series that removes that "mv" command. That way, this test can do test_when_finished " rm -fr .git/hooks && mv .git/hooks-saved .git/hooks " && mv .git/hooks .git/hooks-saved && mkdir .git/hooks && ... or even better (because of increased realism) test_when_finished rm .git/hooks/applypatch-msg && write_script .git/hooks/applypatch-msg <<-\EOF && ... B. Run "git init" ourselves so we know what we're getting: test_when_finished "rm -fr .git && mv .git-saved .git" && mv .git .git-saved && git init && ... > + write_script .git/hooks/applypatch-msg && write_script looks for a script on its stdin. test_eval_ redirects stdin to come from /dev/null, so we happen to get an empty script, but this is subtle. How about something like write_script .git/hooks/applypatch-msg <<-\EOF && echo >&2 "rejecting message in $1" exit 1 EOF or write_script .git/hooks/applypatch-msg </dev/null && ? > + write_script .git/hooks/prepare-commit-msg.sample && Likewise. > + git bugreport -s hooks && > + grep applypatch-msg git-bugreport-hooks.txt && > + ! grep prepare-commit-msg git-bugreport-hooks.txt > +' Thanks, Jonathan
Jonathan Nieder wrote: > Emily Shaffer wrote: >> +++ 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 && > > This is subtle. Since c09a69a83e3 (Disable hooks during tests, > 2005-10-16), the repository in which the test runs has its "hooks" > directory renamed to "hooks-disabled", with rationale > > Individual tests for hooks would want to have their own tests when > written. Also we should not pick up from random templates the user > happens to have. > > That rationale is a bit strange because we explicitly passed > --template to "git init" even then, so we could be confident that the > built-in templates that do not enable any hooks would be in use. > > Mailing list diving finds [1]. We didn't have f98f8cbac01 (Ship > sample hooks with .sample suffix, 2008-06-24) yet, which means that on > filesystems that do not record an executable bit, the sample hooks > were all enabled by default. Nowadays that has been fixed and we > should be able to get rid of the "mv .git/hooks .git/hooks-disabled" > in the test setup. And of course I forget to include the footnote. Sorry for the noise. Jonathan [1] https://lore.kernel.org/git/81b0412b0510140546ya10bc8fg3dd5eaab429eba6f@mail.gmail.com/
Jonathan Nieder <jrnieder@gmail.com> writes: > When we do that, this "mkdir .git/hooks" will fail because the > directory already exists. Ideas: > > A. Include a preparatory patch in this series that removes that "mv" > command. That way, this test can do While I do not think it is realistic to anticipate that the "test" repository may someday come with a hooks/ directory, even if we did so, we would not enable any hook by default in there. So "move away and restore" feels way overkill. > B. Run "git init" ourselves so we know what we're getting: That is certainly safer, and simpler. But perhaps the simplest would be C. Use "mkdir -p .git/hooks" so we won't get affected. >> + write_script .git/hooks/applypatch-msg && > > write_script looks for a script on its stdin. test_eval_ redirects > stdin to come from /dev/null, so we happen to get an empty script, but > this is subtle. How about something like > > write_script .git/hooks/applypatch-msg <<-\EOF && > echo >&2 "rejecting message in $1" > exit 1 > EOF Yes, that is good. > or > > write_script .git/hooks/applypatch-msg </dev/null && This takes us back to the resuling "empty" hook we wanted to avoid by switching from "use touch to create something" to "write some meaningful contents" approach, no? >> + git bugreport -s hooks && >> + grep applypatch-msg git-bugreport-hooks.txt && >> + ! grep prepare-commit-msg git-bugreport-hooks.txt >> +' > > Thanks, Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> When we do that, this "mkdir .git/hooks" will fail because the >> directory already exists. Ideas: >> >> A. Include a preparatory patch in this series that removes that "mv" >> command. That way, this test can do > > While I do not think it is realistic to anticipate that the "test" > repository may someday come with a hooks/ directory, even if we did > so, we would not enable any hook by default in there. So "move away > and restore" feels way overkill. > >> B. Run "git init" ourselves so we know what we're getting: > > That is certainly safer, and simpler. But perhaps the simplest > would be > > C. Use "mkdir -p .git/hooks" so we won't get affected. In the meantime, I added this SQUASH on top. I do not claim that this is the best solution, but the idea is to refuse to be affected by what is left in .git/hooks either by the test framework or earlier tests in the same test script file. t/t0091-bugreport.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh index 612c12a918..9450cc02e3 100755 --- a/t/t0091-bugreport.sh +++ b/t/t0091-bugreport.sh @@ -58,11 +58,14 @@ test_expect_success 'can create leading directories outside of a git dir' ' ' test_expect_success 'indicates populated hooks' ' - test_when_finished rm git-bugreport-hooks.txt && - test_when_finished rm -fr .git/hooks && + rm -fr .git/hooks && mkdir .git/hooks && - write_script .git/hooks/applypatch-msg && - write_script .git/hooks/prepare-commit-msg.sample && + for hook in applypatch-msg prepare-commit-msg.sample + do + write_script ".git/hooks/$hook" <<-\EOF || return 1 + echo "hook $hook exists" + EOF + done && git bugreport -s hooks && grep applypatch-msg git-bugreport-hooks.txt && ! grep prepare-commit-msg git-bugreport-hooks.txt
On Thu, Apr 30, 2020 at 03:09:55PM -0700, Junio C Hamano wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > >> When we do that, this "mkdir .git/hooks" will fail because the > >> directory already exists. Ideas: > >> > >> A. Include a preparatory patch in this series that removes that "mv" > >> command. That way, this test can do > > > > While I do not think it is realistic to anticipate that the "test" > > repository may someday come with a hooks/ directory, even if we did > > so, we would not enable any hook by default in there. So "move away > > and restore" feels way overkill. > > > >> B. Run "git init" ourselves so we know what we're getting: > > > > That is certainly safer, and simpler. But perhaps the simplest > > would be > > > > C. Use "mkdir -p .git/hooks" so we won't get affected. > > In the meantime, I added this SQUASH on top. I do not claim that > this is the best solution, but the idea is to refuse to be affected > by what is left in .git/hooks either by the test framework or > earlier tests in the same test script file. Thanks for the patience with the reply - like many of us, my attention is being pulled many directions right now :) This solution looks very neat to me, minus one comment. I can send a squash with the change I mention below. - Emily > > t/t0091-bugreport.sh | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh > index 612c12a918..9450cc02e3 100755 > --- a/t/t0091-bugreport.sh > +++ b/t/t0091-bugreport.sh > @@ -58,11 +58,14 @@ test_expect_success 'can create leading directories outside of a git dir' ' > ' > > test_expect_success 'indicates populated hooks' ' > - test_when_finished rm git-bugreport-hooks.txt && > - test_when_finished rm -fr .git/hooks && I'm not sure it's necessary to lose these two lines. Especially the generated bugreport I'd like to clean up. > + rm -fr .git/hooks && > mkdir .git/hooks && > - write_script .git/hooks/applypatch-msg && > - write_script .git/hooks/prepare-commit-msg.sample && > + for hook in applypatch-msg prepare-commit-msg.sample > + do > + write_script ".git/hooks/$hook" <<-\EOF || return 1 > + echo "hook $hook exists" > + EOF > + done && I like this placeholder script a lot. > git bugreport -s hooks && > grep applypatch-msg git-bugreport-hooks.txt && > ! grep prepare-commit-msg git-bugreport-hooks.txt > -- > 2.26.2-447-gd61d20c9b4 >
Emily Shaffer <emilyshaffer@google.com> writes: >> test_expect_success 'indicates populated hooks' ' >> - test_when_finished rm git-bugreport-hooks.txt && >> - test_when_finished rm -fr .git/hooks && > > I'm not sure it's necessary to lose these two lines. Especially the > generated bugreport I'd like to clean up. I do not care either way, actually. I left it so that it would be easier to debug the test by looking at the output. >> + rm -fr .git/hooks && >> mkdir .git/hooks && >> - write_script .git/hooks/applypatch-msg && >> - write_script .git/hooks/prepare-commit-msg.sample && >> + for hook in applypatch-msg prepare-commit-msg.sample >> + do >> + write_script ".git/hooks/$hook" <<-\EOF || return 1 >> + echo "hook $hook exists" >> + EOF >> + done && > > I like this placeholder script a lot. I actually don't. At least the final version should not quote EOF (otherwise $hook will appear verbatim). >> git bugreport -s hooks && >> grep applypatch-msg git-bugreport-hooks.txt && >> ! grep prepare-commit-msg git-bugreport-hooks.txt >> -- >> 2.26.2-447-gd61d20c9b4 >> Thanks.
On Thu, May 07, 2020 at 04:06:14PM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > >> test_expect_success 'indicates populated hooks' ' > >> - test_when_finished rm git-bugreport-hooks.txt && > >> - test_when_finished rm -fr .git/hooks && > > > > I'm not sure it's necessary to lose these two lines. Especially the > > generated bugreport I'd like to clean up. > > I do not care either way, actually. I left it so that it would be > easier to debug the test by looking at the output. That's a good point, although I was worried about the junk left behind for other tests. I think I'd rather delete the junk, as people should still be able to debug the test with "test_pause &&" and see the contents of the temporary hook directory (verified locally). > > >> + rm -fr .git/hooks && > >> mkdir .git/hooks && > >> - write_script .git/hooks/applypatch-msg && > >> - write_script .git/hooks/prepare-commit-msg.sample && > >> + for hook in applypatch-msg prepare-commit-msg.sample > >> + do > >> + write_script ".git/hooks/$hook" <<-\EOF || return 1 > >> + echo "hook $hook exists" > >> + EOF > >> + done && > > > > I like this placeholder script a lot. > > I actually don't. At least the final version should not quote EOF > (otherwise $hook will appear verbatim). ACK. New patch inbound as discussed in standup. - 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 acacca8fef..aa8a489c35 100644 --- a/bugreport.c +++ b/bugreport.c @@ -3,6 +3,8 @@ #include "strbuf.h" #include "help.h" #include "compat/compiler.h" +#include "run-command.h" + static void get_system_info(struct strbuf *sys_info) { @@ -31,6 +33,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 @@ -114,6 +163,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..612c12a918 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 && + write_script .git/hooks/applypatch-msg && + write_script .git/hooks/prepare-commit-msg.sample && + git bugreport -s hooks && + grep applypatch-msg git-bugreport-hooks.txt && + ! grep prepare-commit-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 v2, switched to write_script, and modified the test to check for uninstalled hooks and sample hooks having correct behavior. Documentation/git-bugreport.txt | 1 + bugreport.c | 52 +++++++++++++++++++++++++++++++++ t/t0091-bugreport.sh | 10 +++++++ 3 files changed, 63 insertions(+)