| Message ID | dbb7eae390c90d4b710f48d8875bd7db0409aea3.1654718942.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Create branch_checked_out() helper | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > +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)); > + } 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: > From: Derrick Stolee <derrickstolee@github.com> > [...] > + path_in_set = strmap_get(¤t_checked_out_branches, refname); > + if (path_in_set && path) > + *path = xstrdup(path_in_set); 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)).
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(¤t_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(¤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 as const 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 > /* > * 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
On 14/06/2022 11:09, Phillip Wood wrote: > [..] >> +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(). 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. >> + 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. 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/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote: > >> From: Derrick Stolee <derrickstolee@github.com> >> [...] >> + path_in_set = strmap_get(¤t_checked_out_branches, refname); >> + if (path_in_set && path) >> + *path = xstrdup(path_in_set); > > 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. 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'. > 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)). 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 6/14/2022 6:09 AM, Phillip Wood wrote: > Hi Stolee > > On 08/06/2022 21:08, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <derrickstolee@github.com> >> + 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. That .strdup_strings is only for the key, not the value (since the value could be anything, not necessarily a string). >> + } >> + >> + 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 as > > const 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. 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
On Tue, Jun 14 2022, Derrick Stolee wrote: > On 6/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote: >> >> On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote: >> >>> From: Derrick Stolee <derrickstolee@github.com> >>> [...] >>> + path_in_set = strmap_get(¤t_checked_out_branches, refname); >>> + if (path_in_set && path) >>> + *path = xstrdup(path_in_set); >> >> 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. > > 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'. 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. >> 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)). > > 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(). 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)... > 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(). ...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: diff --git a/branch.c b/branch.c index c7896f69a7c..efe1f6ce865 100644 --- a/branch.c +++ b/branch.c @@ -373,14 +373,13 @@ int validate_branchname(const char *name, struct strbuf *ref) static int initialized_checked_out_branches; static struct strmap current_checked_out_branches = STRMAP_INIT; -static void prepare_checked_out_branches(void) +struct strmap *checked_out_branches_map(void) { int i = 0; struct worktree **worktrees; if (initialized_checked_out_branches) - return; - initialized_checked_out_branches = 1; + goto done; worktrees = get_worktrees(); @@ -426,18 +425,10 @@ static void prepare_checked_out_branches(void) } 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; +done: + initialized_checked_out_branches = 1; + return ¤t_checked_out_branches; } /* @@ -456,7 +447,8 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force) die(_("a branch named '%s' already exists"), ref->buf + strlen("refs/heads/")); - if (branch_checked_out(ref->buf, &path)) + path = strmap_get(checked_out_branches_map() , ref->buf); + if (path) die(_("cannot force update the branch '%s' " "checked out at '%s'"), ref->buf + strlen("refs/heads/"), path); diff --git a/branch.h b/branch.h index 5ea93d217b1..63cb0f7adc4 100644 --- a/branch.h +++ b/branch.h @@ -103,11 +103,12 @@ void create_branches_recursively(struct repository *r, const char *name, 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. + * Lazily returns a "struct strmap" of checked out branches. The + * returned value is a global that'll be populated on the first + * call. You should only call read-only strmap API functions on this, + * such as strmap_get(), strmap_contains() etc. */ -int branch_checked_out(const char *refname, char **path); +struct strmap *checked_out_branches_map(void); /* * Check if 'name' can be a valid name for a branch; die otherwise. diff --git a/builtin/branch.c b/builtin/branch.c index 8e11e433840..2913b2e75e4 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -24,6 +24,7 @@ #include "worktree.h" #include "help.h" #include "commit-reach.h" +#include "strmap.h" static const char * const builtin_branch_usage[] = { N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"), @@ -253,12 +254,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, name = mkpathdup(fmt, bname.buf); if (kinds == FILTER_REFS_BRANCHES) { - char *path; - if (branch_checked_out(name, &path)) { + const char *path = strmap_get(checked_out_branches_map(), + name); + if (path) { error(_("Cannot delete branch '%s' " "checked out at '%s'"), bname.buf, path); - free(path); ret = 1; continue; } diff --git a/builtin/fetch.c b/builtin/fetch.c index 9ee7f2241ad..a57506a4003 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -30,6 +30,7 @@ #include "shallow.h" #include "worktree.h" #include "bundle-uri.h" +#include "strmap.h" #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) @@ -890,7 +891,6 @@ static int update_local_ref(struct ref *ref, struct worktree **worktrees) { struct commit *current = NULL, *updated; - char *path = NULL; const char *pretty_ref = prettify_refname(ref->name); int fast_forward = 0; @@ -905,17 +905,17 @@ static int update_local_ref(struct ref *ref, } if (!update_head_ok && - !is_null_oid(&ref->old_oid) && - branch_checked_out(ref->name, &path)) { + !is_null_oid(&ref->old_oid)) { + int in_set = strmap_contains(checked_out_branches_map(), + 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... */ format_display(display, '!', _("[rejected]"), - path ? _("can't fetch in current branch") : + in_set ? _("can't fetch in current branch") : _("checked out in another worktree"), remote, pretty_ref, summary_width); - free(path); return 1; } @@ -1445,7 +1445,8 @@ static void check_not_current_branch(struct ref *ref_map) for (; ref_map; ref_map = ref_map->next) if (ref_map->peer_ref && starts_with(ref_map->peer_ref->name, "refs/heads/") && - branch_checked_out(ref_map->peer_ref->name, &path)) + (path = strmap_get(checked_out_branches_map(), + ref_map->peer_ref->name))) die(_("refusing to fetch into branch '%s' " "checked out at '%s'"), ref_map->peer_ref->name, path);
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; + +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)); + } + + 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; +} + /* * 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