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

[Junio C Hamano <gitster@pobox.com>]

Anders Kaseorg <andersk@mit.edu> writes:

> A bare repository won’t have a working tree at .., but it may still have

My reading hiccupped after "at"; perhaps enclose the double-dot
inside a pair of double quotes would make it easier to follow.

> separate working trees created with git worktree. We should protect the
> current branch of such working trees from being updated or deleted,
> according to receive.denyCurrentBranch.

Good point.  I was wondering about that exact thing while reading
the fetch side.

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

> -	else
> -		work_tree = "..";
> -
> -	if (is_bare_repository())
> +	else if (is_bare_repository())
>  		return "denyCurrentBranch = updateInstead needs a worktree";
> +	else
> +		work_tree = "..";
> +
>  	if (worktree)
>  		git_dir = get_worktree_git_dir(worktree);
>  	if (!git_dir)
> @@ -1486,7 +1486,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  	struct object_id *old_oid = &cmd->old_oid;
>  	struct object_id *new_oid = &cmd->new_oid;
>  	int do_update_worktree = 0;
> -	const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);
> +	const struct worktree *worktree = find_shared_symref("HEAD", name);

OK.  This change does make sense.  The worktree we happen to be in
might be bare, but there can be other worktrees that have the branch
in question checked out, so find_shared_symref() must be called
regardless.

The callsite of the other function this patch modifies is in this
update() function much later, and I think it should be updated to
use the variable "worktree" instead of calling find_shared_symref()
again with the same parameters.

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 4ef4ecbe71..52a4686afe 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1763,20 +1763,25 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
>  
>  test_expect_success 'denyCurrentBranch and worktrees' '
>  	git worktree add new-wt &&
> +	git clone --bare . bare.git &&
> +	git -C bare.git worktree add bare-wt &&

We create a bare.git bare repository with a bare-wt worktree that
has a working tree.  bare-wt branch must be protected now.

>  	git clone . cloned &&
>  	test_commit -C cloned first &&
>  	test_config receive.denyCurrentBranch refuse &&
>  	test_must_fail git -C cloned push origin HEAD:new-wt &&
> +	test_config -C bare.git receive.denyCurrentBranch refuse &&
> +	test_must_fail git -C cloned push ../bare.git HEAD:bare-wt &&

And pushing to that branch is refused (which is the default without
the receive.denyCurrentBranch configuration, too).  Good.

>  	test_config receive.denyCurrentBranch updateInstead &&
>  	git -C cloned push origin HEAD:new-wt &&
> -	test_must_fail git -C cloned push --delete origin new-wt
> +	test_must_fail git -C cloned push --delete origin new-wt &&

> +	test_config -C bare.git receive.denyCurrentBranch updateInstead &&
> +	git -C cloned push ../bare.git HEAD:bare-wt &&

And when set to update, it would update the working tree as expected.

We are not checking if we correctly update the working tree; we are
only seeing "git push" succeeds.  Which might want to be tightened
up.

> +	test_must_fail git -C cloned push --delete ../bare.git bare-wt

And even with updateInstead, we do not let the branch to be deleted.

>  '
>  
>  test_expect_success 'refuse fetch to current branch of worktree' '
>  	test_commit -C cloned second &&
>  	test_must_fail git fetch cloned HEAD:new-wt &&
> -	git clone --bare . bare.git &&
> -	git -C bare.git worktree add bare-wt &&

It is a bit sad that these two tests are so inter-dependent.
Depending on an earlier failure of other tests, this may fail in an
unexpected way.

>  	test_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt &&
>  	git fetch -u cloned HEAD:new-wt &&
>  	git -C bare.git fetch -u ../cloned HEAD:bare-wt

I think the core of the patch looks well thought out.  If the tests
are cleaned up a bit more, it would be perfect.

Thanks.