Re: [PATCH 1/4] branch: add branch_checked_out() helper
- From
- Phillip 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(¤t_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(¤t_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 asconst char* branch_checked_out(const char *refname)
{
prepare_checked_out_branches();
return strmap_get(¤t_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