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

Hi Junio,

On Mon, 8 Nov 2021, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

quoted

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

It's such a blast from the past! I first worked on this in 1404bcbb6b3
(receive-pack: add another option for receive.denyCurrentBranch,
2014-11-26), and Hariom & I worked on this last year, before the pandemic
hit over here (and therefore it feels like a decade ago).

The `worktree` variable was introduced in 4ef346482d6
(receive.denyCurrentBranch: respect all worktrees, 2020-02-23), and since
the patch under discussion does away with the `is_bare_repository()` call,
I think that we now can safely change these lines:

        if (do_update_worktree) {
                ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
                if (ret)
                        return ret;
        }

to pass `worktree` directly to the `update_worktree()` function, rather
than calling `find_shared_symref()` again.

And since that is the case, I think your analysis is correct, we always
call `update_worktree()` with a `worktree` parameter that is non-`NULL`.

As to the riddle about why we check `git_work_tree_cfg` at all? Back when
I introduced support for `denyCurrentBranch = updateInstead`, there were
no worktrees, the only way to give a bare repository a worktree was via
that config.

And from how I read the code in `worktree`, both "main" and "linked"
worktrees do have a `path` attribute that is non-`NULL`. We therefore
really have to look at the `is_bare` attribute to know whether
`worktree->path` _actually_ refers to a worktree. But as you also pointed
out, `find_shared_symref()` skips any worktree with non-zero `is_bare`.

We also can be pretty certain that only the `if (worktree &&
worktree->path)` arm is hit, we should probably turn the code into:

	if (!worktree || (!worktree->path && !worktree->is_bare))
                BUG("update_worktree() called without a path");

	if (worktree->is_bare)
		return "denyCurrentBranch = updateInstead needs a worktree";

	work_tree = worktree->path;

Ciao,
Dscho