Re: [GSoC Patch v4 3/4] repo: add path.commondir with absolute and relative suffix formatting

From: Justin Tobler <jltobler@gmail.com>
Date: Jun 15, 2026, 18:17 UTC
Message-ID: <ajAz5izOU1FSVrKW@denethor>
In-Reply-To: <20260615045112.50686-4-jayatheerthkulkarni2005@gmail.com>

On 26/06/15 10:21AM, K Jayatheerth wrote:

Show 14 quoted lines

> Scripts working with worktree setups need a reliable way to discover
> the common directory, which diverges from the git directory when
> multiple worktrees are in use. There is no way to retrieve this path
> from git repo info today.
> 
> Introduce path.commondir.absolute and path.commondir.relative keys.
> Exposing explicit format variants rather than a single key with a
> default avoids ambiguity for scripts that require predictable output.
> 
> Add a test helper test_repo_info_path that creates isolated
> repositories per test case to prevent state leaks, captures the repo
> root before changing directories to avoid eval, and accepts an optional
> init_command to cover environment variable overrides such as
> GIT_COMMON_DIR and GIT_DIR.

I'm not sure this last paragraph in the log message provides much value. To me it's a bit verbose and focuses mostly on what the test helper is doing. Maybe we can just omit this section? If we want to have a note though maybe we could say something like:

  Each path key is expected to have an absolute and relative form. To
  reduce duplication, a test_repo_info_path helper function is
  introduced to configure and exercise both cases.

> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---

[snip]

Show 22 quoted lines

> diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
> index 39bb77dda0..0c0228687f 100755
> --- a/t/t1900-repo-info.sh
> +++ b/t/t1900-repo-info.sh
> @@ -155,4 +155,65 @@ test_expect_success 'git repo info -h shows only repo info usage' '
>  	test_grep ! "git repo structure" actual
>  '
>  
> +# Helper function to test path keys in both absolute and relative formats.
> +# $1: label for the test
> +# $2: field_name (e.g., commondir)
> +# $3: unique repo name for isolation
> +# $4: expect_absolute (suffix appended to repo root)
> +# $5: expect_relative (the relative path string expected)
> +# $6: init_command (extra setup like exporting env vars)
> +test_repo_info_path () {
> +	label=$1
> +	field_name=$2
> +	repo_name=$3
> +	expect_absolute_suffix=$4
> +	expect_relative=$5
> +	init_command=$6

I may be overthinking it, but I can't help but feel this test helper is overly complicated. I wonder if we can simlify and reduce the number of arguments. For example, could we programatically construct the label from the field name and init_command instead of explicitly passing it?

Show 8 quoted lines

> +	absolute_root="$repo_name-absolute"
> +	relative_root="$repo_name-relative"
> +
> +	test_expect_success "setup: $label" '
> +		git init "$absolute_root" &&
> +		git init "$relative_root" &&
> +		mkdir -p "$absolute_root/sub" "$relative_root/sub"
> +	'

Do really need this setup test case? Could we instead embed the setup in both test cases below? Something like:

	test_when_finished rm -rf repo &&
	git init repo &&
	(
	  mkdir repo/sub &&
	  cd repo/sub &&
	  ...
	)

With something like this, each test case is responsible to creating its own repo and cleaning it up when finished. Then we could avoid have to provide a separate repo name for each set of test cases and remove the repo_name argument.

Show 35 quoted lines

> +	test_expect_success "absolute: $label" '
> +		(
> +			cd "$absolute_root/sub" &&
> +			ROOT="$(test-tool path-utils real_path ..)" && export ROOT &&
> +			eval "$init_command" &&
> +			expect_path="$ROOT${expect_absolute_suffix:+/$expect_absolute_suffix}" &&
> +			echo "path.$field_name.absolute=$expect_path" >expect &&
> +			git repo info "path.$field_name.absolute" >actual &&
> +			test_cmp expect actual
> +		)
> +	'
> +
> +	test_expect_success "relative: $label" '
> +		(
> +			cd "$relative_root/sub" &&
> +			ROOT="$(test-tool path-utils real_path ..)" && export ROOT &&
> +			eval "$init_command" &&
> +			echo "path.$field_name.relative=$expect_relative" >expect &&
> +			git repo info "path.$field_name.relative" >actual &&
> +			test_cmp expect actual
> +		)
> +	'
> +}
> +
> +test_repo_info_path 'commondir standard' 'commondir' 'commondir-std' \
> +	'.git' '../.git'
> +
> +test_repo_info_path 'commondir with GIT_COMMON_DIR and GIT_DIR' 'commondir' \
> +	'commondir-envs' 'custom-common' '../custom-common' \
> +	'GIT_COMMON_DIR="$ROOT/custom-common" && export GIT_COMMON_DIR &&
> +	 GIT_DIR="../.git" && export GIT_DIR &&
> +	 git init --bare "$ROOT/custom-common"'
> +
> +test_repo_info_path 'commondir with only GIT_DIR' 'commondir' \
> +	'commondir-only-gitdir' '.git' '../.git' \

For each of these test cases, the `expect_absolute_suffix` and `expect_relative` and exactly the same. This also appears to be the case for the test cases in the next patch. Do these really need to be configurable at all? Can we just embed them directly in each test case assertion? Or maybe future keys will need this to be configurable?

> +	'GIT_DIR="../.git" && export GIT_DIR'
> +
>  test_done

Overall the rest of the patch looks good to me.

-Justin