git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/4] branch: add branch_checked_out() helper

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Jun 14, 2022, 10:09 UTC
Message-ID
<42ada1a3-adc2-2c43-9aa1-7854858865ae@gmail.com>
In-Reply-To
<dbb7eae390c90d4b710f48d8875bd7db0409aea3.1654718942.git.gitgitgadget@gmail.com>
Hi Stolee
On 08/06/2022 21:08, Derrick Stolee via GitGitGadget wrote:
Show 56 quoted lines
> 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
> 
> diff --git a/branch.c b/branch.c
> index 2d6569b0c62..061a11f3415 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -10,6 +10,7 @@
>   #include "worktree.h"
>   #include "submodule-config.h"
>   #include "run-command.h"
> +#include "strmap.h"
>   
>   struct tracking {
>   	struct refspec_item spec;
> @@ -369,6 +370,47 @@ int validate_branchname(const char *name, struct strbuf *ref)
>   	return ref_exists(ref->buf);
>   }
>   
> +static int initialized_checked_out_branches;
> +static struct strmap current_checked_out_branches = STRMAP_INIT;

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

Show 21 quoted lines
> +static void prepare_checked_out_branches(void)
> +{
> +	int i = 0;
> +	struct worktree **worktrees;
> +
> +	if (initialized_checked_out_branches)
> +		return;
> +	initialized_checked_out_branches = 1;
> +
> +	worktrees = get_worktrees();
> +
> +	while (worktrees[i]) {
> +		struct worktree *wt = worktrees[i++];
> +
> +		if (wt->is_bare)
> +			continue;
> +
> +		if (wt->head_ref)
> +			strmap_put(&current_checked_out_branches,
> +				   wt->head_ref,
> +				   xstrdup(wt->path));
STRMAP_INIT sets .strdup_strings = 1, so the xstrdup() is unnecessary.
Show 16 quoted lines
> +	}
> +
> +	free_worktrees(worktrees);
> +}
> +
> +int branch_checked_out(const char *refname, char **path)
> +{
> +	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;
> +}
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
Show 52 quoted lines
>   /*
>    * Check if a branch 'name' can be created as a new branch; die otherwise.
>    * 'force' can be used when it is OK for the named branch already exists.
> diff --git a/branch.h b/branch.h
> index 560b6b96a8f..5ea93d217b1 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -101,6 +101,14 @@ void create_branches_recursively(struct repository *r, const char *name,
>   				 const char *tracking_name, int force,
>   				 int reflog, int quiet, enum branch_track track,
>   				 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.
> + */
> +int branch_checked_out(const char *refname, char **path);
> +
>   /*
>    * Check if 'name' can be a valid name for a branch; die otherwise.
>    * Return 1 if the named branch already exists; return 0 otherwise.
> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
> new file mode 100755
> index 00000000000..dd905dc1a5c
> --- /dev/null
> +++ b/t/t2407-worktree-heads.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +
> +test_description='test operations trying to overwrite refs at worktree HEAD'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	for i in 1 2 3 4
> +	do
> +		test_commit $i &&
> +		git branch wt-$i &&
> +		git worktree add wt-$i wt-$i || return 1
> +	done
> +'
> +
> +test_expect_success 'refuse to overwrite: checked out in worktree' '
> +	for i in 1 2 3 4
> +	do
> +		test_must_fail git branch -f wt-$i HEAD 2>err
> +		grep "cannot force update the branch" err || return 1
> +	done
> +'
> +
> +test_done
Previous: Ævar Arnfjörð BjarmasonNext: Phillip Wood
Message 7 of 28 in “Create branch_checked_out() helper”
  1. 0/4 Create branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 8, 2022
  2. 1/4 branch: add branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 8, 2022
  3. Junio C HamanoJun 9, 2022
  4. Ævar Arnfjörð BjarmasonJun 13, 2022
  5. Derrick StoleeJun 14, 2022
  6. Ævar Arnfjörð BjarmasonJun 14, 2022
  7. Phillip WoodJun 14, 2022
  8. Phillip WoodJun 14, 2022
  9. Derrick StoleeJun 14, 2022
  10. 2/4 branch: check for bisects and rebasesDerrick Stolee via GitGitGadget, Jun 8, 2022
  11. Junio C HamanoJun 8, 2022
  12. 3/4 fetch: use new branch_checked_out() and add testsDerrick Stolee via GitGitGadget, Jun 8, 2022
  13. Ævar Arnfjörð BjarmasonJun 14, 2022
  14. Phillip WoodJun 14, 2022
  15. Derrick StoleeJun 14, 2022
  16. 4/4 branch: use branch_checked_out() when deleting refsDerrick Stolee via GitGitGadget, Jun 8, 2022
  17. 5/5 branch: fix branch_checked_out() leaksDerrick Stolee, Jun 13, 2022
  18. Junio C HamanoJun 13, 2022
  19. Ævar Arnfjörð BjarmasonJun 14, 2022
  20. Derrick StoleeJun 14, 2022
  21. Ævar Arnfjörð BjarmasonJun 14, 2022
  22. 0/5 Create branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 14, 2022
  23. 1/5 branch: add branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 14, 2022
  24. 2/5 branch: check for bisects and rebasesDerrick Stolee via GitGitGadget, Jun 14, 2022
  25. 3/5 fetch: use new branch_checked_out() and add testsDerrick Stolee via GitGitGadget, Jun 14, 2022
  26. 4/5 branch: use branch_checked_out() when deleting refsDerrick Stolee via GitGitGadget, Jun 14, 2022
  27. 5/5 branch: fix branch_checked_out() leaksDerrick Stolee via GitGitGadget, Jun 14, 2022
  28. Phillip WoodJun 19, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.