Re: [PATCH v4 5/8] worktree: add relative cli/config options to `add` command

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 19, 2024, 15:07 UTC
Message-ID: <20fdb5ba-075f-48b5-9e92-f85378500710@gmail.com>
In-Reply-To: <20241031-wt_relative_options-v4-5-07a3dc0f02a3@pm.me>

Hi Caleb

On 01/11/2024 04:38, Caleb White wrote:

Show 10 quoted lines

> This introduces the `--[no-]relative-paths` CLI option and
> `worktree.useRelativePaths` configuration setting to the `worktree add`
> command. When enabled these options allow worktrees to be linked using
> relative paths, enhancing portability across environments where absolute
> paths may differ (e.g., containerized setups, shared network drives).
> Git still creates absolute paths by default, but these options allow
> users to opt-in to relative paths if desired.
> 
> The t2408 test file is removed and more comprehensive tests are
> written for the various worktree operations in their own files.

This looks good, I've left a couple of comments below - there are a few places where the tests could be cleaned up (I've only commented on the first instance of each cleanup) but the coverage looks good and the implementation looks good to me too. I'll leave it here for today, sorry it has taken me so long to get round to looking at these. I should be able to look at the rest of the patches this week.

Show 20 quoted lines

> +worktree.useRelativePaths::
> +	Link worktrees using relative paths (when "true") or absolute
> +	paths (when "false"). This is particularly useful for setups
> +	where the repository and worktrees may be moved between
> +	different locations or environments. Defaults to "false".
> ++
> +Note that setting `worktree.useRelativePaths` to "true" implies enabling the
> +`extension.relativeWorktrees` config (see linkgit:git-config[1]),
> +thus making it incompatible with older versions of Git.
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 70437c815f13852bd2eb862176b8b933e6de0acf..88d2261012166a929b7f167d79720e4d965fd71b 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -216,6 +216,10 @@ To remove a locked worktree, specify `--force` twice.
>   This can also be set up as the default behaviour by using the
>   `worktree.guessRemote` config option.
>   
> +--[no-]relative-paths::
> +	Overrides the `worktree.useRelativePaths` config option, see
> +	linkgit:git-config[1].

I think it would be helpful to describe what this option does directly, rather than in terms of a config setting which is documented elsewhere and that may not be set

Show 12 quoted lines

>   --[no-]track::
>   	When creating a new branch, if `<commit-ish>` is a branch,
>   	mark it as "upstream" from the new branch.  This is the
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index dae63dedf4cac2621f51f95a39aa456b33acd894..e3b4a71ee0bc13d5e817cf7dcc398e9e2bd975de 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -120,12 +120,14 @@ struct add_opts {
>   	int quiet;
>   	int checkout;
>   	int orphan;
> +	int relative_paths;

Excellent - thanks for getting rind of the global in /worktree.c

Show 11 quoted lines

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index cfc4aeb1798c6d023909cec771e5b74e983af5ea..d36d8a4db0e924877787697579544f10f92dc0cf 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -1207,4 +1207,58 @@ test_expect_success '"add" with initialized submodule, with submodule.recurse se
>   	git -C project-clone -c submodule.recurse worktree add ../project-5
>   '
>   
> +test_expect_success 'can create worktrees with relative paths' '
> +	test_when_finished "git worktree remove relative" &&
> +	git config worktree.useRelativePaths false &&

Using test_config instead of "git config" ensures the setting is cleared at the end of the test

> +	git worktree add --relative-paths ./relative &&
> +	cat relative/.git >actual &&

There's no need to copy the file just so we can say "actual" below

Show 19 quoted lines

> +	echo "gitdir: ../.git/worktrees/relative" >expect &&
> +	test_cmp expect actual &&
> +	cat .git/worktrees/relative/gitdir >actual &&
> +	echo "../../../relative/.git" >expect &&
> +	test_cmp expect actual
> +
> +'
> +
> +test_expect_success 'can create worktrees with absolute paths' '
> +	git config worktree.useRelativePaths true &&
> +	git worktree add ./relative &&
> +	cat relative/.git >actual &&
> +	echo "gitdir: ../.git/worktrees/relative" >expect &&
> +	test_cmp expect actual &&
> +	git worktree add --no-relative-paths ./absolute &&
> +	cat absolute/.git >actual &&
> +	echo "gitdir: $(pwd)/.git/worktrees/absolute" >expect &&
> +	test_cmp expect actual
> +'

We're only checking ".git" here and not the "gitdir" file as well, I guess that's ok as the implemntation that writes the files is tested above and here we're interesting in testing the config setting.

Show 13 quoted lines

> +test_expect_success 'move repo without breaking relative internal links' '
> +	test_when_finished rm -rf repo moved &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		git config worktree.useRelativePaths true &&
> +		test_commit initial &&
> +		git worktree add wt1 &&
> +		cd .. &&
> +		mv repo moved &&
> +		cd moved/wt1 &&
> +		git status >out 2>err &&
> +		test_must_be_empty err

This relies on "git status" failing if the worktree links are broken. A more direct test might be to check the output of "git worktree list" or "git rev-parse --git-dir".

Show 11 quoted lines

> +	)
> +'
> +
> +test_expect_success 'relative worktree sets extension config' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	git -C repo commit --allow-empty -m base &&
> +	git -C repo worktree add --relative-paths ./foo &&
> +	git -C repo config get core.repositoryformatversion >actual &&
> +	echo 1 >expected &&
> +	test_cmp expected actual &&

This can be done with

	test_cmp_config -C repo 1 core.repositoryformatversion

> +	git -C repo config get extensions.relativeworktrees >actual &&
> +	echo true >expected &&
> +	test_cmp expected actual
> +'

Best Wishes

Phillip