patch, 4 partsCreate branch_checked_out() helper

This is a replacement for some patches from v2 of my 'git rebase --update-refs' topic [1]. After some feedback from Philip, I've decided to pull that topic while I rework how I track the refs to rewrite [2]. This series moves forward with the branch_checked_out() helper that was a bit more complicated than expected at first glance. This series is a culmination of the discussion started by Junio at [3].

[1] https://lore.kernel.org/git/pull.1247.v2.git.1654634569.gitgitgadget@gmail.com [2] https://lore.kernel.org/git/34264915-8a37-62db-f954-0b5297639b34@github.com/ [3] https://lore.kernel.org/git/xmqqr140t9am.fsf@gitster.g/

Here is the patch outline:

 1. Add a basic form of branch_checked_out() that only looks at the HEAD of
    each worktree. Since this doesn't look at BISECT_HEAD or REBASE_HEAD,
    the helper isn't linked to any consumer in this patch. A test script is
    added that verifies the current behavior checks that are implemented,
    even if they are not connected yet.
 2. Make branch_checked_out() actually look at BISECT_HEAD and REBASE_HEAD.
    Add tests for those cases, which was previously untested in the Git test
    suite. Use branch_checked_out() in 'git branch -f' to demonstrate this
    helper works as expected.
 3. Use branch_checked_out() in 'git fetch' when checking refs that would be
    updated by the refspec. Add tests for that scenario, too.
 4. Use branch_checked_out() in 'git branch -D' to prevent deleting refs
    from under an existing checkout. The commit message here describes some
    barriers to removing other uses of find_shared_symref() that could be
    good investigations for later.

Thanks, -Stolee

Derrick Stolee (4):
  branch: add branch_checked_out() helper
  branch: check for bisects and rebases
  fetch: use new branch_checked_out() and add tests
  branch: use branch_checked_out() when deleting refs

 branch.c                  | 77 +++++++++++++++++++++++++++++++++----
 branch.h                  |  8 ++++
 builtin/branch.c          |  8 ++--
 builtin/fetch.c           | 25 ++++++------
 t/t2407-worktree-heads.sh | 81 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 173 insertions(+), 26 deletions(-)
 create mode 100755 t/t2407-worktree-heads.sh

base-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1254%2Fderrickstolee%2Fbranch-checked-out-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1254/derrickstolee/branch-checked-out-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1254

From: Derrick Stolee <derrickstolee@github.com>

The validate_new_branchname() method contains a check to see if a branch is checked out in any non-bare worktree. This is intended to prevent a force push that will mess up an existing checkout. This helper is not suitable to performing just that check, because the method will die() when the branch is checked out instead of returning an error code.

Create a new branch_checked_out() helper that performs the most basic form of this check. To ensure we can call branch_checked_out() in a loop with good performance, do a single preparation step that iterates over all worktrees and stores their current HEAD branches in a strmap. The branch_checked_out() helper can then discover these branches using a hash lookup.

This helper is currently missing some key functionality. Namely: it doesn't look for active rebases or bisects which mean that the branch is "checked out" even though HEAD doesn't point to that ref. This functionality will be added in a coming change.

We could use branch_checked_out() in validate_new_branchname(), but this missing functionality would be a regression. However, we have no tests that cover this case!

Add a new test script that will be expanded with these cross-worktree ref updates. The current tests would still pass if we refactored validate_new_branchname() to use this version of branch_checked_out(). The next change will fix that functionality and add the proper test coverage.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 branch.c                  | 42 +++++++++++++++++++++++++++++++++++++++
 branch.h                  |  8 ++++++++
 t/t2407-worktree-heads.sh | 24 ++++++++++++++++++++++
 3 files changed, 74 insertions(+)
 create mode 100755 t/t2407-worktree-heads.sh

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

If two worktrees have by accident or by bug checked out the same branch, this strmap_put() will leak, as it overwrites the path to the worktree we found earlier with the branch checked out with the path to the worktree we just discovered with the same branch checked out. We could easily work it around by immediately freeing what comes back from strmap_put(), presumably. The resulting code

		if (wt->head_ref)
			free(strmap_put(...));

may look strange, though, especially because we will see more instances of strmap_put() in this loop in later steps.

On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:

Looking at the end state of this series there is nothing that passes a null "path" to this function, i.e. it's all &some_variable.

So just dropping that && seems better from a code understanding & analysis perspective.

More generally, can't we just expose an API that gives us the strmap itself, so that callers don't need to keep any special-behavior in mind? I.e. just "make me a strmap of checked out branches".

Then you can just use strmap_get(), or xstrdup(strmap_get()), and if the pattern of "get me the item but duped" is found elsewhere maybe we should eventually have a strmap_get_dup() or whatever.

I.e. just making it: xstrdup(strmap_get(checked_out_branches_strmap(), refname)).

On 6/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote:

I don't see the value in making this method more prone to error in
the future, or less flexible to a possible caller that doesn't want
to give a 'path'.
 

This seems unnecessarily complicated. It also risks someone inserting key-value pairs in an improper place instead of being contained to prepare_checked_out_branches().

If your concern is about creating the static current_checked_out_branches, keep in mind that the callers that call branch_checked_out() in a loop would need to keep track of that strmap across several other methods. That additional complexity is much worse than asking for a simple answer through the black box of branch_checked_out().

Thanks, -Stolee

On Tue, Jun 14 2022, Derrick Stolee wrote:

I think creating wrappers for exsiting APIs when they're not really needed is what's more error prone. I.e. you need to learn to use the new thing, whereas if it returns an existing type like "struct strmap", "struct strbuf" etc. there's no learning curve.

It doesn't matter *that much* in this case, but I think it really adds up.

I really don't think the internal APIs in git need to be this paranoid, just document it as "don't touch this".

I think in practice we have (and can expect) pretty mmuch zero bugs due to misuse of APIs that are clearly meant to be global singletons, whereas as this series & the fix-up shows it's more likely that we'll need to deal with subtle memory leaks etc. due to over-wrapping in new APIS :)

Or, if it does need paranoia simply structure the API to have each caller get its own copy, here all the callers are one-shot built-ins, so isn't this a premature optimization?

And speaking of risking things, probably overly fragile as an underlying "general" API, if that's what you're aiming for.

I.e. you're making the hard assumption that the set of checked out branches aren't changing during the lifetime of the process. So e.g. builtin/checkout.c couldn't call this at the end of its run if something in it had needed to get the map earlier (before we switched branches)...

...that being said I was not suggesting to do away with the static pattern, but to keep it, and to just return the map.

As far as hypothetical hot loops are concerned that would be marginally cheaper, as you could move the "get map" outside a loop in such a caller.

I tried this fix-up on top of "seen", which passes your test, and as noted allocates less memory:

Hi Stolee

On 08/06/2022 21:08, Derrick Stolee via GitGitGadget wrote:

I looks like this map is never freed which I think makes sense but makes me wonder about the relevance of patch 5. I think it would probably be worth marking the map with UNLEAK() in prepare_checked_out_branches().

STRMAP_INIT sets .strdup_strings = 1, so the xstrdup() is unnecessary.

I like the idea of having a specific function to see if a branch is 
checkout out rather than Ævar's suggestion of forcing all callers to do
     strmap_get(get_worktree_refs_strmap(), ref)
which will quickly get tiresome. I do wonder though if we'd be better 
off with a thin wrapper around strmap_get() such as

const char* branch_checked_out(const char *refname)
{
	prepare_checked_out_branches();
	return strmap_get(&current_checked_out_branches, refname);
}

so that the user can choose whether to copy the path or not.

Best Wishes

Phillip

On 14/06/2022 11:09, Phillip Wood wrote:

Actually I think UNLEAK() is for marking stack variables, not globals so ignore that. I think patch 5 makes sense in that in keeps the leak bounded.

Sorry that's nonsense - it is wt->head_ref that is copied by strmap_put(), not wt->path, so the xstrdup() call is correct

Thanks for working on this and sorry for the confusion

Phillip

On 6/14/2022 6:09 AM, Phillip Wood wrote:

That .strdup_strings is only for the key, not the value (since the value could be anything, not necessarily a string).

This is an interesting suggestion. It changes callers a bit, but not too much. The pattern currently is:

	if (branch_checked_out(name, &path)) {
		/* do something */
		free(path);
	}

but would become

	if ((path = branch_checked_out(name))) {
		/* do something */
	}

Sounds good to me.

Thanks, -Stolee

From: Derrick Stolee <derrickstolee@github.com>

The branch_checked_out() helper was added by the previous change, but it used an over-simplified view to check if a branch is checked out. It only focused on the HEAD symref, but ignored whether a bisect or rebase was happening.

Teach branch_checked_out() to check for these things, and also add tests to ensure that we do not lose this functionality in the future.

Now that this test coverage exists, we can safely refactor validate_new_branchname() to use branch_checked_out().

Note that we need to prepend "refs/heads/" to the 'state.branch' after calling wt_status_check_*(). We also need to duplicate wt->path so the value is not freed at the end of the call.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 branch.c                  | 35 +++++++++++++++++++++++++++--------
 t/t2407-worktree-heads.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 8 deletions(-)

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Nice to see that each additional feature corresponds to each additional test.

Thanks.

From: Derrick Stolee <derrickstolee@github.com>

When fetching refs from a remote, it is possible that the refspec will cause use to overwrite a ref that is checked out in a worktree. The existing logic in builtin/fetch.c uses a possibly-slow mechanism. Update those sections to use the new, more efficient branch_checked_out() helper.

These uses were not previously tested, so add a test case that can be used for these kinds of collisions. There is only one test now, but more tests will be added as other consumers of branch_checked_out() are added.

Note that there are two uses in builtin/fetch.c, but only one of the messages is tested. This is because the tested check is run before completing the fetch, and the untested check is not reachable without concurrent updates to the filesystem. Thus, it is beneficial to keep that extra check for the sake of defense-in-depth. However, we should not attempt to test the check, as the effort required is too complicated to be worth the effort.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/fetch.c           | 25 +++++++++++--------------
 t/t2407-worktree-heads.sh | 29 +++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 16 deletions(-)

On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:

re a comment on 1/4...

Init'd to NULL here...

..but only used if branch_checkoed_out() returns non-zero, but (and I've only eyeballed this, not run it) isn't it impossible for that function not to return successfully and have "path" be non-NULL?

This seems to be a defense against the underlying strmap_get() starting to return nonsensical results, but if we instead just trust it...

Also re the "can't we just expose the strmap?" comment on 1/4, here we strmap_get() it, and xstrdup() and free() it, but we never needed our own copy, or even cared about the string at all, we're just checking "was it in the set?".

Which is what strmap_contains() would give us, if we just exposed it at that level. So:

	struct strmap *map = make_my_branchs_map_thing();

        [...]
        if ([...] && strmap_contains(map, ref->name)) {
	        ...

Seems more straightforward, no?

Looking at the other callers it seems none of them need the xstrdup() at all, if looking at the end state. This one is a strmp_contains(), then an error() which could use strmap_get() without the xstrdup(), as we format it immediately.

The remaining two are calling die() right afterwards, which ditto can use a plain strmap_get() without xstrdup().

It's not that I care about the extra xstrdup(), which is obviously trivial, it's just harder to read & reason about APIs which e.g. "&& path", and strdup(), only to find that apparently no user in-tree cared about those...

Maybe they'll be needed, but let's just add them then, and in the meantime aim for simpler? :)

Hi Stolee

On 08/06/2022 21:09, Derrick Stolee via GitGitGadget wrote:

I'm confused by this, isn't path always non-null?

Best Wishes

Phillip

On 6/14/2022 6:10 AM, Phillip Wood wrote:

Yes, at this point it is. This message needs to change since we no longer have access to the worktree. As noted in the commit message, this case requires concurrent changes to the filesystem to activate this logic, so having a simpler "branch is checked out _somewhere_" message should work here.

Thanks, -Stolee

From: Derrick Stolee <derrickstolee@github.com>

This is the last current use of find_shared_symref() that can easily be replaced by branch_checked_out(). The benefit of this switch is that the code is a bit simpler, but also it is faster on repeated calls.

The remaining uses of find_shared_symref() are non-trivial to remove, so we probably should not continue in that direction:

* builtin/notes.c uses find_shared_symref() with "NOTES_MERGE_REF"
  instead of "HEAD", so it doesn't have an immediate analogue with
  branch_checked_out(). Perhaps we should consider extending it to
  include that symref in addition to HEAD, BISECT_HEAD, and
  REBASE_HEAD.

* receive-pack.c checks to see if a worktree has a checkout for the ref
  that is being updated. The tricky part is that it can actually decide
  to update the worktree directly instead of just skipping the update.
  This all depends on the receive.denyCurrentBranch config option. The
  implementation currenty cares about receiving the worktree in the
  result, so the current branch_checked_out() prototype is insufficient
  currently. This is something to investigate later, though, since a
  large number of refs could be updated at the same time and using the
  strmap implementation of branch_checked_out() could be beneficial.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/branch.c          | 8 ++++----
 t/t2407-worktree-heads.sh | 5 ++++-
 2 files changed, 8 insertions(+), 5 deletions(-)

On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote:

Junio pointed out that patch 1 introduced a memory leak when a ref is checked out in multiple places. Here is a patch to fix that scenario. It applies cleanly on top of patch 4, so I include it as a new "patch 5". I will include it in any v2 of the full series, if needed.

Thanks, -Stolee

---- >8 ----

From c3842b36ebb4053ac49b0306154b840431f9bf6f Mon Sep 17 00:00:00 2001
From: Derrick Stolee <derrickstolee@github.com>
Date: Mon, 13 Jun 2022 10:33:20 -0400
Subject: [PATCH 5/5] branch: fix branch_checked_out() leaks

The branch_checked_out() method populates a strmap linking a refname to a worktree that has that branch checked out. While unlikely, it is possible that a bug or filesystem manipulation could create a scenario where the same ref is checked out in multiple places. Further, there are some states in an interactive rebase where HEAD and REBASE_HEAD point to the same ref, leading to multiple insertions into the strmap. In either case, the strmap_put() method returns the old value which is leaked.

Update branch_checked_out() to consume that pointer and free it.

Add a test in t2407 that checks this erroneous case. The test "checks itself" by first confirming that the filesystem manipulations it makes trigger the branch_checked_out() logic, and then sets up similar manipulations to make it look like there are multiple worktrees pointing to the same ref.

While TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the leakage and prevent it in the future, t2407 uses helpers such as 'git clone' that cause the test to fail under that mode.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 branch.c                  | 25 +++++++++++++++----------
 t/t2407-worktree-heads.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 10 deletions(-)

Derrick Stolee <derrickstolee@github.com> writes:

While it is equivalent to

			free(strmap_put(&current_checked_out_branches,
                        		wt->head_ref,
					xstrdup(wt->path)));

writing the "put the new one" and "discard the old one (if exists)" as separate steps like the above does make it easier to follow, at least to me.

Thanks.  Will queue.

On Mon, Jun 13 2022, Derrick Stolee wrote:

If you apply this:
	
	diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
	index 0760595337b..d41171acb83 100755
	--- a/t/t2407-worktree-heads.sh
	+++ b/t/t2407-worktree-heads.sh
	@@ -10,16 +10,8 @@ test_expect_success 'setup' '
	 		test_commit $i &&
	 		git branch wt-$i &&
	 		git worktree add wt-$i wt-$i || return 1
	-	done &&
	-
	-	# Create a server that updates each branch by one commit
	-	git clone . server &&
	-	git remote add server ./server &&
	-	for i in 1 2 3 4
	-	do
	-		git -C server checkout wt-$i &&
	-		test_commit -C server A-$i || return 1
	 	done
	+
	 '
	 
	 test_expect_success 'refuse to overwrite: checked out in worktree' '

And compile with SANITIZE=leak then this will pass as:

	./t2407-worktree-heads.sh  --run=1,6

I.e. you only needed the earlier part of the setup, and not "clone.

Given that I think it makes sense to just create a t2408-worktree-heads-leak.sh or something for this new test, then you can use TEST_PASSES_SANITIZE_LEAK.

Normally I'd just say "let's leave it for later", but in this case the entire point of the commit and the relatively lengthy test is to deal with a memory leak, so just copy/pasting the few lines of setup you actually need to a new test & testing with SANITIZE=leak seems worth the effort in this case.

On 6/13/2022 8:33 PM, Ævar Arnfjörð Bjarmason wrote:

Of course this works for the tests that don't need the 'server' repo, but it fails in the tests that _do_ need it.

I'm able to make this work by creating the 'server' with init and creating the wt-$i branches from scratch (they don't need to be fast-forward updates).

The linux-leaks tests still fail due to 'git fetch' and 'git bisect' calls, but these can be avoided by carefully splitting the tests and using the !SANITIZE_LEAK prereq.

Well, the point isn't to use automation to check for leaks, but instead to fix leaks and add tests for the case where we previously had leaks. The tests demonstrate that we aren't accidentally introducing a use-after- free or double-free.

Thanks, -Stolee

On Tue, Jun 14 2022, Derrick Stolee wrote:

I mean copy/paste the existing file to a new one with this on top, then remove all tests that aren't your new one. That worked for me, doesn't it work for you?

I mean that the patch subject is "fix[...]leaks", and this gives it a CI'd regression test, which seems worth it.

Of course it's also nice to check for use-after-fre etc., but we have other coverage for that. E.g. glibc in CI spots double-free issues with how we set it up, and we tend to segfault soon enough (or someone runs SANITIZE=address) on use-after-free.

This is a replacement for some patches from v2 of my 'git rebase --update-refs' topic [1]. After some feedback from Philip, I've decided to pull that topic while I rework how I track the refs to rewrite [2]. This series moves forward with the branch_checked_out() helper that was a bit more complicated than expected at first glance. This series is a culmination of the discussion started by Junio at [3].

[1] https://lore.kernel.org/git/pull.1247.v2.git.1654634569.gitgitgadget@gmail.com [2] https://lore.kernel.org/git/34264915-8a37-62db-f954-0b5297639b34@github.com/ [3] https://lore.kernel.org/git/xmqqr140t9am.fsf@gitster.g/

Here is the patch outline:

 1. Add a basic form of branch_checked_out() that only looks at the HEAD of
    each worktree. Since this doesn't look at BISECT_HEAD or REBASE_HEAD,
    the helper isn't linked to any consumer in this patch. A test script is
    added that verifies the current behavior checks that are implemented,
    even if they are not connected yet.
 2. Make branch_checked_out() actually look at BISECT_HEAD and REBASE_HEAD.
    Add tests for those cases, which was previously untested in the Git test
    suite. Use branch_checked_out() in 'git branch -f' to demonstrate this
    helper works as expected.
 3. Use branch_checked_out() in 'git fetch' when checking refs that would be
    updated by the refspec. Add tests for that scenario, too.
 4. Use branch_checked_out() in 'git branch -D' to prevent deleting refs
    from under an existing checkout. The commit message here describes some
    barriers to removing other uses of find_shared_symref() that could be
    good investigations for later.
 5. Be careful when constructing the strmap to free old values, even though
    this should only happen in error scenarios. Add tests that verify this
    behavior.

Updates in v2 =============

 * branch_checked_out() has a new prototype where it returns a 'const char
   *' instead of copying the path.
 * The test script is marked with TEST_PASSES_SANITIZE_LEAK and test are
   careful to avoid using 'git rebase' or 'git bisect' when possible.
 * Tests that cannot avoid memory-loss from 'git fetch' are marked with the
   "!SANITIZE_LEAK" prereq.
 * A previous replacement of 'wt->current' with 'path' is removed. This
   changes an error message, but it is a very rare scenario where this error
   would be output.

Thanks, -Stolee

Derrick Stolee (5):
  branch: add branch_checked_out() helper
  branch: check for bisects and rebases
  fetch: use new branch_checked_out() and add tests
  branch: use branch_checked_out() when deleting refs
  branch: fix branch_checked_out() leaks

 branch.c                  |  76 +++++++++++++++++++---
 branch.h                  |   7 +++
 builtin/branch.c          |   7 +--
 builtin/fetch.c           |  22 +++----
 t/t2407-worktree-heads.sh | 129 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 215 insertions(+), 26 deletions(-)
 create mode 100755 t/t2407-worktree-heads.sh

base-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1254%2Fderrickstolee%2Fbranch-checked-out-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1254/derrickstolee/branch-checked-out-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1254

Range-diff vs v1:

 1:  dbb7eae390c ! 1:  bf701119edc branch: add branch_checked_out() helper
     @@ branch.c: int validate_branchname(const char *name, struct strbuf *ref)
      +	free_worktrees(worktrees);
      +}
      +
     -+int branch_checked_out(const char *refname, char **path)
     ++const char *branch_checked_out(const char *refname)
      +{
     -+	const char *path_in_set;
      +	prepare_checked_out_branches();
     -+
     -+	path_in_set = strmap_get(&current_checked_out_branches, refname);
     -+	if (path_in_set && path)
     -+		*path = xstrdup(path_in_set);
     -+
     -+	return !!path_in_set;
     ++	return strmap_get(&current_checked_out_branches, refname);
      +}
      +
       /*
     @@ branch.h: void create_branches_recursively(struct repository *r, const char *nam
       				 int dry_run);
      +
      +/*
     -+ * Returns true if the branch at 'refname' is checked out at any
     -+ * non-bare worktree. The path of the worktree is stored in the
     -+ * given 'path', if provided.
     ++ * If the branch at 'refname' is currently checked out in a worktree,
     ++ * then return the path to that worktree.
      + */
     -+int branch_checked_out(const char *refname, char **path);
     ++const char *branch_checked_out(const char *refname);
      +
       /*
        * Check if 'name' can be a valid name for a branch; die otherwise.
     @@ t/t2407-worktree-heads.sh (new)
      +
      +test_description='test operations trying to overwrite refs at worktree HEAD'
      +
     ++TEST_PASSES_SANITIZE_LEAK=true
      +. ./test-lib.sh
      +
      +test_expect_success 'setup' '
     ++	test_commit init &&
     ++	git branch -f fake-1 &&
     ++	git branch -f fake-2 &&
     ++
      +	for i in 1 2 3 4
      +	do
      +		test_commit $i &&
 2:  18bad9b0c49 ! 2:  9347303db89 branch: check for bisects and rebases
     @@ branch.c: static void prepare_checked_out_branches(void)
       	}
       
       	free_worktrees(worktrees);
     -@@ branch.c: int branch_checked_out(const char *refname, char **path)
     +@@ branch.c: const char *branch_checked_out(const char *refname)
        */
       int validate_new_branchname(const char *name, struct strbuf *ref, int force)
       {
      -	struct worktree **worktrees;
      -	const struct worktree *wt;
      -
     -+	char *path;
     ++	const char *path;
       	if (!validate_branchname(name, ref))
       		return 0;
       
     @@ branch.c: int validate_new_branchname(const char *name, struct strbuf *ref, int
      -	worktrees = get_worktrees();
      -	wt = find_shared_symref(worktrees, "HEAD", ref->buf);
      -	if (wt && !wt->is_bare)
     -+	if (branch_checked_out(ref->buf, &path))
     ++	if ((path = branch_checked_out(ref->buf)))
       		die(_("cannot force update the branch '%s' "
       		      "checked out at '%s'"),
      -		    ref->buf + strlen("refs/heads/"), wt->path);
     @@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: checked out
       '
       
      +test_expect_success 'refuse to overwrite: worktree in bisect' '
     -+	test_when_finished test_might_fail git -C wt-4 bisect reset &&
     ++	test_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&
      +
     -+	(
     -+		git -C wt-4 bisect start &&
     -+		git -C wt-4 bisect bad HEAD &&
     -+		git -C wt-4 bisect good HEAD~3
     -+	) &&
     ++	touch .git/worktrees/wt-4/BISECT_LOG &&
     ++	echo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&
      +
     -+	test_must_fail git branch -f wt-4 HEAD 2>err &&
     -+	grep "cannot force update the branch '\''wt-4'\'' checked out at" err
     ++	test_must_fail git branch -f fake-2 HEAD 2>err &&
     ++	grep "cannot force update the branch '\''fake-2'\'' checked out at.*wt-4" err
      +'
      +
     -+. "$TEST_DIRECTORY"/lib-rebase.sh
     -+
      +test_expect_success 'refuse to overwrite: worktree in rebase' '
     -+	test_when_finished test_might_fail git -C wt-4 rebase --abort &&
     ++	test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
      +
     -+	(
     -+		set_fake_editor &&
     -+		FAKE_LINES="edit 1 2 3" \
     -+			git -C wt-4 rebase -i HEAD~3 >rebase &&
     -+		git -C wt-4 status
     -+	) &&
     ++	mkdir -p .git/worktrees/wt-3/rebase-merge &&
     ++	touch .git/worktrees/wt-3/rebase-merge/interactive &&
     ++	echo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-merge/head-name &&
     ++	echo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-merge/onto &&
      +
     -+	test_must_fail git branch -f wt-4 HEAD 2>err &&
     -+	grep "cannot force update the branch '\''wt-4'\'' checked out at" err
     ++	test_must_fail git branch -f fake-1 HEAD 2>err &&
     ++	grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
      +'
      +
       test_done
 3:  4540dbeed38 ! 3:  1c764bfcfe4 fetch: use new branch_checked_out() and add tests
     @@ Commit message
          concurrent updates to the filesystem. Thus, it is beneficial to keep
          that extra check for the sake of defense-in-depth. However, we should
          not attempt to test the check, as the effort required is too
     -    complicated to be worth the effort.
     +    complicated to be worth the effort. This use in update_local_ref()
     +    also requires a change in the error message because we no longer have
     +    access to the worktree struct, only the path of the worktree. This error
     +    is so rare that making a distinction between the two is not critical.
      
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
       {
       	struct commit *current = NULL, *updated;
      -	const struct worktree *wt;
     -+	char *path = NULL;
       	const char *pretty_ref = prettify_refname(ref->name);
       	int fast_forward = 0;
       
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      -	    (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
      -	    !wt->is_bare && !is_null_oid(&ref->old_oid)) {
      +	    !is_null_oid(&ref->old_oid) &&
     -+	    branch_checked_out(ref->name, &path)) {
     ++	    branch_checked_out(ref->name)) {
       		/*
       		 * If this is the head, and it's not okay to update
       		 * the head, and the old value of the head isn't empty...
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      -			       wt->is_current ?
      -				       _("can't fetch in current branch") :
      -				       _("checked out in another worktree"),
     -+			       path ? _("can't fetch in current branch") :
     -+				      _("checked out in another worktree"),
     ++			       _("can't fetch into checked-out branch"),
       			       remote, pretty_ref, summary_width);
     -+		free(path);
       		return 1;
       	}
     - 
      @@ builtin/fetch.c: cleanup:
       	return result;
       }
     @@ builtin/fetch.c: cleanup:
      +static void check_not_current_branch(struct ref *ref_map)
       {
      -	const struct worktree *wt;
     -+	char *path;
     ++	const char *path;
       	for (; ref_map; ref_map = ref_map->next)
       		if (ref_map->peer_ref &&
       		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
      -		    (wt = find_shared_symref(worktrees, "HEAD",
      -					     ref_map->peer_ref->name)) &&
      -		    !wt->is_bare)
     -+		    branch_checked_out(ref_map->peer_ref->name, &path))
     ++		    (path = branch_checked_out(ref_map->peer_ref->name)))
       			die(_("refusing to fetch into branch '%s' "
       			      "checked out at '%s'"),
      -			    ref_map->peer_ref->name, wt->path);
     @@ t/t2407-worktree-heads.sh: test_expect_success 'setup' '
      +	done &&
      +
      +	# Create a server that updates each branch by one commit
     -+	git clone . server &&
     ++	git init server &&
     ++	test_commit -C server initial &&
      +	git remote add server ./server &&
      +	for i in 1 2 3 4
      +	do
     -+		git -C server checkout wt-$i &&
     ++		git -C server checkout -b wt-$i &&
      +		test_commit -C server A-$i || return 1
     ++	done &&
     ++	for i in 1 2
     ++	do
     ++		git -C server checkout -b fake-$i &&
     ++		test_commit -C server f-$i || return 1
       	done
       '
       
     -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: checked out in worktree' '
     - 	done
     +@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in rebase' '
     + 	grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
       '
       
     -+test_expect_success 'refuse to overwrite during fetch' '
     ++test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '
      +	test_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&
      +	grep "refusing to fetch into branch '\''refs/heads/wt-3'\''" err &&
      +
     @@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: checked out
      +	grep "refusing to fetch into branch" err
      +'
      +
     - test_expect_success 'refuse to overwrite: worktree in bisect' '
     - 	test_when_finished test_might_fail git -C wt-4 bisect reset &&
     - 
     -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in bisect' '
     - 	) &&
     - 
     - 	test_must_fail git branch -f wt-4 HEAD 2>err &&
     --	grep "cannot force update the branch '\''wt-4'\'' checked out at" err
     -+	grep "cannot force update the branch '\''wt-4'\'' checked out at" err &&
     ++test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in bisect' '
     ++	test_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&
      +
     -+	test_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&
     -+	grep "refusing to fetch into branch '\''refs/heads/wt-4'\''" err
     - '
     - 
     - . "$TEST_DIRECTORY"/lib-rebase.sh
     -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in rebase' '
     - 	) &&
     - 
     - 	test_must_fail git branch -f wt-4 HEAD 2>err &&
     --	grep "cannot force update the branch '\''wt-4'\'' checked out at" err
     -+	grep "cannot force update the branch '\''wt-4'\'' checked out at" err &&
     ++	touch .git/worktrees/wt-4/BISECT_LOG &&
     ++	echo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&
     ++
     ++	test_must_fail git fetch server +refs/heads/fake-2:refs/heads/fake-2 2>err &&
     ++	grep "refusing to fetch into branch" err
     ++'
     ++
     ++test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in rebase' '
     ++	test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
     ++
     ++	mkdir -p .git/worktrees/wt-4/rebase-merge &&
     ++	touch .git/worktrees/wt-4/rebase-merge/interactive &&
     ++	echo refs/heads/fake-1 >.git/worktrees/wt-4/rebase-merge/head-name &&
     ++	echo refs/heads/fake-2 >.git/worktrees/wt-4/rebase-merge/onto &&
     ++
     ++	test_must_fail git fetch server +refs/heads/fake-1:refs/heads/fake-1 2>err &&
     ++	grep "refusing to fetch into branch" err
     ++'
      +
     -+	test_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&
     -+	grep "refusing to fetch into branch '\''refs/heads/wt-4'\''" err
     - '
     - 
       test_done
 4:  af645b43032 ! 4:  d7e63f9dfd6 branch: use branch_checked_out() when deleting refs
     @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo
      -			const struct worktree *wt =
      -				find_shared_symref(worktrees, "HEAD", name);
      -			if (wt) {
     -+			char *path;
     -+			if (branch_checked_out(name, &path)) {
     ++			const char *path;
     ++			if ((path = branch_checked_out(name))) {
       				error(_("Cannot delete branch '%s' "
       					"checked out at '%s'"),
      -				      bname.buf, wt->path);
      +				      bname.buf, path);
     -+				free(path);
       				ret = 1;
       				continue;
       			}
 -:  ----------- > 5:  0aa9478bc38 branch: fix branch_checked_out() leaks

From: Derrick Stolee <derrickstolee@github.com>

The validate_new_branchname() method contains a check to see if a branch is checked out in any non-bare worktree. This is intended to prevent a force push that will mess up an existing checkout. This helper is not suitable to performing just that check, because the method will die() when the branch is checked out instead of returning an error code.

Create a new branch_checked_out() helper that performs the most basic form of this check. To ensure we can call branch_checked_out() in a loop with good performance, do a single preparation step that iterates over all worktrees and stores their current HEAD branches in a strmap. The branch_checked_out() helper can then discover these branches using a hash lookup.

This helper is currently missing some key functionality. Namely: it doesn't look for active rebases or bisects which mean that the branch is "checked out" even though HEAD doesn't point to that ref. This functionality will be added in a coming change.

We could use branch_checked_out() in validate_new_branchname(), but this missing functionality would be a regression. However, we have no tests that cover this case!

Add a new test script that will be expanded with these cross-worktree ref updates. The current tests would still pass if we refactored validate_new_branchname() to use this version of branch_checked_out(). The next change will fix that functionality and add the proper test coverage.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 branch.c                  | 36 ++++++++++++++++++++++++++++++++++++
 branch.h                  |  7 +++++++
 t/t2407-worktree-heads.sh | 29 +++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)
 create mode 100755 t/t2407-worktree-heads.sh

From: Derrick Stolee <derrickstolee@github.com>

The branch_checked_out() helper was added by the previous change, but it used an over-simplified view to check if a branch is checked out. It only focused on the HEAD symref, but ignored whether a bisect or rebase was happening.

Teach branch_checked_out() to check for these things, and also add tests to ensure that we do not lose this functionality in the future.

Now that this test coverage exists, we can safely refactor validate_new_branchname() to use branch_checked_out().

Note that we need to prepend "refs/heads/" to the 'state.branch' after calling wt_status_check_*(). We also need to duplicate wt->path so the value is not freed at the end of the call.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 branch.c                  | 35 +++++++++++++++++++++++++++--------
 t/t2407-worktree-heads.sh | 22 ++++++++++++++++++++++
 2 files changed, 49 insertions(+), 8 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

When fetching refs from a remote, it is possible that the refspec will cause use to overwrite a ref that is checked out in a worktree. The existing logic in builtin/fetch.c uses a possibly-slow mechanism. Update those sections to use the new, more efficient branch_checked_out() helper.

These uses were not previously tested, so add a test case that can be used for these kinds of collisions. There is only one test now, but more tests will be added as other consumers of branch_checked_out() are added.

Note that there are two uses in builtin/fetch.c, but only one of the messages is tested. This is because the tested check is run before completing the fetch, and the untested check is not reachable without concurrent updates to the filesystem. Thus, it is beneficial to keep that extra check for the sake of defense-in-depth. However, we should not attempt to test the check, as the effort required is too complicated to be worth the effort. This use in update_local_ref() also requires a change in the error message because we no longer have access to the worktree struct, only the path of the worktree. This error is so rare that making a distinction between the two is not critical.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/fetch.c           | 22 +++++++-----------
 t/t2407-worktree-heads.sh | 47 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 14 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

This is the last current use of find_shared_symref() that can easily be replaced by branch_checked_out(). The benefit of this switch is that the code is a bit simpler, but also it is faster on repeated calls.

The remaining uses of find_shared_symref() are non-trivial to remove, so we probably should not continue in that direction:

* builtin/notes.c uses find_shared_symref() with "NOTES_MERGE_REF"
  instead of "HEAD", so it doesn't have an immediate analogue with
  branch_checked_out(). Perhaps we should consider extending it to
  include that symref in addition to HEAD, BISECT_HEAD, and
  REBASE_HEAD.

* receive-pack.c checks to see if a worktree has a checkout for the ref
  that is being updated. The tricky part is that it can actually decide
  to update the worktree directly instead of just skipping the update.
  This all depends on the receive.denyCurrentBranch config option. The
  implementation currenty cares about receiving the worktree in the
  result, so the current branch_checked_out() prototype is insufficient
  currently. This is something to investigate later, though, since a
  large number of refs could be updated at the same time and using the
  strmap implementation of branch_checked_out() could be beneficial.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/branch.c          | 7 +++----
 t/t2407-worktree-heads.sh | 5 ++++-
 2 files changed, 7 insertions(+), 5 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

The branch_checked_out() method populates a strmap linking a refname to a worktree that has that branch checked out. While unlikely, it is possible that a bug or filesystem manipulation could create a scenario where the same ref is checked out in multiple places. Further, there are some states in an interactive rebase where HEAD and REBASE_HEAD point to the same ref, leading to multiple insertions into the strmap. In either case, the strmap_put() method returns the old value which is leaked.

Update branch_checked_out() to consume that pointer and free it.

Add a test in t2407 that checks this erroneous case. The test "checks itself" by first confirming that the filesystem manipulations it makes trigger the branch_checked_out() logic, and then sets up similar manipulations to make it look like there are multiple worktrees pointing to the same ref.

While TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the leakage and prevent it in the future, t2407 uses helpers such as 'git clone' that cause the test to fail under that mode.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 branch.c                  | 25 +++++++++++++++----------
 t/t2407-worktree-heads.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 10 deletions(-)

Hi Stolee

On 14/06/2022 20:27, Derrick Stolee via GitGitGadget wrote:

The changes look good to me (modulo Peff's fixup), thanks for working on this

Best Wishes

Phillip