From: Anders Kaseorg <andersk@mit.edu>
Date: Fri, 05 Nov 2021 10:01:39 GMT
Subject: Re: [PATCH v2] fetch: Protect branches checked out in all worktrees
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().

> 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.

>    $ 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