| Message ID | 8670d6c6-b5cd-a1e3-8fbf-b948cb687388@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | faa4d5983bc1739351f49269660285a2628a3d72 |
| Headers | show |
| Series | [v3,1/4] worktree: introduce is_shared_symref() | expand |
On Sun, Feb 05 2023, Rubén Justo wrote: > - 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); > + for (int i = 0; worktrees[i]; i++) { I see that there are existing "int i" for counting worktrees in worktree.c, FWIW for new code I wouldn't mind if it's "size_t i" instead, to make it future proof (and to eventually get rid of cast warnings as we move more things from "int" to "size_t"). > @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, > const char *target) > { > > - for (int i = 0; worktrees[i]; i++) { > + for (int i = 0; worktrees[i]; i++) > if (is_shared_symref(worktrees[i], symref, target)) > return worktrees[i]; > - } You added this function in the last commit, let's just skip adding the braces to begin with, rather than this style-fix after the fact.
On 06-feb-2023 17:56:55, Ævar Arnfjörð Bjarmason wrote: > > On Sun, Feb 05 2023, Rubén Justo wrote: > > > - 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); > > + for (int i = 0; worktrees[i]; i++) { > > I see that there are existing "int i" for counting worktrees in > worktree.c, FWIW for new code I wouldn't mind if it's "size_t i" > instead, to make it future proof (and to eventually get rid of cast > warnings as we move more things from "int" to "size_t"). OK. > > @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, > > const char *target) > > { > > > > - for (int i = 0; worktrees[i]; i++) { > > + for (int i = 0; worktrees[i]; i++) > > if (is_shared_symref(worktrees[i], symref, target)) > > return worktrees[i]; > > - } > > You added this function in the last commit, let's just skip adding the > braces to begin with, rather than this style-fix after the fact. OK. Thanks.
On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote: > > On Sun, Feb 05 2023, Rubén Justo wrote: > >> - 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); >> + for (int i = 0; worktrees[i]; i++) { > > I see that there are existing "int i" for counting worktrees in > worktree.c, FWIW for new code I wouldn't mind if it's "size_t i" > instead, to make it future proof (and to eventually get rid of cast > warnings as we move more things from "int" to "size_t"). This seems to be different from the usual worries about int/size_t comparisons/truncation. worktrees is NULL terminated so there is no signed/unsigned comparison here as we're not comparing it to anything. The only concern would be that there are more than INT_MAX which seems unlikely. Best Wishes Phillip >> @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, >> const char *target) >> { >> >> - for (int i = 0; worktrees[i]; i++) { >> + for (int i = 0; worktrees[i]; i++) >> if (is_shared_symref(worktrees[i], symref, target)) >> return worktrees[i]; >> - } > > You added this function in the last commit, let's just skip adding the > braces to begin with, rather than this style-fix after the fact.
On Tue, Feb 07 2023, Phillip Wood wrote: > On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote: >> On Sun, Feb 05 2023, Rubén Justo wrote: >> >>> - 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); >>> + for (int i = 0; worktrees[i]; i++) { >> I see that there are existing "int i" for counting worktrees in >> worktree.c, FWIW for new code I wouldn't mind if it's "size_t i" >> instead, to make it future proof (and to eventually get rid of cast >> warnings as we move more things from "int" to "size_t"). > > This seems to be different from the usual worries about int/size_t > comparisons/truncation. worktrees is NULL terminated so there is no > signed/unsigned comparison here as we're not comparing it to > anything. Having looked at this again I think using "int" for now is the right thing, sorry about the noise. > The only concern would be that there are more than INT_MAX > which seems unlikely. My general concern isn't just with the code that we can prove doesn't overflow in such cases, but that by having different types for such things (which isn't the case here, I thought our "struct worktrees **" would be alloc'd with a "size_t") we end up with coercion warnings. Those warnings are so prevalent in this codebase that we've had to suppress them, and consequently we make it harder to spot the real issues.
diff --git a/branch.c b/branch.c index e5614b53b3..64b7dbfd17 100644 --- a/branch.c +++ b/branch.c @@ -820,12 +820,16 @@ 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; - 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); + for (int i = 0; worktrees[i]; i++) { + if (worktrees[i]->is_current && ignore_current_worktree) + continue; + + 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 40cb9874b7..34043d8fe0 100644 --- a/worktree.c +++ b/worktree.c @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, const char *target) { - for (int i = 0; worktrees[i]; i++) { + for (int i = 0; worktrees[i]; i++) if (is_shared_symref(worktrees[i], symref, target)) return worktrees[i]; - } return NULL; }
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 in the list. Instead of find_shared_symref(), let's do the search using use the recently introduced API is_shared_symref(), and consider ignore_current_worktree when necessary. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- branch.c | 14 +++++++++----- worktree.c | 3 +-- 2 files changed, 10 insertions(+), 7 deletions(-)