Re: [PATCH v3] bugreport: collect list of populated hooks

[Jonathan Nieder <jrnieder@gmail.com>]

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