Re: [PATCH] worktree: repair copied repository and linked worktrees

From: Caleb White <cdwhite3@pm.me>
Date: Oct 8, 2024, 15:30 UTC
Message-ID: <20241008153035.71178-1-cdwhite3@pm.me>

Eric Sunshine <ericsunshine@charter.net> writes:

Show 20 quoted lines

> +	inferred_backlink = infer_backlink(realdotgit.buf);
>  	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
>  	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
>  		fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
>  		goto done;
>  	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> -		if (!(backlink = infer_backlink(realdotgit.buf))) {
> +		if (inferred_backlink) {
> +			/*
> +			 * Worktree's .git file does not point at a repository
> +			 * but we found a .git/worktrees/<id> in this
> +			 * repository with the same <id> as recorded in the
> +			 * worktree's .git file so make the worktree point at
> +			 * the discovered .git/worktrees/<id>. (Note: backlink
> +			 * is already NULL, so no need to free it first.)
> +			 */
> +			backlink = inferred_backlink;
> +			inferred_backlink = NULL;
> +		} else {
>  			fn(1, realdotgit.buf, _("unable to locate reposito

ry; .git file does not reference a repository"), cb_data);

>  			goto done;
>  		}

Moving the `infer_backlink()` call outside of the error condition caused me to discover a bug in my own refactor of infer_backlink() on my relative paths series. Turns out I needed to clear the `inferred` path if an error occurred. This is OK.

Show 12 quoted lines

> +	/*
> +	 * If we got this far, either the worktree's .git file pointed at a
> +	 * valid repository (i.e. read_gitfile_gently() returned success) or
> +	 * the .git file did not point at a repository but we were able to
> +	 * infer a suitable new value for the .git file by locating a
> +	 * .git/worktrees/<id> in *this* repository corresponding to the <id>
> +	 * recorded in the worktree's .git file.
> +	 *
> +	 * However, if, at this point, inferred_backlink is non-NULL (i.e. we
> +	 * found a suitable .git/worktrees/<id> in *this* repository) *and* the
> +	 * worktree's .git file points at a valid repository *and* those two
> +	 * paths

differ, then that indicates that the user probably *copied*

Show 11 quoted lines

> +	 * the main and linked worktrees to a new location as a unit rather
> +	 * than *moving* them. Thus, the copied worktree's .git file actually
> +	 * points at the .git/worktrees/<id> in the *original* repository, not
> +	 * in the "copy" repository. In this case, point the "copy" worktree's
> +	 * .git file at the "copy" repository.
> +	 */
> +	if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {
> +		free(backlink);
> +		backlink = inferred_backlink;
> +		inferred_backlink = NULL;
> +	}

This edge case seems to primarily be an artifact of using absolute paths to link the worktrees. The test case you provided passes on my relative path series even with this condition commented out. However, we do still want to support absolute paths to maintain backwards compatibility so I think we should try to detect this edge case if we can.

Unfortunately, this change now "repairs" (i.e., breaks) worktr ees from other repositories if the worktree_id happens to be identical between the repositories. For example, the following test passes on `master` (with absolute paths) and on my relative path series (relative paths without your changes integrated), but it fails on your patch (and my relative patch series with your changes integrated):

    test_expect_success 'does not repair worktrees from another repo' '
        test_when_finished "rm -rf repo1 repo2" &&
        mkdir -p repo1 &&
        git -C repo1 init main &&
        test_commit -C repo1/main nothing &&
        git -C repo1/main worktree add ../linked &&
        cp repo1/main/.git/worktrees/linked/gitdir repo1/main.expect &&
        cp repo1/linked/.git repo1/linked.expect &&
        mkdir -p repo2 &&
        git -C repo2 init main &&
        test_commit -C repo2/main nothing &&
        git -C repo2/main worktree add ../linked &&
        cp repo2/main/.git/worktrees/linked/gitdir repo2/main.expect &&
        
cp repo2/linked/.git repo2/linked.expect &&
        git -C repo1/main worktree repair ../.. &&
        test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&
        test_cmp repo1/linked.expect repo1/linked/.git &&
        test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&
        test_cmp repo2/linked.expect repo2/linked/.git &&
        git -C repo2/main worktree repair ../../repo1/linked &&
        test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&
        test_cmp repo1/linked.expect repo1/linked/.git &&
        test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&
        test_cmp repo2/linked.expect repo2/linked/.git
    '

Granted, this is likely a rare edge case, but I would not expect trying to repair a valid worktree from another repository to change the pointers. I think a good solution to this would be to introduce a unique hash on the worktree_id when creating a worktree to guarantee uniqu eness across repositories, e.g.,:

    repo1/main/.git/worktrees/linked-6a4b5d/gitdir:
        /path/to/repo1/linked/.git
    repo1/linked/.git:
        gitdir: /path/to/repo1/main/worktrees/linked-6a4b5d

    repo2/main/.git/worktrees/linked-9ab26fe/gitdir:
        /path/to/repo2/linked/.git
    repo2/linked/.git:
        gitdir: /path/to/repo2/main/worktrees/linked-9ab26fe

This would allow us to detect this copied case while not "repairing" valid worktrees from other repositories with the same linked worktree directory name.

Best,

- Caleb