Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 19, 2023, 10:48 UTC
Message-ID: <cedc93ec-aa6f-65bf-65be-0dca3d4d0186@dunelm.org.uk>
In-Reply-To: <f209958f-e824-181b-f9d5-6e4bc8e53646@gmail.com>

Hi Rubén

On 18/01/2023 23:50, Rubén Justo wrote:

Show 5 quoted lines

> On 17-ene-2023 15:27:29, Junio C Hamano wrote:
> 
>> being solved. Rather, it is unclear what problem you are solving.
> 
> Sorry, I didn't explain the motivation well in the message.

I'm afraid I'm still not totally clear from this message exactly what the problem is. Having looked at die_if_checked_out() I think it maybe the second of Junio's options

  - If the current branch the the current worktree is checked out in
    a different worktree, we get the current worktree back.

Here is the function:

void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
	struct worktree **worktrees = get_worktrees();
	const struct worktree *wt;

	wt = find_shared_symref(worktrees, "HEAD", branch);
	if (wt && (!ignore_current_worktree || !wt->is_current)) {
		skip_prefix(branch, "refs/heads/", &branch);
		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
	}

	free_worktrees(worktrees);
}

It takes a flag to ignore the current worktree but uses find_shared_symref() which does not have such a flag to see if the branch is checked out. This means that if a branch is checkout out twice find_shared_symref() may return the current worktree rather than the one we're interested in.

If that is the problem you're trying to solve I think it would be better to keep the signature of find_shared_symref() the same and add a helper function that is called by die_if_checked_out() and find_shared_symref() which can optionally ignore the current worktree. The commit message for such a patch should explain you're fixing a bug rather than trying to change the existing behavior.

Best Wishes

Phillip

Show 34 quoted lines

> First, I'm not sure if "ignore_current_worktree" is correct, maybe needs
> to be "prefer_current_worktree".  Having said that, let me use an example.
> 
> With...
> 
> 	$ git worktree add wt1 -b main
> 	$ git worktree add wt2 -f main
> 
> ... we get confuse results:
> 
> 	$ git -C wt1 rebase main main
> 	Current branch main is up to date.
> 	$ git -C wt2 rebase main main
> 	fatal: 'main' is already checked out...
> 
> The problem I'm trying to solve is that find_shared_symref() returns the
> first matching worktree, and a possible matching "current worktree"
> might not be the first matching one.  That's why die_if_checked_out()
> dies with "git -C wt2".
> 
> find_shared_symref() searches through the list of worktrees that
> get_worktrees() composes: first the main worktree and then, as getdir()
> returns them, those in .git/worktrees/*.  The search is sequential and
> once a match is found, it is returned.  And so die_if_checked_out(),
> when asked to "ignore_current_worktree", is going to consider for
> "is_current" the worktree which may or may not be the "current" one,
> depending on the sequence from get_worktree(), and getdir() ultimately.
> 
> If we want to disallow operations on a worktree with a checked out
> branch also on another worktree, we need the "ignore_current_worktree".
> But, and now I'm more in favor of this, if we prefer to allow the
> operation, we need a "prefer_current_worktree", to induce
> find_shared_symref() to return the "current" one if it matches, or any
> other one that matches.