[v2,4/4] receive.denyCurrentBranch: respect all worktrees

Commit Message

Linus Arver via GitGitGadget Feb. 22, 2020, 10:35 p.m. UTC

From: Hariom Verma <[e-mail]>

The receive.denyCurrentBranch config option controls what happens if
you push to a branch that is checked out into a non-bare repository.
By default, it rejects it. It can be disabled via `ignore` or `warn`.
Another yet trickier option is `updateInstead`.

However, this setting was forgotten when the git worktree command was
introduced: only the main worktree's current branch is respected.

With this change, all worktrees are respected.

That change also leads to revealing another bug,
i.e. `receive.denyCurrentBranch = true` was ignored when pushing into a
non-bare repository's unborn current branch using ref namespaces. As
`is_ref_checked_out()` returns 0 which means `receive-pack` does not get
into conditional statement to switch `deny_current_branch` accordingly
(ignore, warn, refuse, unconfigured, updateInstead).

receive.denyCurrentBranch uses the function `refs_resolve_ref_unsafe()`
(called via `resolve_refdup()`) to resolve the symbolic ref HEAD, but
that function fails when HEAD does not point at a valid commit.
As we replace the call to `refs_resolve_ref_unsafe()` with
`find_shared_symref()`, which has no problem finding the worktree for a
given branch even if it is unborn yet, this bug is fixed at the same
time: receive.denyCurrentBranch now also handles worktrees with unborn
branches as intended even while using ref namespaces.

Helped-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Hariom Verma <[e-mail]>
---
 builtin/receive-pack.c           | 37 +++++++++++++++++---------------
 t/t5509-fetch-push-namespaces.sh |  2 +-
 t/t5516-fetch-push.sh            | 11 ++++++++++
 3 files changed, 32 insertions(+), 18 deletions(-)

Comments

Junio C Hamano Feb. 23, 2020, 6:18 a.m. UTC | #1

"Hariom Verma via GitGitGadget" <[e-mail]> writes:

> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
> index c89483fdba2..6270fb7b576 100755
> --- a/t/t5509-fetch-push-namespaces.sh
> +++ b/t/t5509-fetch-push-namespaces.sh
> @@ -152,7 +152,7 @@ test_expect_success 'clone chooses correct HEAD (v2)' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '
> +test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
>  	cd original &&
>  	git init unborn &&
>  	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index c81ca360ac4..49982b0fd90 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
>  	)
>  '
>  
> +test_expect_success 'denyCurrentBranch and worktrees' '
> +	git worktree add new-wt &&
> +	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 receive.denyCurrentBranch updateInstead &&
> +	git -C cloned push origin HEAD:new-wt &&
> +	test_must_fail git -C cloned push --delete origin new-wt
> +'
> +
>  test_done

This adds one new test and also flips a test that was added in a
separate step that expected a failure to expect success, which looks
a bit strange.

For a series this small, having a test that demonstrates that the
updated code works as expected together with the fix to the code in
a single patch is easier to manage.  After applying a single
test+fix patch, you can easily apply the same patch except for the
test part in reverse on top, if you need to see in what way the code
without the change breaks by running the test.

On a truly large fix, sometimes it may make sense to add a failing
test and nothing else and then a separate step that changes the code
and flips the expectation of the test from failure->success, but I
think a change this size is easier to handle without such an artificial
split.

Thanks.

Patch

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 411e0b4d999..b5ca3123b78 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -27,6 +27,7 @@ 
 #include "object-store.h"
 #include "protocol.h"
 #include "commit-reach.h"
+#include "worktree.h"
 
 static const char * const receive_pack_usage[] = {
 	N_("git receive-pack <git-dir>"),
@@ -816,16 +817,6 @@  static int run_update_hook(struct command *cmd)
 	return finish_command(&proc);
 }
 
-static int is_ref_checked_out(const char *ref)
-{
-	if (is_bare_repository())
-		return 0;
-
-	if (!head_name)
-		return 0;
-	return !strcmp(head_name, ref);
-}
-
 static char *refuse_unconfigured_deny_msg =
 	N_("By default, updating the current branch in a non-bare repository\n"
 	   "is denied, because it will make the index and work tree inconsistent\n"
@@ -997,16 +988,27 @@  static const char *push_to_checkout(unsigned char *hash,
 		return NULL;
 }
 
-static const char *update_worktree(unsigned char *sha1)
+static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
 {
-	const char *retval;
-	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	const char *retval, *work_tree, *git_dir = NULL;
 	struct argv_array env = ARGV_ARRAY_INIT;
 
+	if (worktree && worktree->path)
+		work_tree = worktree->path;
+	else if (git_work_tree_cfg)
+		work_tree = git_work_tree_cfg;
+	else
+		work_tree = "..";
+
 	if (is_bare_repository())
 		return "denyCurrentBranch = updateInstead needs a worktree";
+	
+	if (worktree)
+		git_dir = get_worktree_git_dir(worktree);
+	if (!git_dir)
+		git_dir = get_git_dir();
 
-	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
 	if (!find_hook(push_to_checkout_hook))
 		retval = push_to_deploy(sha1, &env, work_tree);
@@ -1026,6 +1028,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);
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1037,7 +1040,7 @@  static const char *update(struct command *cmd, struct shallow_info *si)
 	free(namespaced_name);
 	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
 
-	if (is_ref_checked_out(namespaced_name)) {
+	if (worktree) {
 		switch (deny_current_branch) {
 		case DENY_IGNORE:
 			break;
@@ -1069,7 +1072,7 @@  static const char *update(struct command *cmd, struct shallow_info *si)
 			return "deletion prohibited";
 		}
 
-		if (head_name && !strcmp(namespaced_name, head_name)) {
+		if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {
 			switch (deny_delete_current) {
 			case DENY_IGNORE:
 				break;
@@ -1118,7 +1121,7 @@  static const char *update(struct command *cmd, struct shallow_info *si)
 	}
 
 	if (do_update_worktree) {
-		ret = update_worktree(new_oid->hash);
+		ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
 		if (ret)
 			return ret;
 	}
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index c89483fdba2..6270fb7b576 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -152,7 +152,7 @@  test_expect_success 'clone chooses correct HEAD (v2)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '
+test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
 	cd original &&
 	git init unborn &&
 	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c81ca360ac4..49982b0fd90 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1712,4 +1712,15 @@  test_expect_success 'updateInstead with push-to-checkout hook' '
 	)
 '
 
+test_expect_success 'denyCurrentBranch and worktrees' '
+	git worktree add new-wt &&
+	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 receive.denyCurrentBranch updateInstead &&
+	git -C cloned push origin HEAD:new-wt &&
+	test_must_fail git -C cloned push --delete origin new-wt
+'
+
 test_done