Re: [PATCH v2] fetch: Protect branches checked out in all worktrees

From: Anders Kaseorg <andersk@mit.edu>
Date: Nov 5, 2021, 10:01 UTC
Message-ID: <991240f7-2a3e-8ae7-ae25-a1d9d96d55d4@mit.edu>
In-Reply-To: <YYTtdZblnHYgDgBq@coredump.intra.peff.net>

On 11/5/21 01:38, Jeff King wrote:

> If we get the non-default worktree here, it might be nice to tell the
> user which worktree has it checked out. Otherwise it may lead to
> head-scratching as they peek at their current HEAD.

I’d agree if this case were reachable, but as per below, it seems not to be. So I didn’t bother figuring out how to shoehorn such a message into the arguments expected by format_display().

Show 5 quoted lines

> I also notice that find_shared_symref() will catch cases where we're on
> a detached HEAD, but it's because we're rebasing or bisecting on a given
> branch. Arguably that's a sensible thing to do for this check, but it is
> a change from the current behavior (even if you are not using worktrees
> at all).

That’s a good observation. I agree it seems sensible, but should be noted in the commit message.

> So if the earlier hunk covered this case, then what is this hunk for? :)

Yeah, that duplication confused me too. The relevant call paths seem to be:

1. do_fetch calls check_not_current_branch (later hunk);
2. do_fetch calls fetch_and_consume_refs → store_updated_refs → 
update_local_ref (earlier hunk);
3. do_fetch calls backfill_tags → fetch_and_consume_refs → 
store_updated_refs → update_local_ref (earlier hunk);

in that order. That suggests there should be no way to trip the earlier hunk’s check without first tripping the latter hunk’s check.

Show 5 quoted lines

>    $ git fetch . main:main
>    fatal: Refusing to fetch into branch 'refs/heads/main' checked out at '/home/peff/tmp'
> 
> It's technically correct, but perhaps it would be nicer to keep the old
> message if the worktree we found is the current one.

I note that this new message is more consistent with

$ git branch -d main error: Cannot delete branch 'main' checked out at '/home/anders/tmp'

While looking for other related messages, I noticed another bug: receive-pack doesn’t stop you from updating or deleting a branch checked out in a worktree for a bare repository.

$ git init $ git commit --allow-empty -qm foo $ git clone --bare . bare.git $ git -C bare.git worktree add wt $ git -C bare.git/wt push .. :wt

To ..

  - [deleted]         wt

This is_bare_repository() check in update() in builtin/receive-pack.c seems wrong:

const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);

Anders