From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 22 Nov 2024 15:55:31 GMT
Subject: Re: [PATCH v4 6/8] worktree: add relative cli/config options to `move` command
Message-ID: <75b4023a-4005-49ff-a06d-e50fcd1c2b50@gmail.com>
In-Reply-To: <20241031-wt_relative_options-v4-6-07a3dc0f02a3@pm.me>

Hi Caleb

This looks good, I've left a couple of comments on the tests - they 
could be cleaned up like the last patch. The implementation looks good.

On 01/11/2024 04:38, Caleb White wrote:
> This teaches the `worktree move` command to respect the
> `--[no-]relative-paths` CLI option and `worktree.useRelativePaths`
> config setting. If an existing worktree is moved with `--relative-paths`
> the new path will be relative (and visa-versa).
> 
> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>   builtin/worktree.c       |  4 +++-
>   t/t2403-worktree-move.sh | 22 ++++++++++++++++++++++
>   worktree.c               | 23 ++++++++++-------------
>   worktree.h               |  3 ++-
>   4 files changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index e3b4a71ee0bc13d5e817cf7dcc398e9e2bd975de..302151506981718658db1cd338cd9064688f5c14 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -1190,6 +1190,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>   		OPT__FORCE(&force,
>   			 N_("force move even if worktree is dirty or locked"),
>   			 PARSE_OPT_NOCOMPLETE),
> +		OPT_BOOL(0, "relative-paths", &use_relative_paths,
> +			 N_("use relative paths for worktrees")),

By the time this function is called we've already initialized 
use_relative_paths from the config setting - good

> [...]
> +test_expect_success 'move worktree with absolute path to relative path' '
> +	git config worktree.useRelativePaths false &&
> +	git worktree add ./absolute &&
> +	git worktree move --relative-paths absolute relative &&
> +	cat relative/.git >actual &&
> +	echo "gitdir: ../.git/worktrees/absolute" >expect &&
> +	test_cmp expect actual &&
> +	git config worktree.useRelativePaths true &&
> +	git worktree move relative relative2 &&
> +	cat relative2/.git >actual &&
> +	echo "gitdir: ../.git/worktrees/absolute" >expect &&
> +	test_cmp expect actual
> +'

As with the last patch we should use test_config and stop copying files 
just to compare them. It's probably worth checking the gitdir file as 
well as .git here as well.

> +test_expect_success 'move worktree with relative path to absolute path' '
> +	git config worktree.useRelativePaths true &&
> +	git worktree move --no-relative-paths relative2 absolute &&
> +	cat absolute/.git >actual &&
> +	echo "gitdir: $(pwd)/.git/worktrees/absolute" >expect &&
> +	test_cmp expect actual
> +'
> +
>   test_done
> [...]
> -	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> +	strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1);

We used to append "/gitdir" below but now we do it all in one - nice.

>   	strbuf_realpath(&path, path_, 1);
> +	strbuf_addf(&dotgit, "%s/.git", path.buf);
>   	if (fspathcmp(wt->path, path.buf)) {

get_linked_worktree guarentees that wt->path is absolute so this looks good.

> -		strbuf_addf(&file, "%s/gitdir", repo.buf);
> -		write_file(file.buf, "%s/.git", relative_path(path.buf, repo.buf, &tmp));
> -		strbuf_reset(&file);
> -		strbuf_addf(&file, "%s/.git", path.buf);
> -		write_file(file.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
> +		write_worktree_linking_files(dotgit, gitdir, use_relative_paths);

This is a nice simplification

Best Wishes

Phillip