Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2021, 00:44 UTC
Message-ID: <xmqqpmra40p6.fsf@gitster.g>
In-Reply-To: <xmqqzgqe448a.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 20 quoted lines

>> @@ -1456,11 +1456,11 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
>>  		work_tree = worktree->path;
>>  	else if (git_work_tree_cfg)
>>  		work_tree = git_work_tree_cfg;
>
> Not a fault of this patch at all, but I am not sure if this existing
> bit of code is correct.  Everything else in this function works by
> assuming that the worktree that comes from the caller was checked
> with find_shared_symref("HEAD", name) to ensure that, if not NULL,
> it has the branch checked out and updating to the new commit given
> as the other parameter makes sense.
>
> But this "fall back to configured worktree" is taken when the gave
> us NULL worktree or worktree without the .path member (i.e. no
> checkout), and it must have come from a NULL return from the call to
> find_shared_symref().  IOW, the function said "no worktree
> associated with the repository checks out that branch being
> updated."  I doubt it is a bug to update the working tree of the
> repository with the commit pushed to some branch that is *not* HEAD,
> only because core.worktree was set to point at an explicit location.

Not "I doubt", but I suspect it is a bug.  Sorry.

But in practice, especially with the new code structure, we'd never flip do_update_worktree on unless find_shared_symref() says that the ref we are updating in the function is what is checked out, which means worktree is always non-NULL when we call update_worktree().

So, unless there is some situation where worktree->path is NULL for a worktree with a checkout, the "else if" above is a dead code, I think.

Similarly, I suspect that is_bare_repository() call the patch moved into the if/else if/ chain is even reachable with the updated caller. find_shared_symref() is always called, and unless it gives a non-NULL worktree, do_update_worktree never becomes true.

Anyway, enough bug finding in the existing code. I think the update-instead was Dscho's invention and when the codepath was updated to be worktree ready, Dscho helped Hariom to do so, so I'll CC Dscho to see if he has input.

Thanks.