| Message ID | 17f267b1-7f5e-2fb6-fb14-1c37ec355e65@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix die_if_checked_out() when ignore_current_worktree | expand |
Rubén Justo <rjusto@gmail.com> writes: > Let's stop using find_shared_symref() in die_if_checked_out(), to handle > correctly ignore_current_worktree. This says what the code stops using, but does not say what it uses instead. Factoring is_shared_symref() out of find_shared_symref() is probably a good idea that can stand alone without any other change in this patch as a single step, and then a second step can update die_if_checked_out() using the new function. As the proposed log message explained, updating die_if_checked_out() with this patch would fix a bug---can we demonstrate the existing breakage and protect the fix from future breakages by adding a test or two? Other than that, it looks like it is going in the right direction. Thanks for working on the topic. > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > branch.c | 16 +++++++++++----- > worktree.c | 54 +++++++++++++++++++++++++++++------------------------- > worktree.h | 6 ++++++ > 3 files changed, 46 insertions(+), 30 deletions(-) > > diff --git a/branch.c b/branch.c > index d182756827..2378368415 100644 > --- a/branch.c > +++ b/branch.c > @@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose) > void die_if_checked_out(const char *branch, int ignore_current_worktree) > { > struct worktree **worktrees = get_worktrees(); > - const struct worktree *wt; > + int i; > + > + for (i = 0; worktrees[i]; i++) > + { Style. WRite the above on a single line, i.e. for (i = 0; worktrees[i]; i++) { Optionally, we can lose the separate declaration of "i" by using C99 variable declaration, i.e. for (int i = 0; worktrees[i]; i++) { > diff --git a/worktree.c b/worktree.c > index aa43c64119..d500d69e4c 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt, > * bisect). New commands that do similar things should update this > * function as well. > */ The above comment is about find_shared_symref() which iterates over worktrees and find the one that uses the named symref. Now the comment appears to apply to is_shared_symref() which does not iterate but takes one specific worktree instance. Do their differences necessitate some updates to the comment? > +int is_shared_symref(const struct worktree *wt, const char *symref, > + const char *target) > +{ What this function does sound more like "is target in use in this particular worktree by being pointed at by the symref?" IOW, I do not see where "shared" comes into its name from. "HEAD" that is tentatively detached while bisecting or rebasing the "target" branch is still considered to point at the "target", so perhaps symref_points_at_target() or something? > const struct worktree *find_shared_symref(struct worktree **worktrees, > const char *symref, > const char *target) > @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, > int i = 0; > > for (i = 0; worktrees[i]; i++) { Not a new problem, but the initialization on the declaration of "i" is redundant and unnecessary. Again, we can use the C99 style, i.e. const struct worktree *existing = NULL; - int i = 0; - - for (i = 0; worktrees[i]; i++) { + for (int i = 0; worktrees[i]; i++) { > + if (is_shared_symref(worktrees[i], symref, target)) { > + existing = worktrees[i]; > break; > } > } > diff --git a/worktree.h b/worktree.h > index 9dcea6fc8c..7889c4761d 100644 > --- a/worktree.h > +++ b/worktree.h > @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, > const char *symref, > const char *target); > > +/* > + * Returns true if a symref points to a ref in a worktree. > + */ Make it clear that what you called "a ref" in the above is what is called "target" below. > +int is_shared_symref(const struct worktree *wt, > + const char *symref, const char *target); > + > /* > * Similar to head_ref() for all HEADs _except_ one from the current > * worktree, which is covered by head_ref().
On 21-ene-2023 17:50:55, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > Let's stop using find_shared_symref() in die_if_checked_out(), to handle > > correctly ignore_current_worktree. > > This says what the code stops using, but does not say what it uses > instead. I thought the code for that was a better and clearer description. I'll add some details to the message. > Factoring is_shared_symref() out of find_shared_symref() is probably > a good idea that can stand alone without any other change in this > patch as a single step, and then a second step can update > die_if_checked_out() using the new function. OK. I'll split that in two. > As the proposed log message explained, updating die_if_checked_out() > with this patch would fix a bug---can we demonstrate the existing > breakage and protect the fix from future breakages by adding a test > or two? 2/3 and 3/3, I think makes more sense on its own commit. > > - const struct worktree *wt; > > + int i; > > + > > + for (i = 0; worktrees[i]; i++) > > + { > > Style. WRite the above on a single line, i.e. > > for (i = 0; worktrees[i]; i++) { Sorry. I'll fix that. > > Optionally, we can lose the separate declaration of "i" by using C99 > variable declaration, i.e. > > for (int i = 0; worktrees[i]; i++) { OK. Yes, I was playing with this, changed my mind and ended up with this and the other style below, mess. > > > diff --git a/worktree.c b/worktree.c > > index aa43c64119..d500d69e4c 100644 > > --- a/worktree.c > > +++ b/worktree.c > > @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt, > > * bisect). New commands that do similar things should update this > > * function as well. > > */ > > The above comment is about find_shared_symref() which iterates over > worktrees and find the one that uses the named symref. Now the > comment appears to apply to is_shared_symref() which does not > iterate but takes one specific worktree instance. Do their > differences necessitate some updates to the comment? I think the comment still makes sense as is for the new function, both the description and the recommendation. I will review it again. > > > +int is_shared_symref(const struct worktree *wt, const char *symref, > > + const char *target) > > +{ > > What this function does sound more like "is target in use in this > particular worktree by being pointed at by the symref?" IOW, I do > not see where "shared" comes into its name from. > > "HEAD" that is tentatively detached while bisecting or rebasing the > "target" branch is still considered to point at the "target", so > perhaps symref_points_at_target() or something? > I tried to maintain the terms as much as possible. I'll think about the name you suggest. > > const struct worktree *find_shared_symref(struct worktree **worktrees, > > const char *symref, > > const char *target) > > @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, > > int i = 0; > > > > for (i = 0; worktrees[i]; i++) { > > Not a new problem, but the initialization on the declaration of "i" > is redundant and unnecessary. Again, we can use the C99 style, i.e. > > const struct worktree *existing = NULL; > - int i = 0; > - > - for (i = 0; worktrees[i]; i++) { > + for (int i = 0; worktrees[i]; i++) { I'll fix this. > > > + if (is_shared_symref(worktrees[i], symref, target)) { > > + existing = worktrees[i]; > > break; > > } > > } > > diff --git a/worktree.h b/worktree.h > > index 9dcea6fc8c..7889c4761d 100644 > > --- a/worktree.h > > +++ b/worktree.h > > @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, > > const char *symref, > > const char *target); > > > > +/* > > + * Returns true if a symref points to a ref in a worktree. > > + */ > > Make it clear that what you called "a ref" in the above is what is > called "target" below. > Again, that was an attempt to maintain the terms from find_shared_symref(). Thank you for your review.
Rubén Justo <rjusto@gmail.com> writes: >> As the proposed log message explained, updating die_if_checked_out() >> with this patch would fix a bug---can we demonstrate the existing >> breakage and protect the fix from future breakages by adding a test >> or two? > > 2/3 and 3/3, I think makes more sense on its own commit. Hmph, how so? Especially once you split 1/3 into the preliminary refactoring and the real fix, the fix becomes fairly small and clear. And the tests to protect the fix would go best in the same commit. >> The above comment is about find_shared_symref() which iterates over >> worktrees and find the one that uses the named symref. Now the >> comment appears to apply to is_shared_symref() which does not >> iterate but takes one specific worktree instance. Do their >> differences necessitate some updates to the comment? > > I think the comment still makes sense as is for the new function, both the > description and the recommendation. I will review it again. OK. Thanks. >> > +int is_shared_symref(const struct worktree *wt, const char *symref, >> > + const char *target) >> > +{ >> >> What this function does sound more like "is target in use in this >> particular worktree by being pointed at by the symref?" IOW, I do >> not see where "shared" comes into its name from. >> >> "HEAD" that is tentatively detached while bisecting or rebasing the >> "target" branch is still considered to point at the "target", so >> perhaps symref_points_at_target() or something? > > I tried to maintain the terms as much as possible. I'll think about the name > you suggest. When you did not change a thing in such a way that it does not change the relationship between that thing and other things, it makes perfect sense to keep the same term to refer to the thing. Otherwise, once the thing starts playing different roles in the world, there may be a better word to refer to the updated and improved thing.
On 22-ene-2023 11:58:05, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> As the proposed log message explained, updating die_if_checked_out() > >> with this patch would fix a bug---can we demonstrate the existing > >> breakage and protect the fix from future breakages by adding a test > >> or two? > > > > 2/3 and 3/3, I think makes more sense on its own commit. > > Hmph, how so? Especially once you split 1/3 into the preliminary > refactoring and the real fix, the fix becomes fairly small and > clear. And the tests to protect the fix would go best in the same > commit. My intention is to protect rebase (2/3) and switch (3/3). If any of those tests break, even if die_if_checked_out() is no longer used by them, I try to make the original intent clear with that in there. die_if_checked_out() was initially fine, the ignore_current_worktree was unfortunately introduced. I haven't checked, but other callers not affected by the change, i.e. ignore_current_worktree = 0, his tests should have protected them by the change. You are right, in a future reroll, split 1/3 could leave a fairly small commit, maybe not a bad thing. Definitely this need a reroll, because of the style issues, but I will wait some time for other reviewers. > >> The above comment is about find_shared_symref() which iterates over > >> worktrees and find the one that uses the named symref. Now the > >> comment appears to apply to is_shared_symref() which does not > >> iterate but takes one specific worktree instance. Do their > >> differences necessitate some updates to the comment? > > > > I think the comment still makes sense as is for the new function, both the > > description and the recommendation. I will review it again. > > OK. Thanks. > > >> > +int is_shared_symref(const struct worktree *wt, const char *symref, > >> > + const char *target) > >> > +{ > >> > >> What this function does sound more like "is target in use in this > >> particular worktree by being pointed at by the symref?" IOW, I do > >> not see where "shared" comes into its name from. > >> > >> "HEAD" that is tentatively detached while bisecting or rebasing the > >> "target" branch is still considered to point at the "target", so > >> perhaps symref_points_at_target() or something? > > > > I tried to maintain the terms as much as possible. I'll think about the name > > you suggest. > > When you did not change a thing in such a way that it does not > change the relationship between that thing and other things, it > makes perfect sense to keep the same term to refer to the thing. > Otherwise, once the thing starts playing different roles in the > world, there may be a better word to refer to the updated and > improved thing. I tried to maintain the relationship and the role, too. Just introduce the helper, as Phillip suggested and I think it is a good idea. Thank you.
Hi Rubén On 22/01/2023 23:21, Rubén Justo wrote: > I tried to maintain the relationship and the role, too. Just introduce > the helper, as Phillip suggested and I think it is a good idea. When I suggested adding a helper I was thinking of something like static const struct worktree *do_find_shared_symref(struct worktree **worktrees, const char *symref, const char *target, int ignore_current) { /* * Body moved from find_share_symref() with a couple * of lines added to support ignore_current /* } const struct worktree *find_shared_symref(struct worktree **worktrees, const char *symref, const char *target) { return do_find_shared_symref(worktrees, symref, target, 0) } void die_if_checked_out(const char *branch, int ignore_current_worktree) { struct worktree **worktrees = get_worktrees(); const struct worktree *wt; wt = do_find_shared_symref(worktrees, "HEAD", branch, ignore_current_worktree); /* rest unchanged */ } The aim was to avoid changing the public api Best Wishes Phillip
On 24-ene-2023 10:35:26, Phillip Wood wrote: > On 22/01/2023 23:21, Rubén Justo wrote: > > I tried to maintain the relationship and the role, too. Just introduce > > the helper, as Phillip suggested and I think it is a good idea. > > When I suggested adding a helper I was thinking of something like > > static const struct worktree *do_find_shared_symref(struct worktree > **worktrees, > const char *symref, > const char *target, > int ignore_current) > { > /* > * Body moved from find_share_symref() with a couple > * of lines added to support ignore_current > /* > } > > const struct worktree *find_shared_symref(struct worktree **worktrees, > const char *symref, > const char *target) > { > return do_find_shared_symref(worktrees, symref, target, 0) > } > > void die_if_checked_out(const char *branch, int ignore_current_worktree) > { > struct worktree **worktrees = get_worktrees(); > const struct worktree *wt; > > wt = do_find_shared_symref(worktrees, "HEAD", branch, > ignore_current_worktree); > /* rest unchanged */ > } > > The aim was to avoid changing the public api I thought about a solution like the one you suggest. Also another one based on iterations, something like wt_head_refs().... I ended up with is_shared_symref(), it adds some value, I think. The public API remains unchanged, and I like that the comment for find_shared_ref(), which is an important note, is moved to is_shared_symref(), which contains the essential work related to the comment. die_if_checked_out() needs to iterate (here was the wt_heads_refs()), but my intuition makes me think it's a good step since we might need another level, I'm not sure yet but, like "die_if_if_checked_out(allow_if_current)". I'm going to send a v3 addressing the issues Junio commented, still with the is_shared_symref(). If the above reasoning doesn't work for you or if the change as-is introduces any problem, I have no objection to "do_find_shared_symref()". Thank you.
diff --git a/branch.c b/branch.c index d182756827..2378368415 100644 --- a/branch.c +++ b/branch.c @@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose) void die_if_checked_out(const char *branch, int ignore_current_worktree) { struct worktree **worktrees = get_worktrees(); - const struct worktree *wt; + int i; + + for (i = 0; worktrees[i]; i++) + { + if (worktrees[i]->is_current && ignore_current_worktree) + continue; - wt = find_shared_symref(worktrees, "HEAD", branch); - if (wt && (!ignore_current_worktree || !wt->is_current)) { - skip_prefix(branch, "refs/heads/", &branch); - die(_("'%s' is already checked out at '%s'"), branch, wt->path); + if (is_shared_symref(worktrees[i], "HEAD", branch)) { + skip_prefix(branch, "refs/heads/", &branch); + die(_("'%s' is already checked out at '%s'"), + branch, worktrees[i]->path); + } } free_worktrees(worktrees); diff --git a/worktree.c b/worktree.c index aa43c64119..d500d69e4c 100644 --- a/worktree.c +++ b/worktree.c @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt, * bisect). New commands that do similar things should update this * function as well. */ +int is_shared_symref(const struct worktree *wt, const char *symref, + const char *target) +{ + const char *symref_target; + struct ref_store *refs; + int flags; + + if (wt->is_bare) + return 0; + + if (wt->is_detached && !strcmp(symref, "HEAD")) { + if (is_worktree_being_rebased(wt, target)) + return 1; + if (is_worktree_being_bisected(wt, target)) + return 1; + } + + refs = get_worktree_ref_store(wt); + symref_target = refs_resolve_ref_unsafe(refs, symref, 0, + NULL, &flags); + if ((flags & REF_ISSYMREF) && + symref_target && !strcmp(symref_target, target)) + return 1; + + return 0; +} + const struct worktree *find_shared_symref(struct worktree **worktrees, const char *symref, const char *target) @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, int i = 0; for (i = 0; worktrees[i]; i++) { - struct worktree *wt = worktrees[i]; - const char *symref_target; - struct ref_store *refs; - int flags; - - if (wt->is_bare) - continue; - - if (wt->is_detached && !strcmp(symref, "HEAD")) { - if (is_worktree_being_rebased(wt, target)) { - existing = wt; - break; - } - if (is_worktree_being_bisected(wt, target)) { - existing = wt; - break; - } - } - - refs = get_worktree_ref_store(wt); - symref_target = refs_resolve_ref_unsafe(refs, symref, 0, - NULL, &flags); - if ((flags & REF_ISSYMREF) && - symref_target && !strcmp(symref_target, target)) { - existing = wt; + if (is_shared_symref(worktrees[i], symref, target)) { + existing = worktrees[i]; break; } } diff --git a/worktree.h b/worktree.h index 9dcea6fc8c..7889c4761d 100644 --- a/worktree.h +++ b/worktree.h @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, const char *symref, const char *target); +/* + * Returns true if a symref points to a ref in a worktree. + */ +int is_shared_symref(const struct worktree *wt, + const char *symref, const char *target); + /* * Similar to head_ref() for all HEADs _except_ one from the current * worktree, which is covered by head_ref().
In 8d9fdd7 (worktree.c: check whether branch is rebased in another worktree, 2016-04-22) die_if_checked_out() learned a new option ignore_current_worktree, to modify the operation from "die() if the branch is checked out in any worktree" to "die() if the branch is checked out in any worktree other than the current one". Unfortunately we implemented it by checking the flag is_current in the worktree that find_shared_symref() returns. When the same branch is checked out in several worktrees simultaneously, find_shared_symref() will return the first matching worktree in the list composed by get_worktrees(). If one of the worktrees with the checked out branch is the current worktree, find_shared_symref() may or may not return it, depending on the order of the list. Let's stop using find_shared_symref() in die_if_checked_out(), to handle correctly ignore_current_worktree. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- branch.c | 16 +++++++++++----- worktree.c | 54 +++++++++++++++++++++++++++++------------------------- worktree.h | 6 ++++++ 3 files changed, 46 insertions(+), 30 deletions(-)