[v5,3/4] receive-pack: Protect current branch for bare repository worktree

Commit Message

Anders Kaseorg Nov. 9, 2021, 11:09 p.m. UTC

A bare repository won’t have a working tree at "..", but it may still
have 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.

Signed-off-by: Anders Kaseorg <[e-mail]>
---
 builtin/receive-pack.c |  6 +++---
 t/t5516-fetch-push.sh  | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Nov. 10, 2021, 4 a.m. UTC | #1

On Tue, Nov 09 2021, Anders Kaseorg wrote:

> +test_expect_success 'denyCurrentBranch and bare repository worktrees' '
> +	test_when_finished "rm -fr bare.git" &&
> +	git clone --bare . bare.git &&
> +	git -C bare.git worktree add wt &&
> +	test_commit grape &&
> +	test_config -C bare.git receive.denyCurrentBranch refuse &&
> +	test_must_fail git push bare.git HEAD:wt &&
> +	test_config -C bare.git receive.denyCurrentBranch updateInstead &&
> +	git push bare.git HEAD:wt &&
> +	test_path_exists bare.git/wt/grape.t &&
> +	test_must_fail git push --delete bare.git wt
> +'
> +
>  test_expect_success 'refuse fetch to current branch of worktree' '
>  	test_when_finished "git worktree remove --force wt && git branch -D wt" &&
>  	git worktree add wt &&

Nit: Pick either a "git init sub-repo" or "rm -rf when-done.git" pattern
as you're doing here, or test_config. It doesn't make sense to combine
the two.

We don't need to run around in test_when_finished and unset config for
something we're about to "rm -rf" anyway.

I think it's good practice to avoid test_config whenever possible,
i.e. it's made redundant by using a sturdier test pattern of not
needlessly sharing state.

But when that's needed, i.e. you need one persistent repo you're
modifying, is when it should be used.

Patch

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 542431e692..04a2ec6abc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1455,7 +1455,7 @@  static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 	if (!worktree || !worktree->path)
 		BUG("worktree->path must be non-NULL");
 
-	if (is_bare_repository())
+	if (worktree->is_bare)
 		return "denyCurrentBranch = updateInstead needs a worktree";
 	git_dir = get_worktree_git_dir(worktree);
 
@@ -1479,7 +1479,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);
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1491,7 +1491,7 @@  static const char *update(struct command *cmd, struct shallow_info *si)
 	free(namespaced_name);
 	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
 
-	if (worktree) {
+	if (worktree && !worktree->is_bare) {
 		switch (deny_current_branch) {
 		case DENY_IGNORE:
 			break;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f07e32126f..4847793a1c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1769,9 +1769,23 @@  test_expect_success 'denyCurrentBranch and worktrees' '
 	test_must_fail git -C cloned push origin HEAD:new-wt &&
 	test_config receive.denyCurrentBranch updateInstead &&
 	git -C cloned push origin HEAD:new-wt &&
+	test_path_exists new-wt/first.t &&
 	test_must_fail git -C cloned push --delete origin new-wt
 '
 
+test_expect_success 'denyCurrentBranch and bare repository worktrees' '
+	test_when_finished "rm -fr bare.git" &&
+	git clone --bare . bare.git &&
+	git -C bare.git worktree add wt &&
+	test_commit grape &&
+	test_config -C bare.git receive.denyCurrentBranch refuse &&
+	test_must_fail git push bare.git HEAD:wt &&
+	test_config -C bare.git receive.denyCurrentBranch updateInstead &&
+	git push bare.git HEAD:wt &&
+	test_path_exists bare.git/wt/grape.t &&
+	test_must_fail git push --delete bare.git wt
+'
+
 test_expect_success 'refuse fetch to current branch of worktree' '
 	test_when_finished "git worktree remove --force wt && git branch -D wt" &&
 	git worktree add wt &&