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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 22, 2024, 15:55 UTC
Message-ID: <135739ad-6722-449b-9f9b-31c0bbc9f9cb@gmail.com>
In-Reply-To: <20241031-wt_relative_options-v4-7-07a3dc0f02a3@pm.me>

Hi Caleb

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

Show 6 quoted lines

> This teaches the `worktree repair` command to respect the
> `--[no-]relative-paths` CLI option and `worktree.useRelativePaths`
> config setting. If an existing worktree with an absolute path is repaired
> with `--relative-paths`, the links will be replaced with relative paths,
> even if the original path was correct. This allows a user to covert
> existing worktrees between absolute/relative as desired.

This looks pretty good the strbuf changes rather mask the real meat of the patch though.

Show 34 quoted lines

> Signed-off-by: Caleb White <cdwhite3@pm.me>
> diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
> index 7686e60f6ad186519b275f11a5e14064c905b207..84451e903b2ef3c645c0311faf055c846588baf6 100755
> --- a/t/t2406-worktree-repair.sh
> +++ b/t/t2406-worktree-repair.sh
> @@ -216,4 +216,30 @@ test_expect_success 'repair copied main and linked worktrees' '
>   	test_cmp dup/linked.expect dup/linked/.git
>   '
>   
> +test_expect_success 'repair absolute worktree to use relative paths' '
> +	test_when_finished "rm -rf main side sidemoved" &&
> +	test_create_repo main &&
> +	test_commit -C main init &&
> +	git -C main worktree add --detach ../side &&
> +	echo "../../../../sidemoved/.git" >expect-gitdir &&
> +	echo "gitdir: ../main/.git/worktrees/side" >expect-gitfile &&
> +	mv side sidemoved &&
> +	git -C main worktree repair --relative-paths ../sidemoved &&
> +	test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
> +	test_cmp expect-gitfile sidemoved/.git
> +'
> +
> +test_expect_success 'repair relative worktree to use absolute paths' '
> +	test_when_finished "rm -rf main side sidemoved" &&
> +	test_create_repo main &&
> +	test_commit -C main init &&
> +	git -C main worktree add --relative-paths --detach ../side &&
> +	echo "$(pwd)/sidemoved/.git" >expect-gitdir &&
> +	echo "gitdir: $(pwd)/main/.git/worktrees/side" >expect-gitfile &&
> +	mv side sidemoved &&
> +	git -C main worktree repair ../sidemoved &&
> +	test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
> +	test_cmp expect-gitfile sidemoved/.git
> +'

These tests looks sensibile, we should probably check that "git worktree repair" repects worktree.userelativepaths. I wonder if we have any coverage of repair_worktrees() as I think in these tests the problem is fixed by repair_worktree_at_path() before we call repair_worktrees().

Show 12 quoted lines

>   test_done
> diff --git a/worktree.c b/worktree.c
> index 6b640cd9549ecb060236f7eddf1390caa181f1a0..2cb994ac462debf966ac51b5a4f33c30cfebd4ef 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -574,12 +574,14 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
>    * pointing at <repo>/worktrees/<id>.
>    */
>   static void repair_gitfile(struct worktree *wt,
> -			   worktree_repair_fn fn, void *cb_data)
> +			   worktree_repair_fn fn,
> +			   void *cb_data,

Style wise leaving "fn" and "cb_data" on the same line would be fine. That applies to all the functions.

> +			   int use_relative_paths)
>   {

Show 8 quoted lines

> [...]
>   	if (dotgit_contents) {
> @@ -612,18 +615,20 @@ static void repair_gitfile(struct worktree *wt,
>   		repair = _(".git file broken");
>   	else if (fspathcmp(backlink.buf, repo.buf))
>   		repair = _(".git file incorrect");
> +	else if (use_relative_paths == is_absolute_path(dotgit_contents))
> +		repair = _(".git file absolute/relative path mismatch");

Comparing ints as booleans makes me nervous in case we have a non-zero value that isn't 1 but is_absolute_path() returns 0 or 1 and we know use_relative_paths is 0 or 1.

>   	if (repair) {
>   		fn(0, wt->path, repair, cb_data);
> -		write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp));
> +		write_worktree_linking_files(dotgit, gitdir, use_relative_paths);

We used to update only the ".git", now we'll update both. In the case where we're changing to/from absolute/relative paths that's good because we'll update the "gitdir" file as well. In the other cases it looks like we've we've found this worktree via the "gitdir" file so it should be safe to write the same value back to that file.

Show 25 quoted lines

> [...]
>   void repair_worktree_at_path(const char *path,
> -			     worktree_repair_fn fn, void *cb_data)
> +			     worktree_repair_fn fn,
> +			     void *cb_data,
> +			     int use_relative_paths)
>   {
>   	struct strbuf dotgit = STRBUF_INIT;
> -	struct strbuf realdotgit = STRBUF_INIT;
>   	struct strbuf backlink = STRBUF_INIT;
>   	struct strbuf inferred_backlink = STRBUF_INIT;
>   	struct strbuf gitdir = STRBUF_INIT;
>   	struct strbuf olddotgit = STRBUF_INIT;
> -	struct strbuf realolddotgit = STRBUF_INIT;
> -	struct strbuf tmp = STRBUF_INIT;
>
>   	char *dotgit_contents = NULL;
>   	const char *repair = NULL;
>   	int err;
> @@ -779,25 +783,25 @@ void repair_worktree_at_path(const char *path,
>   		goto done;
>   
>   	strbuf_addf(&dotgit, "%s/.git", path);
> -	if (!strbuf_realpath(&realdotgit, dotgit.buf, 0)) {
> +	if (!strbuf_realpath(&dotgit, dotgit.buf, 0)) {

This works because strbuf_realpath() copies dotgit.buf before it resets dotgit but that does not seem to be documented and looking at the output of

     git grep strbuf_realpath | grep \\.buf

I don't see any other callers relying on this outside of your earlier changes to this file. Given that I wonder if we should leave it as is which would also simplify this patch as the interesting changes are swamped by the strbuf tweaking.

Show 5 quoted lines

> [...]   
>   	if (repair) {
>   		fn(0, gitdir.buf, repair, cb_data);
> -		write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp));
> +		write_worktree_linking_files(dotgit, gitdir, use_relative_paths);

We used to just update "gitdir" but we now update ".git" as well. As above that's good when we're repairing a relative/absolute mismatch. In the other cases dotgit always contains the path to the ".git" file in the worktree so that should be fine.

Best Wishes

Phillip