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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2016, 20:33 UTC
Message-ID: <xmqqlh41junh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1461532702-4045-1-git-send-email-avarab@gmail.com>

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

Show 10 quoted lines

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

Show 13 quoted lines

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

Show 17 quoted lines

> 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

Show 5 quoted lines

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

Show 17 quoted lines

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