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

[Justin Tobler <jltobler@gmail.com>]

On 26/06/15 10:21AM, K Jayatheerth wrote:
> 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]
> 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?

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

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