Re: [GSoC Patch v7 4/8] repo: add path.hooks with absolute and relative suffixes

[Justin Tobler <jltobler@gmail.com>]

On 26/09/27 05:14PM, K Jayatheerth wrote:
> Hooks are an integral part of a repository's configuration and are
> commonly used by tooling to automate repository-specific workflows.
> Currently, scripts typically retrieve the hooks directory by invoking
> `git rev-parse --git-path hooks`.
> 
> Introduce `path.hooks.absolute` and `path.hooks.relative` keys to
> `git repo info`. This exposes the hooks directory as a scriptable
> config-like key using standard format rules, allowing scripts to
> retrieve it through the same interface as other repository path
> information.
> 
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  Documentation/git-repo.adoc |  9 +++++++++
>  builtin/repo.c              | 22 ++++++++++++++++++++++
>  t/t1900-repo-info.sh        | 28 ++++++++++++++++++++++++++--
>  3 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> index e524a07f53..20836cf8f6 100644
> --- a/Documentation/git-repo.adoc
> +++ b/Documentation/git-repo.adoc
> @@ -119,6 +119,15 @@ values that they return:
>  `path.gitdir.relative`::
>  	The path to the Git repository directory relative to the current working directory.
>  
> +`path.hooks.absolute`::
> +	The canonical absolute path to the repository's hooks directory.
> +	Respects the `core.hooksPath` configuration. If `core.hooksPath` is
> +	set to `/dev/null`, that value is returned unchanged.
> +
> +`path.hooks.relative`::
> +	The path to the repository's hooks directory relative to the current
> +	working directory. Respects the `core.hooksPath` configuration.

So if a repository has `core.hooksPath` set to "/dev/null", the relative
path output here would be something like "../../../../dev/null". I
recognize that this matches the current behavior:

  $ git -c core.hooksPath=/dev/null rev-parse --path-format=relative --git-path hooks
  ../../../../dev/null

...but I'm still not really convinced that is is a behavior we would
want to carry over as it makes it harder to users to figure out when
hooks are disabled.

I would suggest we print the "/dev/null" here even when the relative
path is requested and document the behavior.

[snip]
> diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
> index eec576a1d9..1da5db4942 100755
> --- a/t/t1900-repo-info.sh
> +++ b/t/t1900-repo-info.sh
> @@ -174,7 +174,11 @@ test_repo_info_path () {
>  			cd repo/sub &&
>  			ROOT="$(test-tool path-utils real_path ..)" && export ROOT &&
>  			eval "$init_command" &&
> -			echo "path.$field_name.absolute=$ROOT/$expected_dir" >expect &&
> +			case "$expected_dir" in
> +			/*) EXPECT_ABS="$expected_dir" ;;
> +			*) EXPECT_ABS="$ROOT/$expected_dir" ;;
> +			esac &&
> +			echo "path.$field_name.absolute=$EXPECT_ABS" >expect &&
>  			git repo info "path.$field_name.absolute" >actual &&
>  			test_cmp expect actual
>  		)
> @@ -188,7 +192,11 @@ test_repo_info_path () {
>  			cd repo/sub &&
>  			ROOT="$(test-tool path-utils real_path ..)" && export ROOT &&
>  			eval "$init_command" &&
> -			echo "path.$field_name.relative=../$expected_dir" >expect &&
> +			case "$expected_dir" in
> +			/*) EXPECT_REL="$(test-tool path-utils relative_path "$expected_dir" "$PWD")" ;;
> +			*) EXPECT_REL="../$expected_dir" ;;
> +			esac &&
> +			echo "path.$field_name.relative=$EXPECT_REL" >expect &&
>  			git repo info "path.$field_name.relative" >actual &&
>  			test_cmp expect actual
>  		)

From what I can gather, the code changes to this test helper is really
only required for the "/dev/null" test case. IMO, this helper function
has now become quite unwieldy and rather difficult to parse.

I think we would be better off writing the "/dev/null" test case by hand.

> @@ -213,6 +221,22 @@ test_repo_info_path 'gitdir with explicit GIT_DIR' 'gitdir' \
>  	'.git' \
>  	'GIT_DIR="../.git" && export GIT_DIR'
>  
> +test_repo_info_path 'hooks standard' 'hooks' '.git/hooks'
> +
> +test_repo_info_path 'hooks with core.hooksPath override' 'hooks' \
> +	'custom-hooks' \
> +	'git config core.hooksPath "$ROOT/custom-hooks" && mkdir -p "$ROOT/custom-hooks"'
> +
> +# /dev/null is not a real, canonicalizable filesystem path on Windows,
> +# so path resolution for core.hooksPath=/dev/null cannot be expected to
> +# produce a literal "/dev/null" the way it does on POSIX systems.
> +if ! test_have_prereq MINGW
> +then
> +	test_repo_info_path 'hooks with core.hooksPath=/dev/null' 'hooks' \
> +		'/dev/null' \
> +		'git config core.hooksPath /dev/null'
> +fi

I'm not quite sure I follow here. Are we saying that the git-repo-info
command won't work on windows if the hooks are set to "/dev/null"? Or is
it just some issue with how the test is written? If its the latter,
maybe this is something we can work around if the test is written
without the use of the `test_repo_info_path` helper?

-Justin