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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Stolee

On 08/06/2022 21:08, Derrick Stolee via GitGitGadget wrote:
> 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().

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

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

>   /*
>    * 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