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

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

[Anders Kaseorg]

[-- Attachment #1: Type: text/plain, Size: 3119 bytes --]

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 <andersk@mit.edu>
---
 builtin/receive-pack.c | 10 +++++-----
 t/t5516-fetch-push.sh  | 11 ++++++++---
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d960..5efc9bc9fa 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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;
-	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);
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
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 &&
 	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 &&
 	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 &&
+	test_must_fail git -C cloned push --delete ../bare.git bare-wt
 '
 
 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 &&
 	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
-- 
2.33.1

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

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

[Anders Kaseorg]

On 11/8/21 15:28, Junio C Hamano wrote:
> My reading hiccupped after "at"; perhaps enclose the double-dot
> inside a pair of double quotes would make it easier to follow.

Will update.

>> @@ -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.
Perhaps this code is unreachable?

The worktree argument of update_worktree() should never be NULL, because 
we’d only have set do_update_worktree = 1 if find_shared_symref() 
returned non-NULL.  And it looks to me like worktree->path should always 
be initialized to non-NULL, in either get_main_worktree() or 
get_linked_worktree()?  I haven’t read enough of the code to be totally 
confident in this though.

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

Will update.

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

Reasonable.  This is also the case with the existing test.

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

Yeah, I guess I wasn’t sure how much interdependence was allowed or 
expected.  For example, the existing test already fails when run by 
itself (./t5516-fetch-push.sh --run=103) because the repository starts 
out empty.  I’ll see what I can do, perhaps making use of the 
test_when_finished helper.

Anders

[PATCH v4 1/4] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

Refuse to fetch into the currently checked out branch of any working
tree, not just the current one.

Fixes this previously reported bug:

https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de

As a side effect of using find_shared_symref, we’ll also refuse the
fetch when we’re on a detached HEAD because we’re rebasing or bisecting
on the branch in question. This seems like a sensible change.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 builtin/fetch.c       | 28 ++++++++++++++--------------
 t/t5516-fetch-push.sh | 18 ++++++++++++++++++
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f7abbc31ff..0ef2170ef9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -28,6 +28,7 @@
 #include "promisor-remote.h"
 #include "commit-graph.h"
 #include "shallow.h"
+#include "worktree.h"
 
 #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
 
@@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,
 			    int summary_width)
 {
 	struct commit *current = NULL, *updated;
-	struct branch *current_branch = branch_get(NULL);
+	const struct worktree *wt;
 	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
@@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,
 		return 0;
 	}
 
-	if (current_branch &&
-	    !strcmp(ref->name, current_branch->name) &&
-	    !(update_head_ok || is_bare_repository()) &&
+	if (!update_head_ok &&
+	    (wt = find_shared_symref("HEAD", ref->name)) &&
+	    !wt->is_bare &&
 	    !is_null_oid(&ref->old_oid)) {
 		/*
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
 		format_display(display, '!', _("[rejected]"),
-			       _("can't fetch in current branch"),
+			       wt->is_current ?
+			       _("can't fetch in current branch") :
+			       _("checked out in another worktree"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}
@@ -1387,16 +1390,13 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 
 static void check_not_current_branch(struct ref *ref_map)
 {
-	struct branch *current_branch = branch_get(NULL);
-
-	if (is_bare_repository() || !current_branch)
-		return;
-
+	const struct worktree *wt;
 	for (; ref_map; ref_map = ref_map->next)
-		if (ref_map->peer_ref && !strcmp(current_branch->refname,
-					ref_map->peer_ref->name))
-			die(_("Refusing to fetch into current branch %s "
-			    "of non-bare repository"), current_branch->refname);
+		if (ref_map->peer_ref &&
+		    (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)))
+			die(_("Refusing to fetch into branch '%s' "
+			      "checked out at '%s'"),
+			    ref_map->peer_ref->name, wt->path);
 }
 
 static int truncate_fetch_head(void)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8212ca56dc..2c2d6fa6e7 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '
 	git -C cloned push origin HEAD:new-wt &&
 	test_must_fail git -C cloned push --delete origin new-wt
 '
+
+test_expect_success 'refuse fetch to current branch of worktree' '
+	test_when_finished "git worktree remove --force wt" &&
+	git worktree add wt &&
+	test_commit apple &&
+	test_must_fail git fetch . HEAD:wt &&
+	git fetch -u . HEAD:wt
+'
+
+test_expect_success 'refuse fetch to current branch of bare repository worktree' '
+	test_when_finished "rm -fr bare.git" &&
+	git clone --bare . bare.git &&
+	git -C bare.git worktree add wt &&
+	test_commit banana &&
+	test_must_fail git -C bare.git fetch .. HEAD:wt &&
+	git -C bare.git fetch -u .. HEAD:wt
+'
+
 test_done
-- 
2.33.1

[PATCH v4 2/4] receive-pack: Clean dead code from update_worktree()

[Anders Kaseorg]

update_worktree() can only be called with a non-NULL worktree parameter,
because that’s the only case where we set do_update_worktree = 1.
worktree->path is always initialized to non-NULL.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 builtin/receive-pack.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d960..cf575280fc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1449,29 +1449,19 @@ static const char *push_to_checkout(unsigned char *hash,
 
 static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
 {
-	const char *retval, *work_tree, *git_dir = NULL;
+	const char *retval, *git_dir;
 	struct strvec env = STRVEC_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();
+	git_dir = get_worktree_git_dir(worktree);
 
 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
 	if (!hook_exists(push_to_checkout_hook))
-		retval = push_to_deploy(sha1, &env, work_tree);
+		retval = push_to_deploy(sha1, &env, worktree->path);
 	else
-		retval = push_to_checkout(sha1, &env, work_tree);
+		retval = push_to_checkout(sha1, &env, worktree->path);
 
 	strvec_clear(&env);
 	return retval;
@@ -1579,7 +1569,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	}
 
 	if (do_update_worktree) {
-		ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
+		ret = update_worktree(new_oid->hash, worktree);
 		if (ret)
 			return ret;
 	}
-- 
2.33.1

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

[Anders Kaseorg]

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 <andersk@mit.edu>
---
 builtin/receive-pack.c |  4 +---
 t/t5516-fetch-push.sh  | 12 ++++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cf575280fc..5a3c6d8423 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1452,8 +1452,6 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 	const char *retval, *git_dir;
 	struct strvec env = STRVEC_INIT;
 
-	if (is_bare_repository())
-		return "denyCurrentBranch = updateInstead needs a worktree";
 	git_dir = get_worktree_git_dir(worktree);
 
 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
@@ -1476,7 +1474,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)) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2c2d6fa6e7..06cd34b0db 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1772,6 +1772,18 @@ test_expect_success 'denyCurrentBranch and worktrees' '
 	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_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 worktree add wt &&
-- 
2.33.1

[PATCH v4 4/4] branch: Protect branches checked out in all worktrees

[Anders Kaseorg]

Refuse to force-move a branch over the currently checked out branch of
any working tree, not just the current one.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 branch.c          | 10 ++++++----
 t/t3200-branch.sh |  7 +++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/branch.c b/branch.c
index 07a46430b3..581f0c02c2 100644
--- a/branch.c
+++ b/branch.c
@@ -199,7 +199,7 @@ int validate_branchname(const char *name, struct strbuf *ref)
  */
 int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 {
-	const char *head;
+	const struct worktree *wt;
 
 	if (!validate_branchname(name, ref))
 		return 0;
@@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 		die(_("A branch named '%s' already exists."),
 		    ref->buf + strlen("refs/heads/"));
 
-	head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-		die(_("Cannot force update the current branch."));
+	wt = find_shared_symref("HEAD", ref->buf);
+	if (wt && !wt->is_bare)
+		die(_("Cannot force update the branch '%s'"
+		      "checked out at '%s'."),
+		    ref->buf + strlen("refs/heads/"), wt->path);
 
 	return 1;
 }
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e575ffb4ff..4c868bf971 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -168,6 +168,13 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'
 	test_must_fail git branch -M bar foo
 '
 
+test_expect_success 'git branch -M foo bar should fail when bar is checked out in worktree' '
+	git branch -f bar &&
+	test_when_finished "git worktree remove wt && git branch -D wt" &&
+	git worktree add wt &&
+	test_must_fail git branch -M bar wt
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '
 	git checkout -b baz &&
 	git branch bam &&
-- 
2.33.1

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

[Johannes Schindelin]

Hi,

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

> Anders Kaseorg <andersk@mit.edu> writes:
>
> > 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.
>
> [...]
>
> >  '
> >
> >  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.

Indeed. Maybe we should keep the latter as-is, and add this to the former?

	test_when_finished "rm -rf bare.git bare-wt" &&

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Junio,

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

> Junio C Hamano <gitster@pobox.com> writes:
>
> >> @@ -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

Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]

Hi Anders,

On Mon, 8 Nov 2021, Anders Kaseorg wrote:

> Refuse to fetch into the currently checked out branch of any working
> tree, not just the current one.
>
> Fixes this previously reported bug:
>
> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de
>
> As a side effect of using find_shared_symref, we’ll also refuse the
> fetch when we’re on a detached HEAD because we’re rebasing or bisecting
> on the branch in question. This seems like a sensible change.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
>  builtin/fetch.c       | 28 ++++++++++++++--------------
>  t/t5516-fetch-push.sh | 18 ++++++++++++++++++
>  2 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index f7abbc31ff..0ef2170ef9 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -28,6 +28,7 @@
>  #include "promisor-remote.h"
>  #include "commit-graph.h"
>  #include "shallow.h"
> +#include "worktree.h"
>
>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>
> @@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,
>  			    int summary_width)
>  {
>  	struct commit *current = NULL, *updated;
> -	struct branch *current_branch = branch_get(NULL);
> +	const struct worktree *wt;
>  	const char *pretty_ref = prettify_refname(ref->name);
>  	int fast_forward = 0;
>
> @@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,
>  		return 0;
>  	}
>
> -	if (current_branch &&
> -	    !strcmp(ref->name, current_branch->name) &&
> -	    !(update_head_ok || is_bare_repository()) &&
> +	if (!update_head_ok &&
> +	    (wt = find_shared_symref("HEAD", ref->name)) &&
> +	    !wt->is_bare &&
>  	    !is_null_oid(&ref->old_oid)) {
>  		/*
>  		 * If this is the head, and it's not okay to update
>  		 * the head, and the old value of the head isn't empty...
>  		 */
>  		format_display(display, '!', _("[rejected]"),
> -			       _("can't fetch in current branch"),
> +			       wt->is_current ?
> +			       _("can't fetch in current branch") :
> +			       _("checked out in another worktree"),
>  			       remote, pretty_ref, summary_width);
>  		return 1;
>  	}
> @@ -1387,16 +1390,13 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
>
>  static void check_not_current_branch(struct ref *ref_map)
>  {
> -	struct branch *current_branch = branch_get(NULL);
> -
> -	if (is_bare_repository() || !current_branch)
> -		return;
> -
> +	const struct worktree *wt;
>  	for (; ref_map; ref_map = ref_map->next)
> -		if (ref_map->peer_ref && !strcmp(current_branch->refname,
> -					ref_map->peer_ref->name))
> -			die(_("Refusing to fetch into current branch %s "
> -			    "of non-bare repository"), current_branch->refname);
> +		if (ref_map->peer_ref &&
> +		    (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)))

Do we need `&& !wt->is_bare` here, too?

> +			die(_("Refusing to fetch into branch '%s' "
> +			      "checked out at '%s'"),
> +			    ref_map->peer_ref->name, wt->path);
>  }
>
>  static int truncate_fetch_head(void)
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 8212ca56dc..2c2d6fa6e7 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '
>  	git -C cloned push origin HEAD:new-wt &&
>  	test_must_fail git -C cloned push --delete origin new-wt
>  '
> +
> +test_expect_success 'refuse fetch to current branch of worktree' '
> +	test_when_finished "git worktree remove --force wt" &&

Do we also need `&& git branch -D wt` here?

> +	git worktree add wt &&
> +	test_commit apple &&
> +	test_must_fail git fetch . HEAD:wt &&
> +	git fetch -u . HEAD:wt

Maybe even `test_path_exists wt/apple.t`, to verify that the worktree has
been updated?

These would also apply to the next test case.

> +'
> +
> +test_expect_success 'refuse fetch to current branch of bare repository worktree' '
> +	test_when_finished "rm -fr bare.git" &&
> +	git clone --bare . bare.git &&
> +	git -C bare.git worktree add wt &&
> +	test_commit banana &&
> +	test_must_fail git -C bare.git fetch .. HEAD:wt &&
> +	git -C bare.git fetch -u .. HEAD:wt
> +'
> +
>  test_done

Thanks for working on this!
Dscho

Re: [PATCH v4 2/4] receive-pack: Clean dead code from update_worktree()

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2754 bytes --]

Hi Anders,

looks good, just one suggestion, see inlined.

On Mon, 8 Nov 2021, Anders Kaseorg wrote:

> update_worktree() can only be called with a non-NULL worktree parameter,
> because that’s the only case where we set do_update_worktree = 1.
> worktree->path is always initialized to non-NULL.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
>  builtin/receive-pack.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 49b846d960..cf575280fc 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1449,29 +1449,19 @@ static const char *push_to_checkout(unsigned char *hash,
>
>  static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
>  {
> -	const char *retval, *work_tree, *git_dir = NULL;
> +	const char *retval, *git_dir;
>  	struct strvec env = STRVEC_INIT;
>
> -	if (worktree && worktree->path)
> -		work_tree = worktree->path;
> -	else if (git_work_tree_cfg)
> -		work_tree = git_work_tree_cfg;
> -	else
> -		work_tree = "..";

We might want to make sure that `worktree` and `worktree->path` are
non-`NULL`, and otherwise call a `BUG()`.

> -
>  	if (is_bare_repository())

Okay, I lied, I have two suggestions. Shouldn't this be turned into
`worktree->is_bare`?

Of course, `find_shared_symref()` will currently not return any worktree
with non-zero `is_bare`...

>  		return "denyCurrentBranch = updateInstead needs a worktree";
> -	if (worktree)
> -		git_dir = get_worktree_git_dir(worktree);
> -	if (!git_dir)
> -		git_dir = get_git_dir();
> +	git_dir = get_worktree_git_dir(worktree);
>
>  	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
>
>  	if (!hook_exists(push_to_checkout_hook))
> -		retval = push_to_deploy(sha1, &env, work_tree);
> +		retval = push_to_deploy(sha1, &env, worktree->path);
>  	else
> -		retval = push_to_checkout(sha1, &env, work_tree);
> +		retval = push_to_checkout(sha1, &env, worktree->path);
>
>  	strvec_clear(&env);
>  	return retval;
> @@ -1579,7 +1569,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  	}
>
>  	if (do_update_worktree) {
> -		ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
> +		ret = update_worktree(new_oid->hash, worktree);

Makes sense. If `worktree` is `NULL`, `do_update_worktree` won't ever be
set, and if `worktree` is not `NULL`, even before we remove that
`is_bare_repository()` ternary, it is set to `find_shared_symref("HEAD",
name)` (and `name` is not changed in the `update()` function).

Thanks,
Dscho

>  		if (ret)
>  			return ret;
>  	}
> --
> 2.33.1
>
>

Re: [PATCH v4 3/4] receive-pack: Protect current branch for bare repository worktree

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

Hi Anders,

On Mon, 8 Nov 2021, Anders Kaseorg wrote:

> 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 <andersk@mit.edu>
> ---
>  builtin/receive-pack.c |  4 +---
>  t/t5516-fetch-push.sh  | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index cf575280fc..5a3c6d8423 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1452,8 +1452,6 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
>  	const char *retval, *git_dir;
>  	struct strvec env = STRVEC_INIT;
>
> -	if (is_bare_repository())
> -		return "denyCurrentBranch = updateInstead needs a worktree";
>  	git_dir = get_worktree_git_dir(worktree);
>
>  	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
> @@ -1476,7 +1474,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);

While `find_shared_symref()` currently won't return a `worktree` with a
non-zero `is_bare`, to future-proof the code we might want to turn the
`if (worktree)` below (8 lines outside the current diff context) into `if
(worktree && !worktree->is_bare)`.

>
>  	/* only refs/... are allowed */
>  	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 2c2d6fa6e7..06cd34b0db 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1772,6 +1772,18 @@ test_expect_success 'denyCurrentBranch and worktrees' '
>  	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" &&

While `wt/` will be created inside `bare.git` and therefore be removed,
the branch `wt` won't. Maybe add `&& git branch -D wt`?

> +	git clone --bare . bare.git &&
> +	git -C bare.git worktree add wt &&
> +	test_commit grape &&

I like fruit, too! Apple, banana, grape, yummy. I wonder what's next :-)

> +	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 &&

Maybe make sure that `bare.git/wt/grape.t` exists? We do want the worktree
to be updated, after all...

Thanks,
Dscho

> +	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 worktree add wt &&
> --
> 2.33.1
>
>

Re: [PATCH v4 4/4] branch: Protect branches checked out in all worktrees

[Johannes Schindelin]

Hi Anders,

On Mon, 8 Nov 2021, Anders Kaseorg wrote:

> Refuse to force-move a branch over the currently checked out branch of
> any working tree, not just the current one.

Good catch!

I read through the four patches, offered a couple of suggestions, but
nothing major. Well done!

Thank you,
Dscho

>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
>  branch.c          | 10 ++++++----
>  t/t3200-branch.sh |  7 +++++++
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 07a46430b3..581f0c02c2 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -199,7 +199,7 @@ int validate_branchname(const char *name, struct strbuf *ref)
>   */
>  int validate_new_branchname(const char *name, struct strbuf *ref, int force)
>  {
> -	const char *head;
> +	const struct worktree *wt;
>
>  	if (!validate_branchname(name, ref))
>  		return 0;
> @@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
>  		die(_("A branch named '%s' already exists."),
>  		    ref->buf + strlen("refs/heads/"));
>
> -	head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
> -	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
> -		die(_("Cannot force update the current branch."));
> +	wt = find_shared_symref("HEAD", ref->buf);
> +	if (wt && !wt->is_bare)
> +		die(_("Cannot force update the branch '%s'"
> +		      "checked out at '%s'."),
> +		    ref->buf + strlen("refs/heads/"), wt->path);
>
>  	return 1;
>  }
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index e575ffb4ff..4c868bf971 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -168,6 +168,13 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'
>  	test_must_fail git branch -M bar foo
>  '
>
> +test_expect_success 'git branch -M foo bar should fail when bar is checked out in worktree' '
> +	git branch -f bar &&
> +	test_when_finished "git worktree remove wt && git branch -D wt" &&
> +	git worktree add wt &&
> +	test_must_fail git branch -M bar wt
> +'
> +
>  test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '
>  	git checkout -b baz &&
>  	git branch bam &&
> --
> 2.33.1
>
>

Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

On 11/9/21 08:09, Johannes Schindelin wrote:
>> +		    (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)))
> 
> Do we need `&& !wt->is_bare` here, too?

Sure (in the “future-proofing” sense), will add.

>> +test_expect_success 'refuse fetch to current branch of worktree' '
>> +	test_when_finished "git worktree remove --force wt" &&
> 
> Do we also need `&& git branch -D wt` here?

Will add.

>> +	git fetch -u . HEAD:wt
> 
> Maybe even `test_path_exists wt/apple.t`, to verify that the worktree has
> been updated?

Not here; git fetch -u never updates working trees, not even the main 
working tree.  Is that a bug?  I don’t know, but if it is, it’s 
certainly a separate one.

Anders

Re: [PATCH v4 2/4] receive-pack: Clean dead code from update_worktree()

[Anders Kaseorg]

On 11/9/21 08:16, Johannes Schindelin wrote:
> We might want to make sure that `worktree` and `worktree->path` are
> non-`NULL`, and otherwise call a `BUG()`.

Okay.

> Okay, I lied, I have two suggestions. Shouldn't this be turned into
> `worktree->is_bare`?

Sure (but in the next commit, since removing is_bare_repository() is a 
bug fix, not pure cleanup).

Anders

Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees

[Junio C Hamano]

Anders Kaseorg <andersk@mit.edu> writes:

>>> +	git fetch -u . HEAD:wt
>> Maybe even `test_path_exists wt/apple.t`, to verify that the
>> worktree has
>> been updated?
>
> Not here; git fetch -u never updates working trees, not even the main
> working tree.

Correct.  The "--update-head-ok" option was invented to let the user
tell Git this: I know updating the ref may make the relationship
between HEAD, the index and the working tree inconsistent, and you
will normally prevent me from doing so to save me trouble. But in
this call, I will reconcile the inconsistencies myself, so do not
worry about the issue and just update the ref.

So there is nothing to fix here.  If the user wanted to update the
working tree, taking the material that was just fetched from the
other side into account, "git pull" would have been used instead.

Thanks.

Re: [PATCH v4 3/4] receive-pack: Protect current branch for bare repository worktree

[Anders Kaseorg]

On 11/9/21 08:22, Johannes Schindelin wrote:
> While `find_shared_symref()` currently won't return a `worktree` with a
> non-zero `is_bare`, to future-proof the code we might want to turn the
> `if (worktree)` below (8 lines outside the current diff context) into `if
> (worktree && !worktree->is_bare)`.

Will do.

>> +test_expect_success 'denyCurrentBranch and bare repository worktrees' '
>> +	test_when_finished "rm -fr bare.git" &&
> 
> While `wt/` will be created inside `bare.git` and therefore be removed,
> the branch `wt` won't. Maybe add `&& git branch -D wt`?

The branch ‘wt’ is inside bare.git.

> I like fruit, too! Apple, banana, grape, yummy. I wonder what's next :-)

Yay!

> Maybe make sure that `bare.git/wt/grape.t` exists? We do want the worktree
> to be updated, after all...

Right, I’d forgotten this one.  Will do.

Thanks all for the helpful reviews!

Anders

[PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

Refuse to fetch into the currently checked out branch of any working
tree, not just the current one.

Fixes this previously reported bug:

https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de

As a side effect of using find_shared_symref, we’ll also refuse the
fetch when we’re on a detached HEAD because we’re rebasing or bisecting
on the branch in question. This seems like a sensible change.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 builtin/fetch.c       | 29 +++++++++++++++--------------
 t/t5516-fetch-push.sh | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f7abbc31ff..ed8a906717 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -28,6 +28,7 @@
 #include "promisor-remote.h"
 #include "commit-graph.h"
 #include "shallow.h"
+#include "worktree.h"
 
 #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
 
@@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,
 			    int summary_width)
 {
 	struct commit *current = NULL, *updated;
-	struct branch *current_branch = branch_get(NULL);
+	const struct worktree *wt;
 	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
@@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,
 		return 0;
 	}
 
-	if (current_branch &&
-	    !strcmp(ref->name, current_branch->name) &&
-	    !(update_head_ok || is_bare_repository()) &&
+	if (!update_head_ok &&
+	    (wt = find_shared_symref("HEAD", ref->name)) &&
+	    !wt->is_bare &&
 	    !is_null_oid(&ref->old_oid)) {
 		/*
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
 		format_display(display, '!', _("[rejected]"),
-			       _("can't fetch in current branch"),
+			       wt->is_current ?
+			       _("can't fetch in current branch") :
+			       _("checked out in another worktree"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}
@@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 
 static void check_not_current_branch(struct ref *ref_map)
 {
-	struct branch *current_branch = branch_get(NULL);
-
-	if (is_bare_repository() || !current_branch)
-		return;
-
+	const struct worktree *wt;
 	for (; ref_map; ref_map = ref_map->next)
-		if (ref_map->peer_ref && !strcmp(current_branch->refname,
-					ref_map->peer_ref->name))
-			die(_("Refusing to fetch into current branch %s "
-			    "of non-bare repository"), current_branch->refname);
+		if (ref_map->peer_ref &&
+		    (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)) &&
+		    !wt->is_bare)
+			die(_("Refusing to fetch into branch '%s' "
+			      "checked out at '%s'"),
+			    ref_map->peer_ref->name, wt->path);
 }
 
 static int truncate_fetch_head(void)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8212ca56dc..f07e32126f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '
 	git -C cloned push origin HEAD:new-wt &&
 	test_must_fail git -C cloned push --delete origin new-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 &&
+	test_commit apple &&
+	test_must_fail git fetch . HEAD:wt &&
+	git fetch -u . HEAD:wt
+'
+
+test_expect_success 'refuse fetch to current branch of bare repository worktree' '
+	test_when_finished "rm -fr bare.git" &&
+	git clone --bare . bare.git &&
+	git -C bare.git worktree add wt &&
+	test_commit banana &&
+	test_must_fail git -C bare.git fetch .. HEAD:wt &&
+	git -C bare.git fetch -u .. HEAD:wt
+'
+
 test_done
-- 
2.33.1

[PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()

[Anders Kaseorg]

update_worktree() can only be called with a non-NULL worktree parameter,
because that’s the only case where we set do_update_worktree = 1.
worktree->path is always initialized to non-NULL.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 builtin/receive-pack.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d960..542431e692 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1449,29 +1449,22 @@ static const char *push_to_checkout(unsigned char *hash,
 
 static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
 {
-	const char *retval, *work_tree, *git_dir = NULL;
+	const char *retval, *git_dir;
 	struct strvec env = STRVEC_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 (!worktree || !worktree->path)
+		BUG("worktree->path must be non-NULL");
 
 	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();
+	git_dir = get_worktree_git_dir(worktree);
 
 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
 	if (!hook_exists(push_to_checkout_hook))
-		retval = push_to_deploy(sha1, &env, work_tree);
+		retval = push_to_deploy(sha1, &env, worktree->path);
 	else
-		retval = push_to_checkout(sha1, &env, work_tree);
+		retval = push_to_checkout(sha1, &env, worktree->path);
 
 	strvec_clear(&env);
 	return retval;
@@ -1579,7 +1572,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	}
 
 	if (do_update_worktree) {
-		ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
+		ret = update_worktree(new_oid->hash, worktree);
 		if (ret)
 			return ret;
 	}
-- 
2.33.1

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

[Anders Kaseorg]

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 <andersk@mit.edu>
---
 builtin/receive-pack.c |  6 +++---
 t/t5516-fetch-push.sh  | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

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 &&
-- 
2.33.1

[PATCH v5 4/4] branch: Protect branches checked out in all worktrees

[Anders Kaseorg]

Refuse to force-move a branch over the currently checked out branch of
any working tree, not just the current one.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 branch.c          | 10 ++++++----
 t/t3200-branch.sh |  7 +++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/branch.c b/branch.c
index 07a46430b3..581f0c02c2 100644
--- a/branch.c
+++ b/branch.c
@@ -199,7 +199,7 @@ int validate_branchname(const char *name, struct strbuf *ref)
  */
 int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 {
-	const char *head;
+	const struct worktree *wt;
 
 	if (!validate_branchname(name, ref))
 		return 0;
@@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 		die(_("A branch named '%s' already exists."),
 		    ref->buf + strlen("refs/heads/"));
 
-	head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-		die(_("Cannot force update the current branch."));
+	wt = find_shared_symref("HEAD", ref->buf);
+	if (wt && !wt->is_bare)
+		die(_("Cannot force update the branch '%s'"
+		      "checked out at '%s'."),
+		    ref->buf + strlen("refs/heads/"), wt->path);
 
 	return 1;
 }
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e575ffb4ff..4c868bf971 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -168,6 +168,13 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'
 	test_must_fail git branch -M bar foo
 '
 
+test_expect_success 'git branch -M foo bar should fail when bar is checked out in worktree' '
+	git branch -f bar &&
+	test_when_finished "git worktree remove wt && git branch -D wt" &&
+	git worktree add wt &&
+	test_must_fail git branch -M bar wt
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '
 	git checkout -b baz &&
 	git branch bam &&
-- 
2.33.1

Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees

[Junio C Hamano]

Another minor thing I noticed and meant to say but kept forgetting.
We do not upcase the first word after the <area>: in the commit
title.  E.g.

Subject: Re: [PATCH v4 1/4] fetch: protect branches checked out in all worktrees

not "fetch: Protect ...".

Thanks.

Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

On 11/9/21 15:28, Junio C Hamano wrote:
> Another minor thing I noticed and meant to say but kept forgetting.
> We do not upcase the first word after the <area>: in the commit
> title.

Ah okay.  Will lowercase if a v6 becomes necessary, otherwise I assume 
you’ll take care of it.

Anders

Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 09 2021, Anders Kaseorg wrote:

> Refuse to fetch into the currently checked out branch of any working
> tree, not just the current one.
>
> Fixes this previously reported bug:
>
> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de
>
> As a side effect of using find_shared_symref, we’ll also refuse the
> fetch when we’re on a detached HEAD because we’re rebasing or bisecting
> on the branch in question. This seems like a sensible change.

Missing tests though, would be nice to have a test that saw what
happened when the branch is in that "git bisect start" or rebasing
state.

Also what those commands to if the branch is updated, e.g. with
git-update-ref.

Re: [PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 09 2021, Anders Kaseorg wrote:

> +	if (!worktree || !worktree->path)
> +		BUG("worktree->path must be non-NULL");

Perhaps a metter of taste, but I think BUG() should really be used for
things that need a custom message over and beyond what assert() gives
us.

In this case using BUG() gives you a worse message, if you do:

    assert(worktree && worktree->path)

You'll get a sensible message from any modern compiler quotign the
variable etc, all of which says the same thing as that BUG() message,
just with less verbosity.

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

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH v5 4/4] branch: Protect branches checked out in all worktrees

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 09 2021, Anders Kaseorg wrote:

> [...]
>  	if (!validate_branchname(name, ref))
>  		return 0;
> @@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
>  		die(_("A branch named '%s' already exists."),
>  		    ref->buf + strlen("refs/heads/"));
>  
> -	head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
> -	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
> -		die(_("Cannot force update the current branch."));
> +	wt = find_shared_symref("HEAD", ref->buf);
> +	if (wt && !wt->is_bare)
> +		die(_("Cannot force update the branch '%s'"

die() etc. messages should start with lower-case. See CodingGuidelines.

Here you're changing an existing die() message, but since it's something
translators will need to re-do let's fix it while we're at it.

Re: [PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

Hi,

On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:

>
> On Tue, Nov 09 2021, Anders Kaseorg wrote:
>
> > +	if (!worktree || !worktree->path)
> > +		BUG("worktree->path must be non-NULL");
>
> Perhaps a metter of taste, but I think BUG() should really be used for
> things that need a custom message over and beyond what assert() gives
> us.
>
> In this case using BUG() gives you a worse message, if you do:
>
>     assert(worktree && worktree->path)
>
> You'll get a sensible message from any modern compiler quotign the
> variable etc, all of which says the same thing as that BUG() message,
> just with less verbosity.

Maybe code reviews should stay away from  contentious matters of taste.

This claim that `assert()` would somehow be preferable to `BUG()` is not
backed up by our very own coding guidelines. See for yourself:
https://github.com/git/git/blob/v2.33.1/Documentation/CodingGuidelines
does not mention it.

The question of `assert()` vs `BUG()` has been brought up on this mailing
list before, without a clear preference for `assert()`, in contrast to
what the comment quoted above would want to make believe.

And the fact that BUG() allows for a well-crafted message without having
to rely on the compiler to guess as to what would make for a helpful
message, that alone speaks volumes.

Ciao,
Johannes

Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4771 bytes --]

Hi Anders,

responding here instead of to the cover letter (because there is none
;-)): great work!

Others pointed out the really tiny nit that some phrases should start with
lower-case, which would be nice to see addressed. I did not find any major
issue anymore (apart from the slightly iffy assumption that `buf->ref`
starts with `refs/heads/` and therefore `buf->ref + strlen("refs/heads/")`
would not overrun, but I _think_ the current code enforces that prefix
somewhere along the lines), so:

	Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thank you for this contribution!
Dscho

On Tue, 9 Nov 2021, Anders Kaseorg wrote:

> Refuse to fetch into the currently checked out branch of any working
> tree, not just the current one.
>
> Fixes this previously reported bug:
>
> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de
>
> As a side effect of using find_shared_symref, we’ll also refuse the
> fetch when we’re on a detached HEAD because we’re rebasing or bisecting
> on the branch in question. This seems like a sensible change.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
>  builtin/fetch.c       | 29 +++++++++++++++--------------
>  t/t5516-fetch-push.sh | 18 ++++++++++++++++++
>  2 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index f7abbc31ff..ed8a906717 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -28,6 +28,7 @@
>  #include "promisor-remote.h"
>  #include "commit-graph.h"
>  #include "shallow.h"
> +#include "worktree.h"
>
>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>
> @@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,
>  			    int summary_width)
>  {
>  	struct commit *current = NULL, *updated;
> -	struct branch *current_branch = branch_get(NULL);
> +	const struct worktree *wt;
>  	const char *pretty_ref = prettify_refname(ref->name);
>  	int fast_forward = 0;
>
> @@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,
>  		return 0;
>  	}
>
> -	if (current_branch &&
> -	    !strcmp(ref->name, current_branch->name) &&
> -	    !(update_head_ok || is_bare_repository()) &&
> +	if (!update_head_ok &&
> +	    (wt = find_shared_symref("HEAD", ref->name)) &&
> +	    !wt->is_bare &&
>  	    !is_null_oid(&ref->old_oid)) {
>  		/*
>  		 * If this is the head, and it's not okay to update
>  		 * the head, and the old value of the head isn't empty...
>  		 */
>  		format_display(display, '!', _("[rejected]"),
> -			       _("can't fetch in current branch"),
> +			       wt->is_current ?
> +			       _("can't fetch in current branch") :
> +			       _("checked out in another worktree"),
>  			       remote, pretty_ref, summary_width);
>  		return 1;
>  	}
> @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
>
>  static void check_not_current_branch(struct ref *ref_map)
>  {
> -	struct branch *current_branch = branch_get(NULL);
> -
> -	if (is_bare_repository() || !current_branch)
> -		return;
> -
> +	const struct worktree *wt;
>  	for (; ref_map; ref_map = ref_map->next)
> -		if (ref_map->peer_ref && !strcmp(current_branch->refname,
> -					ref_map->peer_ref->name))
> -			die(_("Refusing to fetch into current branch %s "
> -			    "of non-bare repository"), current_branch->refname);
> +		if (ref_map->peer_ref &&
> +		    (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)) &&
> +		    !wt->is_bare)
> +			die(_("Refusing to fetch into branch '%s' "
> +			      "checked out at '%s'"),
> +			    ref_map->peer_ref->name, wt->path);
>  }
>
>  static int truncate_fetch_head(void)
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 8212ca56dc..f07e32126f 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '
>  	git -C cloned push origin HEAD:new-wt &&
>  	test_must_fail git -C cloned push --delete origin new-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 &&
> +	test_commit apple &&
> +	test_must_fail git fetch . HEAD:wt &&
> +	git fetch -u . HEAD:wt
> +'
> +
> +test_expect_success 'refuse fetch to current branch of bare repository worktree' '
> +	test_when_finished "rm -fr bare.git" &&
> +	git clone --bare . bare.git &&
> +	git -C bare.git worktree add wt &&
> +	test_commit banana &&
> +	test_must_fail git -C bare.git fetch .. HEAD:wt &&
> +	git -C bare.git fetch -u .. HEAD:wt
> +'
> +
>  test_done
> --
> 2.33.1
>
>

Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

[Junio C Hamano]

Anders Kaseorg <andersk@mit.edu> writes:

> +	if (!update_head_ok &&
> +	    (wt = find_shared_symref("HEAD", ref->name)) &&
> +	    !wt->is_bare &&
>  	    !is_null_oid(&ref->old_oid)) {
>  		/*
>  		 * If this is the head, and it's not okay to update
>  		 * the head, and the old value of the head isn't empty...
>  		 */
>  		format_display(display, '!', _("[rejected]"),
> -			       _("can't fetch in current branch"),
> +			       wt->is_current ?
> +			       _("can't fetch in current branch") :
> +			       _("checked out in another worktree"),
>  			       remote, pretty_ref, summary_width);
>  		return 1;
>  	}
> @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
>  
>  static void check_not_current_branch(struct ref *ref_map)
>  {
> -	struct branch *current_branch = branch_get(NULL);
> -
> -	if (is_bare_repository() || !current_branch)
> -		return;
> -
> +	const struct worktree *wt;
>  	for (; ref_map; ref_map = ref_map->next)
> -		if (ref_map->peer_ref && !strcmp(current_branch->refname,
> -					ref_map->peer_ref->name))
> -			die(_("Refusing to fetch into current branch %s "
> -			    "of non-bare repository"), current_branch->refname);
> +		if (ref_map->peer_ref &&
> +		    (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)) &&
> +		    !wt->is_bare)
> +			die(_("Refusing to fetch into branch '%s' "
> +			      "checked out at '%s'"),
> +			    ref_map->peer_ref->name, wt->path);
>  }

Another thing for the next development cycle.

The find_shared_symref() function is handy (and more correct than
dereferencing HEAD in the current worktree alone, of course), but
its memory ownership model may need to be rethought.

The current semantics is a caller can call find_shared_symref() to
receive at most one worktree, and the caller can use it UNTIL
anybody makes another call to find_shared_symref(), at which point,
the worktree instance becomes unusable and off limit.  The caller
cannot, and should not attempt to, free the worktree instance.

Each time find_shared_symref() is called, we enumerate all the
worktrees and store them in a list that is static to the function.
The returned worktree instance points into that list.  It is not
technically leaked because the static "worktrees" list in the
function holds onto it, and each time the function is called, the
old list of worktrees is discarded and rebuilt anew.  What it means
is that a code like the above, a loop in check_not_current_branch(),
that repeatedly calls find_shared_symref() is both inefficient
(because it takes many "snapshots" of the worktrees attached to the
repository), and also risks an inconsistent view of the world
(because it takes many "snapshots", and each iteration uses
different ones).

I suspect that having a new API function that lets the above be
rewritten along the lines of ...

	struct worktree **all = get_worktrees();
	for ( ; ref_map; ref_map = ref_map->next) {
        	if (!ref_map->peer_ref)
			continue;
                wt = find_wt_with_HEAD(all, ref->map->peer_ref->name);
		if (!wt->is_bare)
			die(_("..."));
	}
	free_worktrees(all);

... would help.

Thanks.

Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

[-- Attachment #1: Type: text/plain, Size: 11089 bytes --]

On Wed, 10 Nov 2021, Junio C Hamano wrote:
> The find_shared_symref() function is handy (and more correct than
> dereferencing HEAD in the current worktree alone, of course), but
> its memory ownership model may need to be rethought.

I wasn’t sure if we wanted to expand the scope of this series, but I do 
agree.  How about moving worktrees from the static variable to a parameter 
of find_shared_symref()?  Would you like me to rebase the series onto this 
patch?

Anders

-- >8 --
Subject: [PATCH] worktree: simplify find_shared_symref() memory ownership model

Storing the worktrees list in a static variable meant that
find_shared_symref() had to rebuild the list on each call (which is
inefficient when the call site is in a loop), and also that each call
invalidated the pointer returned by the previous call (which is
confusing).

Instead, make it the caller’s responsibility to pass in the worktrees
list and manage its lifetime.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 branch.c               | 14 ++++++----
 builtin/branch.c       |  7 ++++-
 builtin/notes.c        |  6 +++-
 builtin/receive-pack.c | 63 +++++++++++++++++++++++++++---------------
 worktree.c             |  8 ++----
 worktree.h             |  5 ++--
 6 files changed, 65 insertions(+), 38 deletions(-)

diff --git a/branch.c b/branch.c
index 07a46430b3..302cc5a04d 100644
--- a/branch.c
+++ b/branch.c
@@ -357,14 +357,16 @@ void remove_branch_state(struct repository *r, int verbose)
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
 {
+	struct worktree **worktrees = get_worktrees();
 	const struct worktree *wt;
 
-	wt = find_shared_symref("HEAD", branch);
-	if (!wt || (ignore_current_worktree && wt->is_current))
-		return;
-	skip_prefix(branch, "refs/heads/", &branch);
-	die(_("'%s' is already checked out at '%s'"),
-	    branch, wt->path);
+	wt = find_shared_symref(worktrees, "HEAD", branch);
+	if (wt && (!ignore_current_worktree || !wt->is_current)) {
+		skip_prefix(branch, "refs/heads/", &branch);
+		die(_("'%s' is already checked out at '%s'"), branch, wt->path);
+	}
+
+	free_worktrees(worktrees);
 }
 
 int replace_each_worktree_head_symref(const char *oldref, const char *newref,
diff --git a/builtin/branch.c b/builtin/branch.c
index 7a1d1eeb07..d8f2164cd7 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -193,6 +193,7 @@ static void delete_branch_config(const char *branchname)
 static int delete_branches(int argc, const char **argv, int force, int kinds,
 			   int quiet)
 {
+	struct worktree **worktrees;
 	struct commit *head_rev = NULL;
 	struct object_id oid;
 	char *name = NULL;
@@ -229,6 +230,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		if (!head_rev)
 			die(_("Couldn't look up commit object for HEAD"));
 	}
+
+	worktrees = get_worktrees();
+
 	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
 		char *target = NULL;
 		int flags = 0;
@@ -239,7 +243,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 
 		if (kinds == FILTER_REFS_BRANCHES) {
 			const struct worktree *wt =
-				find_shared_symref("HEAD", name);
+				find_shared_symref(worktrees, "HEAD", name);
 			if (wt) {
 				error(_("Cannot delete branch '%s' "
 					"checked out at '%s'"),
@@ -300,6 +304,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 
 	free(name);
 	strbuf_release(&bname);
+	free_worktrees(worktrees);
 
 	return ret;
 }
diff --git a/builtin/notes.c b/builtin/notes.c
index 71c59583a1..7f60408dbb 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -861,15 +861,19 @@ static int merge(int argc, const char **argv, const char *prefix)
 		update_ref(msg.buf, default_notes_ref(), &result_oid, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 	else { /* Merge has unresolved conflicts */
+		struct worktree **worktrees;
 		const struct worktree *wt;
 		/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
 		update_ref(msg.buf, "NOTES_MERGE_PARTIAL", &result_oid, NULL,
 			   0, UPDATE_REFS_DIE_ON_ERR);
 		/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
-		wt = find_shared_symref("NOTES_MERGE_REF", default_notes_ref());
+		worktrees = get_worktrees();
+		wt = find_shared_symref(worktrees, "NOTES_MERGE_REF",
+					default_notes_ref());
 		if (wt)
 			die(_("a notes merge into %s is already in-progress at %s"),
 			    default_notes_ref(), wt->path);
+		free_worktrees(worktrees);
 		if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
 			die(_("failed to store link to current notes ref (%s)"),
 			    default_notes_ref());
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d960..017c365298 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1486,12 +1486,17 @@ 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);
+	struct worktree **worktrees = get_worktrees();
+	const struct worktree *worktree =
+		is_bare_repository() ?
+			NULL :
+			find_shared_symref(worktrees, "HEAD", name);
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
 		rp_error("refusing to create funny ref '%s' remotely", name);
-		return "funny refname";
+		ret = "funny refname";
+		goto out;
 	}
 
 	strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
@@ -1510,7 +1515,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			rp_error("refusing to update checked out branch: %s", name);
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				refuse_unconfigured_deny();
-			return "branch is currently checked out";
+			ret = "branch is currently checked out";
+			goto out;
 		case DENY_UPDATE_INSTEAD:
 			/* pass -- let other checks intervene first */
 			do_update_worktree = 1;
@@ -1521,13 +1527,15 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	if (!is_null_oid(new_oid) && !has_object_file(new_oid)) {
 		error("unpack should have generated %s, "
 		      "but I can't find it!", oid_to_hex(new_oid));
-		return "bad pack";
+		ret = "bad pack";
+		goto out;
 	}
 
 	if (!is_null_oid(old_oid) && is_null_oid(new_oid)) {
 		if (deny_deletes && starts_with(name, "refs/heads/")) {
 			rp_error("denying ref deletion for %s", name);
-			return "deletion prohibited";
+			ret = "deletion prohibited";
+			goto out;
 		}
 
 		if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {
@@ -1543,9 +1551,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				if (deny_delete_current == DENY_UNCONFIGURED)
 					refuse_unconfigured_deny_delete_current();
 				rp_error("refusing to delete the current branch: %s", name);
-				return "deletion of the current branch prohibited";
+				ret = "deletion of the current branch prohibited";
+				goto out;
 			default:
-				return "Invalid denyDeleteCurrent setting";
+				ret = "Invalid denyDeleteCurrent setting";
+				goto out;
 			}
 		}
 	}
@@ -1563,25 +1573,30 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		    old_object->type != OBJ_COMMIT ||
 		    new_object->type != OBJ_COMMIT) {
 			error("bad sha1 objects for %s", name);
-			return "bad ref";
+			ret = "bad ref";
+			goto out;
 		}
 		old_commit = (struct commit *)old_object;
 		new_commit = (struct commit *)new_object;
 		if (!in_merge_bases(old_commit, new_commit)) {
 			rp_error("denying non-fast-forward %s"
 				 " (you should pull first)", name);
-			return "non-fast-forward";
+			ret = "non-fast-forward";
+			goto out;
 		}
 	}
 	if (run_update_hook(cmd)) {
 		rp_error("hook declined to update %s", name);
-		return "hook declined";
+		ret = "hook declined";
+		goto out;
 	}
 
 	if (do_update_worktree) {
-		ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
+		ret = update_worktree(new_oid->hash,
+				      find_shared_symref(worktrees, "HEAD",
+							 name));
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	if (is_null_oid(new_oid)) {
@@ -1600,17 +1615,19 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 					   old_oid,
 					   0, "push", &err)) {
 			rp_error("%s", err.buf);
-			strbuf_release(&err);
-			return "failed to delete";
+			ret = "failed to delete";
+		} else {
+			ret = NULL; /* good */
 		}
 		strbuf_release(&err);
-		return NULL; /* good */
 	}
 	else {
 		struct strbuf err = STRBUF_INIT;
 		if (shallow_update && si->shallow_ref[cmd->index] &&
-		    update_shallow_ref(cmd, si))
-			return "shallow error";
+		    update_shallow_ref(cmd, si)) {
+			ret = "shallow error";
+			goto out;
+		}
 
 		if (ref_transaction_update(transaction,
 					   namespaced_name,
@@ -1618,14 +1635,16 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 					   0, "push",
 					   &err)) {
 			rp_error("%s", err.buf);
-			strbuf_release(&err);
-
-			return "failed to update ref";
+			ret = "failed to update ref";
+		} else {
+			ret = NULL; /* good */
 		}
 		strbuf_release(&err);
-
-		return NULL; /* good */
 	}
+
+out:
+	free_worktrees(worktrees);
+	return ret;
 }
 
 static void run_update_post_hook(struct command *commands)
diff --git a/worktree.c b/worktree.c
index 092a4f92ad..cf13d63845 100644
--- a/worktree.c
+++ b/worktree.c
@@ -402,17 +402,13 @@ int is_worktree_being_bisected(const struct worktree *wt,
  * bisect). New commands that do similar things should update this
  * function as well.
  */
-const struct worktree *find_shared_symref(const char *symref,
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+					  const char *symref,
 					  const char *target)
 {
 	const struct worktree *existing = NULL;
-	static struct worktree **worktrees;
 	int i = 0;
 
-	if (worktrees)
-		free_worktrees(worktrees);
-	worktrees = get_worktrees();
-
 	for (i = 0; worktrees[i]; i++) {
 		struct worktree *wt = worktrees[i];
 		const char *symref_target;
diff --git a/worktree.h b/worktree.h
index 8b7c408132..9e06fcbdf3 100644
--- a/worktree.h
+++ b/worktree.h
@@ -143,9 +143,10 @@ void free_worktrees(struct worktree **);
 /*
  * Check if a per-worktree symref points to a ref in the main worktree
  * or any linked worktree, and return the worktree that holds the ref,
- * or NULL otherwise. The result may be destroyed by the next call.
+ * or NULL otherwise.
  */
-const struct worktree *find_shared_symref(const char *symref,
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+					  const char *symref,
 					  const char *target);
 
 /*
-- 
2.33.1

Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> ... (apart from the slightly iffy assumption that `buf->ref`
> starts with `refs/heads/` and therefore `buf->ref + strlen("refs/heads/")`
> would not overrun, but I _think_ the current code enforces that prefix
> somewhere along the lines)

I think that is in 4/4, where the existing code does this:

> diff --git a/branch.c b/branch.c
> index 7a88a4861e..1aaf694b39 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -199,18 +199,20 @@ int validate_branchname(const char *name, struct strbuf *ref)
>   */
>  int validate_new_branchname(const char *name, struct strbuf *ref, int force)
>  {
> -	const char *head;
> +	const struct worktree *wt;
>  
>  	if (!validate_branchname(name, ref))
>  		return 0;

This takes a bare branch name in "name" (or a shorthand like @{-1}),
expand that into a full refname into "ref".  Before passing the ref
into check_refname_format(), "refs/heads/" is unconditionally added
at the beginning.  So we know ref begins with "refs/heads/" after
this point.

>  	if (!force)
>  		die(_("A branch named '%s' already exists."),
>  		    ref->buf + strlen("refs/heads/"));

And we already assume ref->buf has "refs/heads/" as its prefix.  It
may be nice to use skip_prefix(), but it probably is not worth it.

> +	wt = find_shared_symref("HEAD", ref->buf);
> +	if (wt && !wt->is_bare)
> +		die(_("Cannot force update the branch '%s'"
> +		      "checked out at '%s'."),
> +		    ref->buf + strlen("refs/heads/"), wt->path);

And this new use just reuses what we assume to be valid.

So, correctness-wise, I do not think there is much to tweak further
on top of this round.  I've always queued this round more or less
as-is.

In preparation for the next development cycle, however, it might
make sense to add a preparatory clean-up step to downcase the first
word of "die()" messages in the files that are involved in this
series (not necessarily the ones that are touched by the patches,
but all of them) and then apply these four patches (with matching
adjustments, like "Cannot force update" -> "cannot force update") on
top.  In another review message, I also noticed some inefficient
code that is due to insufficient support from the worktree.c API,
but that is not about correctness and can be left out of the series
to get these fixes early in the next cycle.

Thanks.

Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

[Junio C Hamano]

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

> So, correctness-wise, I do not think there is much to tweak further
> on top of this round.  I've always queued this round more or less
> as-is.

Gahh.  Sorry for the typo: "I've already queued", of course.