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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 30, 2020, 01:50 UTC
Message-ID: <20200430015049.GA115238@google.com>
In-Reply-To: <20200430012425.209122-1-emilyshaffer@google.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.

[...]

Show 10 quoted lines

> --- 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