From: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
Date: Mon, 08 Jun 2026 22:17:30 GMT
Subject: Re: [GSoC PATCH v2 3/4] repo: add path.gitdir with absolute and relative suffix formatting
Message-ID: <D14CA105-3690-4BB1-932D-7081B1A1DBFA@gmail.com>
In-Reply-To: <20260605163012.181089-4-jayatheerthkulkarni2005@gmail.com>


> +test_repo_info_path () {
> + field_name=$1
> + expect_absolute_eval=$2
> + expect_relative=$3
> + env_prefix=$4

This helper function needs a documentation.

> + test_expect_success "query individual key: path.$field_name.absolute${env_prefix:+ ($env_prefix)}" '

This makes the output polluted. What about changing it by something like:

	test_expect_success "absolute: $label' '...'
	test_expect_success "relative: $label' '...'

with a custom label?

> +
> +test_expect_success 'setup test repository layout for path fields' '
> + git init test-repo &&
> + mkdir -p test-repo/sub
> +'

The helper function `test_repo_info_path` is relying too much on the
existence of the `test-repo`. I think it would be better to add a new
parameter `repo_name` (or similar) because:

1. You could move this creation to the helper function and
   you won't need to place the test after that creation

2. You could use different for each (test_repo_info_path call, path format)
   pair. Currently, if more than one test fails, its result is overwritten
   and the `expect` and `actual` files from the trash directory will be
   the last of the broken tests.

3. You won't need to use the hacky 'echo "$(cd .. && pwd)' 

This applies my suggestions (feel free to use, adapt or discard it):

test_repo_info_path () {
        label=$1
	field_name=$2
	repo_name=$3
	expect_absolute=$4
	expect_relative=$5
	init_command=$6

	absolute_root="$repo_name"-absolute
	relative_root="$repo_name"-relative

	expect_absolute="$PWD"/"$absolute_root"/"$expect_absolute"

	test_expect_success 'setup test repository layout for path fields' '
		git init "$absolute_root" &&
		git init "$relative_root" &&
	       	mkdir -p "$absolute_root"/sub "$relative_root"/sub
	'

	test_expect_success "absolute: $label" '
		(
			export ROOT="$PWD"/"$absolute_root" &&
			cd "$absolute_root"/sub &&
			eval "$init_command" &&
			echo "path.$field_name.absolute=$expect_absolute" >expect &&
			git repo info path.$field_name.absolute >actual &&
			test_cmp expect actual
		)
	'

	test_expect_success "relative: $label" '
		(
			export ROOT="$PWD"/"$relative_root" &&
			cd "$relative_root"/sub &&
			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 'gitdir' 'gitdir' 'gitdir' '.git' '../.git'