Re: [PATCH v2] hooks: Add ability to specify where the hook directory is

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +core.hooksPath::
> +	By default Git will look for your hooks in the
> +	'$GIT_DIR/hooks' directory. Set this to different path,
> +	e.g. '/etc/git/hooks', and Git will try to find your hooks in
> +	that directory, e.g. '/etc/git/hooks/pre-receive' instead of
> +	in '$GIT_DIR/hooks/pre-receive'.
> ++
> +The path can either be absolute or relative. In the latter case see
> +the discussion in the "DESCRIPTION" section of linkgit:githooks[5]
> +about what the relative path will be relative to.

... which does not seem to appear there, it seems?

>  DESCRIPTION
>  -----------
>  
> -Hooks are programs you can place in the `$GIT_DIR/hooks` directory to
> -trigger action at certain points. Hooks that don't have the executable
> -bit set are ignored.
> +Hooks are programs you can place in a hooks directory to trigger action
> +at certain points. Hooks that don't have the executable bit set are
> +ignored.
> +
> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be
> +changed via the `core.hooksPath` configuration variable (see
> +linkgit:git-config[1]).

The section talks about what the cwd of the process that runs the
hook is, but it is not clear at all from these three lines in
core.hooksPath description above how the cwd of the process is
related with the directory the relative path will be relative to.

Unless the readers know that the implementation makes sure that the
process chdir'ed to that final directory before calling find_hook(),
that is.  And I do not think you want to assume that knowledge on
the side of the readers.

> diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
> new file mode 100755
> index 0000000..31461aa
> --- /dev/null
> +++ b/t/t1350-config-hooks-path.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='Test the core.hooksPath configuration variable'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'set up a pre-commit hook in core.hooksPath' '
> +	mkdir -p .git/custom-hooks .git/hooks &&
> +	write_script .git/custom-hooks/pre-commit <<EOF &&
> +printf "%s" "." >>.git/PRE-COMMIT-HOOK-WAS-CALLED
> +EOF

Because there is no funny interpolation going on, it would help
readers to signal that fact by quoting the end-of-here-text marker.
And it makes the entire test block reads nicer if you indent the
body of hte here-text by prefixing the end-of-here-text marker with
a dash, i.e.

	write_script .git/custom-hooks/pre-commit <<-\EOF &&
	printf "%s" "." >>.git/PRE-COMMIT-HOOK-WAS-CALLED
	EOF

> +	cat >.git/hooks/pre-commit <<EOF &&
> +	write_script .git/hooks/pre-commit &&
> +printf "%s" "SHOULD NOT BE CALLED" >>.git/PRE-COMMIT-HOOK-WAS-CALLED
> +EOF
> +	chmod +x .git/custom-hooks/pre-commit

You didn't want cat and chmod here?

> +'
> +
> +test_expect_success 'Check that various forms of specifying core.hooksPath work' '
> +	test_commit no_custom_hook &&
> +	git config core.hooksPath .git/custom-hooks &&
> +	test_commit have_custom_hook &&
> +	git config core.hooksPath .git/custom-hooks/ &&
> +	test_commit have_custom_hook_trailing_slash &&
> +	git config core.hooksPath "$PWD/.git/custom-hooks" &&
> +	test_commit have_custom_hook_abs_path &&
> +	git config core.hooksPath "$PWD/.git/custom-hooks/" &&
> +	test_commit have_custom_hook_abs_path_trailing_slash &&
> +	printf "%s" "...." >.git/PRE-COMMIT-HOOK-WAS-CALLED.expect &&
> +	test_cmp .git/PRE-COMMIT-HOOK-WAS-CALLED.expect .git/PRE-COMMIT-HOOK-WAS-CALLED
> +'
> +
> +test_done