Re: [PATCH v4 8/8] worktree: refactor `repair_worktree_after_gitdir_move()`

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Caleb

On 01/11/2024 04:39, Caleb White wrote:
> This refactors `repair_worktree_after_gitdir_move()` to use the new
> `write_worktree_linking_files` function. It also preserves the
> relativity of the linking files; e.g., if an existing worktree used
> absolute paths then the repaired paths will be absolute (and visa-versa).

It would be helpful to give a brief explanation of what this function is 
used for as it seems to be called from setup.c rather than as part of 
"git worktree repair"

> This also adds a test case for reinitializing a repository that has
> relative worktrees.

I'm not sure exactly what this is doing - are we breaking something and 
then calling "git init" to repair it?

My understanding of what this function is trying to do is rather limited 
but as far as I can see the conversion looks OK though the strbuf 
changes are quite distracting.

> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>   t/t0001-init.sh | 22 ++++++++++++++++++----
>   worktree.c      | 29 ++++++++---------------------
>   2 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 0178aa62a41f1606f2382a4a10ab593ccf11e0e8..264951592a711f6879555b785905e799db6c5dd4 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -434,6 +434,12 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' '
>   sep_git_dir_worktree ()  {
>   	test_when_finished "rm -rf mainwt linkwt seprepo" &&
>   	git init mainwt &&
> +	if test "relative" = $2
> +	then
> +		git -C mainwt config worktree.useRelativePaths true

test_config is your friend here (it accepts -C <repo>)

Best Wishes

Phillip

> +	else
> +		git -C mainwt config worktree.useRelativePaths false
> +	fi
>   	test_commit -C mainwt gumby &&
>   	git -C mainwt worktree add --detach ../linkwt &&
>   	git -C "$1" init --separate-git-dir ../seprepo &&
> @@ -442,12 +448,20 @@ sep_git_dir_worktree ()  {
>   	test_cmp expect actual
>   }
>   
> -test_expect_success 're-init to move gitdir with linked worktrees' '
> -	sep_git_dir_worktree mainwt
> +test_expect_success 're-init to move gitdir with linked worktrees (absolute)' '
> +	sep_git_dir_worktree mainwt absolute
> +'
> +
> +test_expect_success 're-init to move gitdir within linked worktree (absolute)' '
> +	sep_git_dir_worktree linkwt absolute
> +'
> +
> +test_expect_success 're-init to move gitdir with linked worktrees (relative)' '
> +	sep_git_dir_worktree mainwt relative
>   '
>   
> -test_expect_success 're-init to move gitdir within linked worktree' '
> -	sep_git_dir_worktree linkwt
> +test_expect_success 're-init to move gitdir within linked worktree (relative)' '
> +	sep_git_dir_worktree linkwt relative
>   '
>   
>   test_expect_success MINGW '.git hidden' '
> diff --git a/worktree.c b/worktree.c
> index 2cb994ac462debf966ac51b5a4f33c30cfebd4ef..2c2e9fd8fdc22d4fe3705227d72b8dbd293038b2 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -653,45 +653,32 @@ void repair_worktrees(worktree_repair_fn fn, void *cb_data, int use_relative_pat
>   
>   void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path)
>   {
> -	struct strbuf path = STRBUF_INIT;
> -	struct strbuf repo = STRBUF_INIT;
>   	struct strbuf gitdir = STRBUF_INIT;
>   	struct strbuf dotgit = STRBUF_INIT;
> -	struct strbuf olddotgit = STRBUF_INIT;
> -	struct strbuf tmp = STRBUF_INIT;
> +	int is_relative_path;
>   
>   	if (is_main_worktree(wt))
>   		goto done;
>   
> -	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> -	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
> +	strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1);
>   
> -	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
> +	if (strbuf_read_file(&dotgit, gitdir.buf, 0) < 0)
>   		goto done;
>   
> -	strbuf_rtrim(&olddotgit);
> -	if (is_absolute_path(olddotgit.buf)) {
> -		strbuf_addbuf(&dotgit, &olddotgit);
> -	} else {
> -		strbuf_addf(&dotgit, "%s/worktrees/%s/%s", old_path, wt->id, olddotgit.buf);
> +	strbuf_rtrim(&dotgit);
> +	is_relative_path = ! is_absolute_path(dotgit.buf);
> +	if (is_relative_path) {
> +		strbuf_insertf(&dotgit, 0, "%s/worktrees/%s/", old_path, wt->id);
>   		strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);
>   	}
>   
>   	if (!file_exists(dotgit.buf))
>   		goto done;
>   
> -	strbuf_addbuf(&path, &dotgit);
> -	strbuf_strip_suffix(&path, "/.git");
> -
> -	write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
> -	write_file(gitdir.buf, "%s", relative_path(dotgit.buf, repo.buf, &tmp));
> +	write_worktree_linking_files(dotgit, gitdir, is_relative_path);
>   done:
> -	strbuf_release(&path);
> -	strbuf_release(&repo);
>   	strbuf_release(&gitdir);
>   	strbuf_release(&dotgit);
> -	strbuf_release(&olddotgit);
> -	strbuf_release(&tmp);
>   }
>   
>   void repair_worktrees_after_gitdir_move(const char *old_path)
>