* [PATCH 0/8] various wt-status/worktree cleanups
@ 2020-09-10 19:03 Martin Ågren
2020-09-10 19:03 ` [PATCH 1/8] wt-status: replace sha1 mentions with oid Martin Ågren
` (9 more replies)
0 siblings, 10 replies; 33+ messages in thread
From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Junio noticed that wt-status.c is a bit inconsistent about printing to
`s->fp` vs `stdout`. That's patch two. The third patch tries to
robustify some freeing of memory. While working on that one, I ended up
in the sibling file worktree.c.
This merges pretty well with `seen` (`jc/quote-path-cleanup`) (and the
tests still pass). These patches are obviously low priority, nothing
revolutionary here.
Martin Ågren (8):
wt-status: replace sha1 mentions with oid
wt-status: print to s->fp, not stdout
wt-status: introduce wt_status_state_free_buffers()
worktree: drop useless call to strbuf_reset
worktree: update renamed variable in comment
worktree: rename copy-pasted variable
worktree: use skip_prefix to parse target
worktree: simplify search for unique worktree
wt-status.h | 7 +++++
ref-filter.c | 4 +--
worktree.c | 39 +++++++++++++--------------
wt-status.c | 76 ++++++++++++++++++++++++++++++----------------------
4 files changed, 71 insertions(+), 55 deletions(-)
--
2.28.0.277.g9b3c35fffd
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH 1/8] wt-status: replace sha1 mentions with oid 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 19:03 ` [PATCH 2/8] wt-status: print to s->fp, not stdout Martin Ågren ` (8 subsequent siblings) 9 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano `abbrev_sha1_in_line()` uses a `struct object_id oid` and should be fully prepared to handle non-SHA1 object ids. Rename it to `abbrev_oid_in_line()`. A few comments in `wt_status_get_detached_from()` mention "sha1". The variable they refer to was renamed in e86ab2c1cd ("wt-status: convert to struct object_id", 2017-02-21). Update the comments to reference "oid" instead. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- wt-status.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wt-status.c b/wt-status.c index c560cbe860..59be457015 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1232,7 +1232,7 @@ static int split_commit_in_progress(struct wt_status *s) * The function assumes that the line does not contain useless spaces * before or after the command. */ -static void abbrev_sha1_in_line(struct strbuf *line) +static void abbrev_oid_in_line(struct strbuf *line) { struct strbuf **split; int i; @@ -1282,7 +1282,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines) strbuf_trim(&line); if (!line.len) continue; - abbrev_sha1_in_line(&line); + abbrev_oid_in_line(&line); string_list_append(lines, line.buf); } fclose(f); @@ -1575,9 +1575,9 @@ static void wt_status_get_detached_from(struct repository *r, } if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 && - /* sha1 is a commit? match without further lookup */ + /* oid is a commit? match without further lookup */ (oideq(&cb.noid, &oid) || - /* perhaps sha1 is a tag, try to dereference to a commit */ + /* perhaps oid is a tag, try to dereference to a commit */ ((commit = lookup_commit_reference_gently(r, &oid, 1)) != NULL && oideq(&cb.noid, &commit->object.oid)))) { const char *from = ref; -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/8] wt-status: print to s->fp, not stdout 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren 2020-09-10 19:03 ` [PATCH 1/8] wt-status: replace sha1 mentions with oid Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 19:03 ` [PATCH 3/8] wt-status: introduce wt_status_state_free_buffers() Martin Ågren ` (7 subsequent siblings) 9 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano We pass around a `FILE *` in the `struct wt_status` and almost always print to it. But in a few places, we write to `stdout` instead, either explicitly through `fprintf(stdout, ...)` or implicitly with `printf(...)` (and a few `putchar(...)`). Always be explicit about writing to `s->fp`. To the best of my understanding, this never mattered in practice because these spots are involved in various forms of `git status` which always end up at standard output anyway. When we do write to another file, it's because we're creating a commit message template, and these code paths aren't involved. But let's be consistent to help future readers and avoid future bugs. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- wt-status.c | 57 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/wt-status.c b/wt-status.c index 59be457015..3e0b5d8017 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1806,29 +1806,36 @@ static void wt_longstatus_print(struct wt_status *s) ; /* nothing */ else if (s->workdir_dirty) { if (s->hints) - printf(_("no changes added to commit " - "(use \"git add\" and/or \"git commit -a\")\n")); + fprintf(s->fp, _("no changes added to commit " + "(use \"git add\" and/or " + "\"git commit -a\")\n")); else - printf(_("no changes added to commit\n")); + fprintf(s->fp, _("no changes added to " + "commit\n")); } else if (s->untracked.nr) { if (s->hints) - printf(_("nothing added to commit but untracked files " - "present (use \"git add\" to track)\n")); + fprintf(s->fp, _("nothing added to commit but " + "untracked files present (use " + "\"git add\" to track)\n")); else - printf(_("nothing added to commit but untracked files present\n")); + fprintf(s->fp, _("nothing added to commit but " + "untracked files present\n")); } else if (s->is_initial) { if (s->hints) - printf(_("nothing to commit (create/copy files " - "and use \"git add\" to track)\n")); + fprintf(s->fp, _("nothing to commit (create/" + "copy files and use \"git " + "add\" to track)\n")); else - printf(_("nothing to commit\n")); + fprintf(s->fp, _("nothing to commit\n")); } else if (!s->show_untracked_files) { if (s->hints) - printf(_("nothing to commit (use -u to show untracked files)\n")); + fprintf(s->fp, _("nothing to commit (use -u to " + "show untracked files)\n")); else - printf(_("nothing to commit\n")); + fprintf(s->fp, _("nothing to commit\n")); } else - printf(_("nothing to commit, working tree clean\n")); + fprintf(s->fp, _("nothing to commit, working tree " + "clean\n")); } if(s->show_stash) wt_longstatus_print_stash_summary(s); @@ -1851,12 +1858,12 @@ static void wt_shortstatus_unmerged(struct string_list_item *it, } color_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), "%s", how); if (s->null_termination) { - fprintf(stdout, " %s%c", it->string, 0); + fprintf(s->fp, " %s%c", it->string, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; one = quote_path(it->string, s->prefix, &onebuf); - printf(" %s\n", one); + fprintf(s->fp, " %s\n", one); strbuf_release(&onebuf); } } @@ -1869,16 +1876,16 @@ static void wt_shortstatus_status(struct string_list_item *it, if (d->index_status) color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status); else - putchar(' '); + fputc(' ', s->fp); if (d->worktree_status) color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%c", d->worktree_status); else - putchar(' '); - putchar(' '); + fputc(' ', s->fp); + fputc(' ', s->fp); if (s->null_termination) { - fprintf(stdout, "%s%c", it->string, 0); + fprintf(s->fp, "%s%c", it->string, 0); if (d->rename_source) - fprintf(stdout, "%s%c", d->rename_source, 0); + fprintf(s->fp, "%s%c", d->rename_source, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; @@ -1886,20 +1893,20 @@ static void wt_shortstatus_status(struct string_list_item *it, if (d->rename_source) { one = quote_path(d->rename_source, s->prefix, &onebuf); if (*one != '"' && strchr(one, ' ') != NULL) { - putchar('"'); + fputc('"', s->fp); strbuf_addch(&onebuf, '"'); one = onebuf.buf; } - printf("%s -> ", one); + fprintf(s->fp, "%s -> ", one); strbuf_release(&onebuf); } one = quote_path(it->string, s->prefix, &onebuf); if (*one != '"' && strchr(one, ' ') != NULL) { - putchar('"'); + fputc('"', s->fp); strbuf_addch(&onebuf, '"'); one = onebuf.buf; } - printf("%s\n", one); + fprintf(s->fp, "%s\n", one); strbuf_release(&onebuf); } } @@ -1908,13 +1915,13 @@ static void wt_shortstatus_other(struct string_list_item *it, struct wt_status *s, const char *sign) { if (s->null_termination) { - fprintf(stdout, "%s %s%c", sign, it->string, 0); + fprintf(s->fp, "%s %s%c", sign, it->string, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; one = quote_path(it->string, s->prefix, &onebuf); color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign); - printf(" %s\n", one); + fprintf(s->fp, " %s\n", one); strbuf_release(&onebuf); } } -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 3/8] wt-status: introduce wt_status_state_free_buffers() 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren 2020-09-10 19:03 ` [PATCH 1/8] wt-status: replace sha1 mentions with oid Martin Ågren 2020-09-10 19:03 ` [PATCH 2/8] wt-status: print to s->fp, not stdout Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 19:03 ` [PATCH 4/8] worktree: drop useless call to strbuf_reset Martin Ågren ` (6 subsequent siblings) 9 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano When we have a `struct wt_status_state`, we manually free its `branch`, `onto` and `detached_from`, or sometimes just one or two of them. Provide a function `wt_status_state_free_buffers()` which does the freeing. The callers are still aware of these fields, e.g., they check whether `branch` was populated or not. But this way, they don't need to know about *all* of them, and if `struct wt_status_state` gets more fields, they will not need to learn to free them. Users of `struct wt_status` (which contains a `wt_status_state`) already have `wt_status_collect_free_buffers()` (corresponding to `wt_status_collect()`) which we can also teach to use this new helper. Finally, note that we're currently leaving dangling pointers behind. Some callers work on a stack-allocated struct, where this is obviously ok. But for the users of `run_status()` in builtin/commit.c, there are ample opportunities for someone to mistakenly use those dangling pointers. We seem to be ok for now, but it's a use-after-free waiting to happen. Let's leave NULL-pointers behind instead. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- wt-status.h | 7 +++++++ ref-filter.c | 4 +--- worktree.c | 5 ++--- wt-status.c | 11 ++++++++--- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/wt-status.h b/wt-status.h index f1fa0ec1a7..35b44c388e 100644 --- a/wt-status.h +++ b/wt-status.h @@ -151,7 +151,14 @@ void wt_status_add_cut_line(FILE *fp); void wt_status_prepare(struct repository *r, struct wt_status *s); void wt_status_print(struct wt_status *s); void wt_status_collect(struct wt_status *s); +/* + * Frees the buffers allocated by wt_status_collect. + */ void wt_status_collect_free_buffers(struct wt_status *s); +/* + * Frees the buffers of the wt_status_state. + */ +void wt_status_state_free_buffers(struct wt_status_state *s); void wt_status_get_state(struct repository *repo, struct wt_status_state *state, int get_detached_from); diff --git a/ref-filter.c b/ref-filter.c index 8447cb09be..3f63bb01de 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1502,9 +1502,7 @@ char *get_head_description(void) strbuf_addstr(&desc, _("no branch")); strbuf_addch(&desc, ')'); - free(state.branch); - free(state.onto); - free(state.detached_from); + wt_status_state_free_buffers(&state); return strbuf_detach(&desc, NULL); } diff --git a/worktree.c b/worktree.c index cba2e54598..23dd547e44 100644 --- a/worktree.c +++ b/worktree.c @@ -369,8 +369,7 @@ int is_worktree_being_rebased(const struct worktree *wt, state.branch && starts_with(target, "refs/heads/") && !strcmp(state.branch, target + strlen("refs/heads/"))); - free(state.branch); - free(state.onto); + wt_status_state_free_buffers(&state); return found_rebase; } @@ -385,7 +384,7 @@ int is_worktree_being_bisected(const struct worktree *wt, state.branch && starts_with(target, "refs/heads/") && !strcmp(state.branch, target + strlen("refs/heads/")); - free(state.branch); + wt_status_state_free_buffers(&state); return found_rebase; } diff --git a/wt-status.c b/wt-status.c index 3e0b5d8017..5c4cc4805f 100644 --- a/wt-status.c +++ b/wt-status.c @@ -782,9 +782,14 @@ void wt_status_collect(struct wt_status *s) void wt_status_collect_free_buffers(struct wt_status *s) { - free(s->state.branch); - free(s->state.onto); - free(s->state.detached_from); + wt_status_state_free_buffers(&s->state); +} + +void wt_status_state_free_buffers(struct wt_status_state *state) +{ + FREE_AND_NULL(state->branch); + FREE_AND_NULL(state->onto); + FREE_AND_NULL(state->detached_from); } static void wt_longstatus_print_unmerged(struct wt_status *s) -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 4/8] worktree: drop useless call to strbuf_reset 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren ` (2 preceding siblings ...) 2020-09-10 19:03 ` [PATCH 3/8] wt-status: introduce wt_status_state_free_buffers() Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 19:15 ` Eric Sunshine 2020-09-10 19:03 ` [PATCH 5/8] worktree: update renamed variable in comment Martin Ågren ` (5 subsequent siblings) 9 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano There's no need to reset the strbuf immediately after initializing it. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 1 - 1 file changed, 1 deletion(-) diff --git a/worktree.c b/worktree.c index 23dd547e44..64a9e78997 100644 --- a/worktree.c +++ b/worktree.c @@ -552,7 +552,6 @@ const char *worktree_ref(const struct worktree *wt, const char *refname) { static struct strbuf sb = STRBUF_INIT; - strbuf_reset(&sb); strbuf_worktree_ref(wt, &sb, refname); return sb.buf; } -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset 2020-09-10 19:03 ` [PATCH 4/8] worktree: drop useless call to strbuf_reset Martin Ågren @ 2020-09-10 19:15 ` Eric Sunshine 2020-09-10 19:39 ` Martin Ågren 0 siblings, 1 reply; 33+ messages in thread From: Eric Sunshine @ 2020-09-10 19:15 UTC (permalink / raw) To: Martin Ågren; +Cc: Git List, Junio C Hamano On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren <martin.agren@gmail.com> wrote: > There's no need to reset the strbuf immediately after initializing it. > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > diff --git a/worktree.c b/worktree.c > @@ -552,7 +552,6 @@ const char *worktree_ref(const struct worktree *wt, const char *refname) > { > static struct strbuf sb = STRBUF_INIT; > > - strbuf_reset(&sb); > strbuf_worktree_ref(wt, &sb, refname); > return sb.buf; > } I think this patch is wrong and should be dropped. That strbuf is static, and the called strbuf_worktree_ref() does not reset it, so each call to worktree_ref() will now merely append to the existing content (which is undesirable) following this change. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset 2020-09-10 19:15 ` Eric Sunshine @ 2020-09-10 19:39 ` Martin Ågren 2020-09-10 19:49 ` Eric Sunshine 0 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:39 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano On Thu, 10 Sep 2020 at 21:15, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren <martin.agren@gmail.com> wrote: > > There's no need to reset the strbuf immediately after initializing it. > > > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > > --- > > diff --git a/worktree.c b/worktree.c > > @@ -552,7 +552,6 @@ const char *worktree_ref(const struct worktree *wt, const char *refname) > > { > > static struct strbuf sb = STRBUF_INIT; > > > > - strbuf_reset(&sb); > > strbuf_worktree_ref(wt, &sb, refname); > > return sb.buf; > > } > > I think this patch is wrong and should be dropped. That strbuf is > static, and the called strbuf_worktree_ref() does not reset it, so > each call to worktree_ref() will now merely append to the existing > content (which is undesirable) following this change. Oh wow, that's embarrassing. Thank you so much for spotting. I wonder how many worktrees you need before this optimization to avoid continuous allocation starts paying off. I guess our test runs never actually hit this. Unless the tests are loose enough that my bug manages to go unnoticed even if it actually triggers during a test run. That's not to say this optimization won't ever be useful, of course. I also begin to hope that no caller keeps their returned pointer around for long. It only seems to be used from `other_ref_heads()` and that looks ok. If we do want this strbuf reuse, maybe that function could just keep its own strbuf and reuse it (not necessarily having it be static) and learn not to call `worktree_ref(wt, "HEAD")` twice. But anyway, this patch should definitely be dropped. Thanks Martin ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset 2020-09-10 19:39 ` Martin Ågren @ 2020-09-10 19:49 ` Eric Sunshine 2020-09-12 14:02 ` Martin Ågren 0 siblings, 1 reply; 33+ messages in thread From: Eric Sunshine @ 2020-09-10 19:49 UTC (permalink / raw) To: Martin Ågren; +Cc: Git List, Junio C Hamano On Thu, Sep 10, 2020 at 3:40 PM Martin Ågren <martin.agren@gmail.com> wrote: > On Thu, 10 Sep 2020 at 21:15, Eric Sunshine <sunshine@sunshineco.com> wrote: > > I think this patch is wrong and should be dropped. That strbuf is > > static, and the called strbuf_worktree_ref() does not reset it, so > > each call to worktree_ref() will now merely append to the existing > > content (which is undesirable) following this change. > > That's not to say this optimization won't ever be useful, of course. I > also begin to hope that no caller keeps their returned pointer around > for long. It only seems to be used from `other_ref_heads()` and that > looks ok. If we do want this strbuf reuse, maybe that function could > just keep its own strbuf and reuse it (not necessarily having it be > static) and learn not to call `worktree_ref(wt, "HEAD")` twice. Yep, I wouldn't be unhappy to see worktree_ref() disappear altogether. There are no external callers and it would be easy enough to retrofit the lone internal caller to use the safer strbuf_worktree_ref() anyhow. Plus, both calls to worktree_ref() in other_head_refs() invoke it with the exact same arguments, `worktree_ref(wt, "HEAD")`, which makes one wonder if it need be called twice at all in that particular scenario. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset 2020-09-10 19:49 ` Eric Sunshine @ 2020-09-12 14:02 ` Martin Ågren 0 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-12 14:02 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano On Thu, 10 Sep 2020 at 21:49, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Sep 10, 2020 at 3:40 PM Martin Ågren <martin.agren@gmail.com> wrote: > > That's not to say this optimization won't ever be useful, of course. I > > also begin to hope that no caller keeps their returned pointer around > > for long. It only seems to be used from `other_ref_heads()` and that > > looks ok. If we do want this strbuf reuse, maybe that function could > > just keep its own strbuf and reuse it (not necessarily having it be > > static) and learn not to call `worktree_ref(wt, "HEAD")` twice. > > Yep, I wouldn't be unhappy to see worktree_ref() disappear altogether. > There are no external callers and it would be easy enough to retrofit > the lone internal caller to use the safer strbuf_worktree_ref() > anyhow. Plus, both calls to worktree_ref() in other_head_refs() invoke > it with the exact same arguments, `worktree_ref(wt, "HEAD")`, which > makes one wonder if it need be called twice at all in that particular > scenario. I'll look at this hopefully tomorrow. Thanks for all your comments. Martin ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 5/8] worktree: update renamed variable in comment 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren ` (3 preceding siblings ...) 2020-09-10 19:03 ` [PATCH 4/8] worktree: drop useless call to strbuf_reset Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 19:03 ` [PATCH 6/8] worktree: rename copy-pasted variable Martin Ågren ` (4 subsequent siblings) 9 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano The comment above `add_head_info()` mentions "head_sha1", but it was renamed to "head_oid" in 0f05154c70 ("worktree: convert struct worktree to object_id", 2017-10-15). Update the comment. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worktree.c b/worktree.c index 64a9e78997..050f22dd65 100644 --- a/worktree.c +++ b/worktree.c @@ -21,7 +21,7 @@ void free_worktrees(struct worktree **worktrees) } /** - * Update head_sha1, head_ref and is_detached of the given worktree + * Update head_oid, head_ref and is_detached of the given worktree */ static void add_head_info(struct worktree *wt) { -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 6/8] worktree: rename copy-pasted variable 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren ` (4 preceding siblings ...) 2020-09-10 19:03 ` [PATCH 5/8] worktree: update renamed variable in comment Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 20:29 ` Junio C Hamano 2020-09-10 19:03 ` [PATCH 7/8] worktree: use skip_prefix to parse target Martin Ågren ` (3 subsequent siblings) 9 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano As the commit message of 04a3dfb8b5 ("worktree.c: check whether branch is bisected in another worktree", 2016-04-22) indicates, the function `is_worktree_being_bisected()` is based on the older function `is_worktree_being_rebased()`. This heritage can also be seen in the name of the variable where we store our return value: It was never adapted while copy-editing and remains as `found_rebase`. Rename the variable to make clear that we're looking for a bisect(ion), nothing else. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/worktree.c b/worktree.c index 050f22dd65..a2d0d20564 100644 --- a/worktree.c +++ b/worktree.c @@ -377,15 +377,15 @@ int is_worktree_being_bisected(const struct worktree *wt, const char *target) { struct wt_status_state state; - int found_rebase; + int found_bisect; memset(&state, 0, sizeof(state)); - found_rebase = wt_status_check_bisect(wt, &state) && - state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/")); + found_bisect = wt_status_check_bisect(wt, &state) && + state.branch && + starts_with(target, "refs/heads/") && + !strcmp(state.branch, target + strlen("refs/heads/")); wt_status_state_free_buffers(&state); - return found_rebase; + return found_bisect; } /* -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 6/8] worktree: rename copy-pasted variable 2020-09-10 19:03 ` [PATCH 6/8] worktree: rename copy-pasted variable Martin Ågren @ 2020-09-10 20:29 ` Junio C Hamano 2020-09-12 14:01 ` Martin Ågren 0 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2020-09-10 20:29 UTC (permalink / raw) To: Martin Ågren; +Cc: git Martin Ågren <martin.agren@gmail.com> writes: > As the commit message of 04a3dfb8b5 ("worktree.c: check whether branch > is bisected in another worktree", 2016-04-22) indicates, the function > `is_worktree_being_bisected()` is based on the older function > `is_worktree_being_rebased()`. This heritage can also be seen in the > name of the variable where we store our return value: It was never > adapted while copy-editing and remains as `found_rebase`. > > Rename the variable to make clear that we're looking for a bisect(ion), > nothing else. How bad is this copy and paste? Is it a possibility to make a single helper function and these existing two a thin wrapper around the helper that passes customization between bisect and rebase? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/8] worktree: rename copy-pasted variable 2020-09-10 20:29 ` Junio C Hamano @ 2020-09-12 14:01 ` Martin Ågren 2020-09-27 13:29 ` Martin Ågren 0 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-12 14:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List On Thu, 10 Sep 2020 at 22:29, Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > > As the commit message of 04a3dfb8b5 ("worktree.c: check whether branch > > is bisected in another worktree", 2016-04-22) indicates, the function > > `is_worktree_being_bisected()` is based on the older function > > `is_worktree_being_rebased()`. This heritage can also be seen in the > > name of the variable where we store our return value: It was never > > adapted while copy-editing and remains as `found_rebase`. > > > > Rename the variable to make clear that we're looking for a bisect(ion), > > nothing else. > > How bad is this copy and paste? Is it a possibility to make a > single helper function and these existing two a thin wrapper around > the helper that passes customization between bisect and rebase? That's a good point. I'll look into it. Thanks Martin ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/8] worktree: rename copy-pasted variable 2020-09-12 14:01 ` Martin Ågren @ 2020-09-27 13:29 ` Martin Ågren 0 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:29 UTC (permalink / raw) To: git; +Cc: Junio C Hamano On Sat, 12 Sep 2020 at 16:01, Martin Ågren <martin.agren@gmail.com> wrote: > > On Thu, 10 Sep 2020 at 22:29, Junio C Hamano <gitster@pobox.com> wrote: > > > > Martin Ågren <martin.agren@gmail.com> writes: > > > > > As the commit message of 04a3dfb8b5 ("worktree.c: check whether branch > > > is bisected in another worktree", 2016-04-22) indicates, the function > > > `is_worktree_being_bisected()` is based on the older function > > > `is_worktree_being_rebased()`. This heritage can also be seen in the > > > name of the variable where we store our return value: It was never > > > adapted while copy-editing and remains as `found_rebase`. > > > > How bad is this copy and paste? Is it a possibility to make a > > single helper function and these existing two a thin wrapper around > > the helper that passes customization between bisect and rebase? > > That's a good point. I'll look into it. I did look into this, and here's what I came up with (this is on top of v2 that I just sent out, since that's how I tried it out). If there would be three or four such similar functions, I would feel a lot more confident going with this approach, since it'd be sort of obvious that they were all the same. But for "only" two it somehow feels a bit brittle. If any of those two functions need to do something differently, we might need to start shuffling logic between the helper and the callers. Or we'll end up with just a single caller, the other having broken away from the helper. So in the end I didn't take the plunge for v2. (BTW, the naming in this diff is clearly w-i-p grade...) Martin diff --git a/worktree.c b/worktree.c index a75230a950..4009856b54 100644 --- a/worktree.c +++ b/worktree.c @@ -356,36 +356,40 @@ void update_worktree_location(struct worktree *wt, const char *path_) strbuf_release(&path); } +static int is_worktree_being_x(int (*check_fn)(const struct worktree *wt, + struct wt_status_state *state), + const struct worktree *wt, + const char *target) +{ + struct wt_status_state state = { 0 }; + int found; + + found = check_fn(wt, &state) && + state.branch && + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); + wt_status_state_free_buffers(&state); + return found; +} + +static int check_rebase_in_progress(const struct worktree *wt, + struct wt_status_state *state) +{ + return wt_status_check_rebase(wt, state) && + (state->rebase_in_progress || + state->rebase_interactive_in_progress); +} + int is_worktree_being_rebased(const struct worktree *wt, const char *target) { - struct wt_status_state state; - int found_rebase; - - memset(&state, 0, sizeof(state)); - found_rebase = wt_status_check_rebase(wt, &state) && - (state.rebase_in_progress || - state.rebase_interactive_in_progress) && - state.branch && - skip_prefix(target, "refs/heads/", &target) && - !strcmp(state.branch, target); - wt_status_state_free_buffers(&state); - return found_rebase; + return is_worktree_being_x(check_rebase_in_progress, wt, target); } int is_worktree_being_bisected(const struct worktree *wt, const char *target) { - struct wt_status_state state; - int found_bisect; - - memset(&state, 0, sizeof(state)); - found_bisect = wt_status_check_bisect(wt, &state) && - state.branch && - skip_prefix(target, "refs/heads/", &target) && - !strcmp(state.branch, target); - wt_status_state_free_buffers(&state); - return found_bisect; + return is_worktree_being_x(wt_status_check_bisect, wt, target); } /* -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 7/8] worktree: use skip_prefix to parse target 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren ` (5 preceding siblings ...) 2020-09-10 19:03 ` [PATCH 6/8] worktree: rename copy-pasted variable Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 19:03 ` [PATCH 8/8] worktree: simplify search for unique worktree Martin Ågren ` (2 subsequent siblings) 9 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano Instead of checking for "refs/heads/" using `starts_with()`, then skipping past "refs/heads/" using `strlen()`, just use `skip_prefix()`. In `is_worktree_being_rebased()`, we can adjust the indentation while we're here and lose a pair of parentheses which isn't needed and which might even make the reader wonder what they're missing and why that grouping is there. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/worktree.c b/worktree.c index a2d0d20564..faac87422c 100644 --- a/worktree.c +++ b/worktree.c @@ -364,11 +364,11 @@ int is_worktree_being_rebased(const struct worktree *wt, memset(&state, 0, sizeof(state)); found_rebase = wt_status_check_rebase(wt, &state) && - ((state.rebase_in_progress || - state.rebase_interactive_in_progress) && - state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/"))); + (state.rebase_in_progress || + state.rebase_interactive_in_progress) && + state.branch && + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); wt_status_state_free_buffers(&state); return found_rebase; } @@ -382,8 +382,8 @@ int is_worktree_being_bisected(const struct worktree *wt, memset(&state, 0, sizeof(state)); found_bisect = wt_status_check_bisect(wt, &state) && state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/")); + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); wt_status_state_free_buffers(&state); return found_bisect; } -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 8/8] worktree: simplify search for unique worktree 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren ` (6 preceding siblings ...) 2020-09-10 19:03 ` [PATCH 7/8] worktree: use skip_prefix to parse target Martin Ågren @ 2020-09-10 19:03 ` Martin Ågren 2020-09-10 19:28 ` Eric Sunshine 2020-09-12 3:49 ` [PATCH 0/8] various wt-status/worktree cleanups Taylor Blau 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren 9 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:03 UTC (permalink / raw) To: git; +Cc: Junio C Hamano We track the number of worktrees we've found and break out of the loop early once we hit 2. This is because we're not really interested in the number of matches -- we just want to make sure that we don't find more than one worktree that matches the suffix. This can be done just as well by checking the NULL-ness of the pointer where we collect our answer-to-be. Drop the redundant `nr_found` variable. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/worktree.c b/worktree.c index faac87422c..ac754b88ff 100644 --- a/worktree.c +++ b/worktree.c @@ -172,13 +172,13 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list, const char *suffix) { struct worktree *found = NULL; - int nr_found = 0, suffixlen; + int suffixlen; suffixlen = strlen(suffix); if (!suffixlen) return NULL; - for (; *list && nr_found < 2; list++) { + for (; *list; list++) { const char *path = (*list)->path; int pathlen = strlen(path); int start = pathlen - suffixlen; @@ -186,11 +186,12 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list, /* suffix must start at directory boundary */ if ((!start || (start > 0 && is_dir_sep(path[start - 1]))) && !fspathcmp(suffix, path + start)) { + if (found) + return NULL; found = *list; - nr_found++; } } - return nr_found == 1 ? found : NULL; + return found; } struct worktree *find_worktree(struct worktree **list, -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 8/8] worktree: simplify search for unique worktree 2020-09-10 19:03 ` [PATCH 8/8] worktree: simplify search for unique worktree Martin Ågren @ 2020-09-10 19:28 ` Eric Sunshine 2020-09-10 19:48 ` Martin Ågren 0 siblings, 1 reply; 33+ messages in thread From: Eric Sunshine @ 2020-09-10 19:28 UTC (permalink / raw) To: Martin Ågren; +Cc: Git List, Junio C Hamano On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren <martin.agren@gmail.com> wrote: > We track the number of worktrees we've found and break out of the loop > early once we hit 2. This is because we're not really interested in the > number of matches -- we just want to make sure that we don't find more > than one worktree that matches the suffix. This can be done just as well > by checking the NULL-ness of the pointer where we collect our > answer-to-be. Drop the redundant `nr_found` variable. > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > diff --git a/worktree.c b/worktree.c > @@ -172,13 +172,13 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list, > const char *suffix) > { > - for (; *list && nr_found < 2; list++) { > + for (; *list; list++) { > @@ -186,11 +186,12 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list, > if ((!start || (start > 0 && is_dir_sep(path[start - 1]))) && > !fspathcmp(suffix, path + start)) { > + if (found) > + return NULL; > found = *list; > - nr_found++; > } > } > - return nr_found == 1 ? found : NULL; > + return found; Although this change appears to be correct and does simplify the code, I think it also makes it a bit more opaque. With the explicit `nr_found == 1`, it is quite obvious that the function considers "success" to be when one and only one entry is found and any other number is failure. But with the revised code, it is harder to work out precisely what the conditions are. Having said that, I think a simple comment before the function would suffice to clear up the opaqueness. Perhaps something like: /* If exactly one worktree matches 'target', return it, else NULL. */ ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 8/8] worktree: simplify search for unique worktree 2020-09-10 19:28 ` Eric Sunshine @ 2020-09-10 19:48 ` Martin Ågren 2020-09-10 20:01 ` Eric Sunshine 0 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-10 19:48 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano On Thu, 10 Sep 2020 at 21:28, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren <martin.agren@gmail.com> wrote: > > We track the number of worktrees we've found and break out of the loop > > early once we hit 2. This is because we're not really interested in the > > number of matches -- we just want to make sure that we don't find more > > than one worktree that matches the suffix. This can be done just as well > > by checking the NULL-ness of the pointer where we collect our > > answer-to-be. Drop the redundant `nr_found` variable. > > > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > > --- > > diff --git a/worktree.c b/worktree.c > > @@ -172,13 +172,13 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list, > > const char *suffix) > > { > > - for (; *list && nr_found < 2; list++) { > > + for (; *list; list++) { > > @@ -186,11 +186,12 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list, > > if ((!start || (start > 0 && is_dir_sep(path[start - 1]))) && > > !fspathcmp(suffix, path + start)) { > > + if (found) > > + return NULL; > > found = *list; > > - nr_found++; > > } > > } > > - return nr_found == 1 ? found : NULL; > > + return found; > > Although this change appears to be correct and does simplify the code, > I think it also makes it a bit more opaque. With the explicit > `nr_found == 1`, it is quite obvious that the function considers > "success" to be when one and only one entry is found and any other > number is failure. But with the revised code, it is harder to work out > precisely what the conditions are. Thanks for commenting. I found the original trickier than it had to be. It spreads out the logic in several places and is careful to short-cut the loop. My first thought was "why doesn't this just use the standard form?". But I'm open to the idea that it might be a fairly personal standard form... If there's any risk that someone else comes along and simplifies this to use a `nr_found` variable, then maybe file this under code churning? > Having said that, I think a simple > comment before the function would suffice to clear up the opaqueness. > Perhaps something like: > > /* If exactly one worktree matches 'target', return it, else NULL. */ That's a good suggestion regardless. Thanks Martin ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 8/8] worktree: simplify search for unique worktree 2020-09-10 19:48 ` Martin Ågren @ 2020-09-10 20:01 ` Eric Sunshine 2020-09-10 21:08 ` Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: Eric Sunshine @ 2020-09-10 20:01 UTC (permalink / raw) To: Martin Ågren; +Cc: Git List, Junio C Hamano On Thu, Sep 10, 2020 at 3:48 PM Martin Ågren <martin.agren@gmail.com> wrote: > On Thu, 10 Sep 2020 at 21:28, Eric Sunshine <sunshine@sunshineco.com> wrote: > > Although this change appears to be correct and does simplify the code, > > I think it also makes it a bit more opaque. With the explicit > > `nr_found == 1`, it is quite obvious that the function considers > > "success" to be when one and only one entry is found and any other > > number is failure. But with the revised code, it is harder to work out > > precisely what the conditions are. > > Thanks for commenting. I found the original trickier than it had to be. > It spreads out the logic in several places and is careful to short-cut > the loop. My first thought was "why doesn't this just use the standard > form?". But I'm open to the idea that it might be a fairly personal > standard form... If there's any risk that someone else comes along and > simplifies this to use a `nr_found` variable, then maybe file this under > code churning? Maybe. Dunno. Even with the suggested function comment, I still find that the revised code has a higher cognitive load because the reader has to remember or figure out mentally what `found` contains by the `return found;` at the end of the function. Compare that with the old code, in which the reader doesn't have to remember or figure out anything. The final `return nr_found == 1 ? found : NULL;` condition spells out everything the reader needs to know -- even if the reader didn't pay close attention to the meat of the function. So, we each have a different take on the apparent complexity. > > Having said that, I think a simple > > comment before the function would suffice to clear up the opaqueness. > > Perhaps something like: > > > > /* If exactly one worktree matches 'target', return it, else NULL. */ > > That's a good suggestion regardless. The function is so small that the increased cognitive load (for me) in the rewrite probably doesn't matter at all, so I don't feel strongly one way or the other. Keeping the patch (amended with the suggested comment) or dropping it are both suitable options. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 8/8] worktree: simplify search for unique worktree 2020-09-10 20:01 ` Eric Sunshine @ 2020-09-10 21:08 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2020-09-10 21:08 UTC (permalink / raw) To: Eric Sunshine; +Cc: Martin Ågren, Git List Eric Sunshine <sunshine@sunshineco.com> writes: >> Thanks for commenting. I found the original trickier than it had to be. >> It spreads out the logic in several places and is careful to short-cut >> the loop. My first thought was "why doesn't this just use the standard >> form?". But I'm open to the idea that it might be a fairly personal >> standard form... If there's any risk that someone else comes along and >> simplifies this to use a `nr_found` variable, then maybe file this under >> code churning? > > Maybe. Dunno. Even with the suggested function comment, I still find > that the revised code has a higher cognitive load because the reader > has to remember or figure out mentally what `found` contains by the > `return found;` at the end of the function. Judging from the phrase "standard form", I have a feeling that Martin thinks that the updated code is more intuitive (i.e. "we see another one while keeping the one we already found, so we know there is no unique answer"). I do not claim that would be the standard way and using a counter clamped to 2 is substandard, but I find the code more readable with the patch than the original. Even though it helps somewhat that the counter is named "nr_found" and not "nr_match", I found it a bit awkward that the counter in the original pretends to count all the answers, yet does not really count all of them and stops at 2. I agree with Martin that this would probably be subjective. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/8] various wt-status/worktree cleanups 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren ` (7 preceding siblings ...) 2020-09-10 19:03 ` [PATCH 8/8] worktree: simplify search for unique worktree Martin Ågren @ 2020-09-12 3:49 ` Taylor Blau 2020-09-12 14:03 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren 9 siblings, 1 reply; 33+ messages in thread From: Taylor Blau @ 2020-09-12 3:49 UTC (permalink / raw) To: Martin Ågren; +Cc: git, Junio C Hamano On Thu, Sep 10, 2020 at 09:03:34PM +0200, Martin Ågren wrote: > This merges pretty well with `seen` (`jc/quote-path-cleanup`) (and the > tests still pass). These patches are obviously low priority, nothing > revolutionary here. What revision is this series based on, again? Applying the first patch on top of 'seen' (bf3e2864f3, at the time of writing) produces a conflict due to the dropped parameter in 'dwim_ref()'. But, applying in directly on top of Junio's 'jc/quote-path-cleanup' from his repository handles the first patch just fine, but fails in the second patch. The conflicts here are in 'wt_shortstatus_unmerged' and 'wt_shortstatus_status'. I didn't look deeply to figure out what exactly was going on here, but it would be good to know so that I can play with these patches a bit myself. Thanks, Taylor ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/8] various wt-status/worktree cleanups 2020-09-12 3:49 ` [PATCH 0/8] various wt-status/worktree cleanups Taylor Blau @ 2020-09-12 14:03 ` Martin Ågren 0 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-12 14:03 UTC (permalink / raw) To: Taylor Blau; +Cc: Git Mailing List, Junio C Hamano On Sat, 12 Sep 2020 at 05:49, Taylor Blau <me@ttaylorr.com> wrote: > > On Thu, Sep 10, 2020 at 09:03:34PM +0200, Martin Ågren wrote: > > This merges pretty well with `seen` (`jc/quote-path-cleanup`) (and the > > tests still pass). These patches are obviously low priority, nothing > > revolutionary here. > > What revision is this series based on, again? [ ... ] This is on top of current maint: 47ae905ffb ("Git 2.28", 2020-07-26) > I didn't look deeply to figure out what exactly was going on here, but > it would be good to know so that I can play with these patches a bit > myself. Ok, beware that there's at least one bug, so you're better off dropping "worktree: drop useless call to strbuf_reset". I'll probably replace it with something to remove that function and basically inline it into its only caller. Martin ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 0/7] various wt-status/worktree cleanups 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren ` (8 preceding siblings ...) 2020-09-12 3:49 ` [PATCH 0/8] various wt-status/worktree cleanups Taylor Blau @ 2020-09-27 13:15 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 1/7] wt-status: replace sha1 mentions with oid Martin Ågren ` (6 more replies) 9 siblings, 7 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau Here's a belated update of my series to clean up wt-status.c and worktree.c a bit. The buggy fourth patch has been removed and replaced with one that inlines `worktree_ref()` into its only caller. Thanks Eric for spotting my initial bug and for the very helpful discussion. The final patch, "worktree: simplify search for unique worktree" is no longer. It felt like it was quite subjective whether it was a "simplification", so let's not spend reviewer quality time on it. Martin Ågren (7): wt-status: replace sha1 mentions with oid wt-status: print to s->fp, not stdout wt-status: introduce wt_status_state_free_buffers() worktree: inline `worktree_ref()` into its only caller worktree: update renamed variable in comment worktree: rename copy-pasted variable worktree: use skip_prefix to parse target worktree.h | 7 ----- wt-status.h | 7 +++++ ref-filter.c | 4 +-- worktree.c | 46 ++++++++++++++----------------- wt-status.c | 76 ++++++++++++++++++++++++++++++---------------------- 5 files changed, 72 insertions(+), 68 deletions(-) Range-diff against v1: 1: 734ea28eff = 1: 734ea28eff wt-status: replace sha1 mentions with oid 2: 7ada884d7c = 2: 7ada884d7c wt-status: print to s->fp, not stdout 3: c08fed5a01 = 3: c08fed5a01 wt-status: introduce wt_status_state_free_buffers() 4: 58a2469cc1 < -: ---------- worktree: drop useless call to strbuf_reset -: ---------- > 4: c4825f461e worktree: inline `worktree_ref()` into its only caller 5: 9c1e321fbc = 5: dbbc57cd83 worktree: update renamed variable in comment 6: 6d913ea3e0 = 6: 70e6cea331 worktree: rename copy-pasted variable 7: 872dc384c5 = 7: f8fbee6234 worktree: use skip_prefix to parse target 8: 8383c246f8 < -: ---------- worktree: simplify search for unique worktree -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 1/7] wt-status: replace sha1 mentions with oid 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren @ 2020-09-27 13:15 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 2/7] wt-status: print to s->fp, not stdout Martin Ågren ` (5 subsequent siblings) 6 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau `abbrev_sha1_in_line()` uses a `struct object_id oid` and should be fully prepared to handle non-SHA1 object ids. Rename it to `abbrev_oid_in_line()`. A few comments in `wt_status_get_detached_from()` mention "sha1". The variable they refer to was renamed in e86ab2c1cd ("wt-status: convert to struct object_id", 2017-02-21). Update the comments to reference "oid" instead. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- wt-status.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wt-status.c b/wt-status.c index c560cbe860..59be457015 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1232,7 +1232,7 @@ static int split_commit_in_progress(struct wt_status *s) * The function assumes that the line does not contain useless spaces * before or after the command. */ -static void abbrev_sha1_in_line(struct strbuf *line) +static void abbrev_oid_in_line(struct strbuf *line) { struct strbuf **split; int i; @@ -1282,7 +1282,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines) strbuf_trim(&line); if (!line.len) continue; - abbrev_sha1_in_line(&line); + abbrev_oid_in_line(&line); string_list_append(lines, line.buf); } fclose(f); @@ -1575,9 +1575,9 @@ static void wt_status_get_detached_from(struct repository *r, } if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 && - /* sha1 is a commit? match without further lookup */ + /* oid is a commit? match without further lookup */ (oideq(&cb.noid, &oid) || - /* perhaps sha1 is a tag, try to dereference to a commit */ + /* perhaps oid is a tag, try to dereference to a commit */ ((commit = lookup_commit_reference_gently(r, &oid, 1)) != NULL && oideq(&cb.noid, &commit->object.oid)))) { const char *from = ref; -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 2/7] wt-status: print to s->fp, not stdout 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren 2020-09-27 13:15 ` [PATCH v2 1/7] wt-status: replace sha1 mentions with oid Martin Ågren @ 2020-09-27 13:15 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers() Martin Ågren ` (4 subsequent siblings) 6 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau We pass around a `FILE *` in the `struct wt_status` and almost always print to it. But in a few places, we write to `stdout` instead, either explicitly through `fprintf(stdout, ...)` or implicitly with `printf(...)` (and a few `putchar(...)`). Always be explicit about writing to `s->fp`. To the best of my understanding, this never mattered in practice because these spots are involved in various forms of `git status` which always end up at standard output anyway. When we do write to another file, it's because we're creating a commit message template, and these code paths aren't involved. But let's be consistent to help future readers and avoid future bugs. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- wt-status.c | 57 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/wt-status.c b/wt-status.c index 59be457015..3e0b5d8017 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1806,29 +1806,36 @@ static void wt_longstatus_print(struct wt_status *s) ; /* nothing */ else if (s->workdir_dirty) { if (s->hints) - printf(_("no changes added to commit " - "(use \"git add\" and/or \"git commit -a\")\n")); + fprintf(s->fp, _("no changes added to commit " + "(use \"git add\" and/or " + "\"git commit -a\")\n")); else - printf(_("no changes added to commit\n")); + fprintf(s->fp, _("no changes added to " + "commit\n")); } else if (s->untracked.nr) { if (s->hints) - printf(_("nothing added to commit but untracked files " - "present (use \"git add\" to track)\n")); + fprintf(s->fp, _("nothing added to commit but " + "untracked files present (use " + "\"git add\" to track)\n")); else - printf(_("nothing added to commit but untracked files present\n")); + fprintf(s->fp, _("nothing added to commit but " + "untracked files present\n")); } else if (s->is_initial) { if (s->hints) - printf(_("nothing to commit (create/copy files " - "and use \"git add\" to track)\n")); + fprintf(s->fp, _("nothing to commit (create/" + "copy files and use \"git " + "add\" to track)\n")); else - printf(_("nothing to commit\n")); + fprintf(s->fp, _("nothing to commit\n")); } else if (!s->show_untracked_files) { if (s->hints) - printf(_("nothing to commit (use -u to show untracked files)\n")); + fprintf(s->fp, _("nothing to commit (use -u to " + "show untracked files)\n")); else - printf(_("nothing to commit\n")); + fprintf(s->fp, _("nothing to commit\n")); } else - printf(_("nothing to commit, working tree clean\n")); + fprintf(s->fp, _("nothing to commit, working tree " + "clean\n")); } if(s->show_stash) wt_longstatus_print_stash_summary(s); @@ -1851,12 +1858,12 @@ static void wt_shortstatus_unmerged(struct string_list_item *it, } color_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), "%s", how); if (s->null_termination) { - fprintf(stdout, " %s%c", it->string, 0); + fprintf(s->fp, " %s%c", it->string, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; one = quote_path(it->string, s->prefix, &onebuf); - printf(" %s\n", one); + fprintf(s->fp, " %s\n", one); strbuf_release(&onebuf); } } @@ -1869,16 +1876,16 @@ static void wt_shortstatus_status(struct string_list_item *it, if (d->index_status) color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status); else - putchar(' '); + fputc(' ', s->fp); if (d->worktree_status) color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%c", d->worktree_status); else - putchar(' '); - putchar(' '); + fputc(' ', s->fp); + fputc(' ', s->fp); if (s->null_termination) { - fprintf(stdout, "%s%c", it->string, 0); + fprintf(s->fp, "%s%c", it->string, 0); if (d->rename_source) - fprintf(stdout, "%s%c", d->rename_source, 0); + fprintf(s->fp, "%s%c", d->rename_source, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; @@ -1886,20 +1893,20 @@ static void wt_shortstatus_status(struct string_list_item *it, if (d->rename_source) { one = quote_path(d->rename_source, s->prefix, &onebuf); if (*one != '"' && strchr(one, ' ') != NULL) { - putchar('"'); + fputc('"', s->fp); strbuf_addch(&onebuf, '"'); one = onebuf.buf; } - printf("%s -> ", one); + fprintf(s->fp, "%s -> ", one); strbuf_release(&onebuf); } one = quote_path(it->string, s->prefix, &onebuf); if (*one != '"' && strchr(one, ' ') != NULL) { - putchar('"'); + fputc('"', s->fp); strbuf_addch(&onebuf, '"'); one = onebuf.buf; } - printf("%s\n", one); + fprintf(s->fp, "%s\n", one); strbuf_release(&onebuf); } } @@ -1908,13 +1915,13 @@ static void wt_shortstatus_other(struct string_list_item *it, struct wt_status *s, const char *sign) { if (s->null_termination) { - fprintf(stdout, "%s %s%c", sign, it->string, 0); + fprintf(s->fp, "%s %s%c", sign, it->string, 0); } else { struct strbuf onebuf = STRBUF_INIT; const char *one; one = quote_path(it->string, s->prefix, &onebuf); color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign); - printf(" %s\n", one); + fprintf(s->fp, " %s\n", one); strbuf_release(&onebuf); } } -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers() 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren 2020-09-27 13:15 ` [PATCH v2 1/7] wt-status: replace sha1 mentions with oid Martin Ågren 2020-09-27 13:15 ` [PATCH v2 2/7] wt-status: print to s->fp, not stdout Martin Ågren @ 2020-09-27 13:15 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller Martin Ågren ` (3 subsequent siblings) 6 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau When we have a `struct wt_status_state`, we manually free its `branch`, `onto` and `detached_from`, or sometimes just one or two of them. Provide a function `wt_status_state_free_buffers()` which does the freeing. The callers are still aware of these fields, e.g., they check whether `branch` was populated or not. But this way, they don't need to know about *all* of them, and if `struct wt_status_state` gets more fields, they will not need to learn to free them. Users of `struct wt_status` (which contains a `wt_status_state`) already have `wt_status_collect_free_buffers()` (corresponding to `wt_status_collect()`) which we can also teach to use this new helper. Finally, note that we're currently leaving dangling pointers behind. Some callers work on a stack-allocated struct, where this is obviously ok. But for the users of `run_status()` in builtin/commit.c, there are ample opportunities for someone to mistakenly use those dangling pointers. We seem to be ok for now, but it's a use-after-free waiting to happen. Let's leave NULL-pointers behind instead. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- wt-status.h | 7 +++++++ ref-filter.c | 4 +--- worktree.c | 5 ++--- wt-status.c | 11 ++++++++--- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/wt-status.h b/wt-status.h index f1fa0ec1a7..35b44c388e 100644 --- a/wt-status.h +++ b/wt-status.h @@ -151,7 +151,14 @@ void wt_status_add_cut_line(FILE *fp); void wt_status_prepare(struct repository *r, struct wt_status *s); void wt_status_print(struct wt_status *s); void wt_status_collect(struct wt_status *s); +/* + * Frees the buffers allocated by wt_status_collect. + */ void wt_status_collect_free_buffers(struct wt_status *s); +/* + * Frees the buffers of the wt_status_state. + */ +void wt_status_state_free_buffers(struct wt_status_state *s); void wt_status_get_state(struct repository *repo, struct wt_status_state *state, int get_detached_from); diff --git a/ref-filter.c b/ref-filter.c index 8447cb09be..3f63bb01de 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1502,9 +1502,7 @@ char *get_head_description(void) strbuf_addstr(&desc, _("no branch")); strbuf_addch(&desc, ')'); - free(state.branch); - free(state.onto); - free(state.detached_from); + wt_status_state_free_buffers(&state); return strbuf_detach(&desc, NULL); } diff --git a/worktree.c b/worktree.c index cba2e54598..23dd547e44 100644 --- a/worktree.c +++ b/worktree.c @@ -369,8 +369,7 @@ int is_worktree_being_rebased(const struct worktree *wt, state.branch && starts_with(target, "refs/heads/") && !strcmp(state.branch, target + strlen("refs/heads/"))); - free(state.branch); - free(state.onto); + wt_status_state_free_buffers(&state); return found_rebase; } @@ -385,7 +384,7 @@ int is_worktree_being_bisected(const struct worktree *wt, state.branch && starts_with(target, "refs/heads/") && !strcmp(state.branch, target + strlen("refs/heads/")); - free(state.branch); + wt_status_state_free_buffers(&state); return found_rebase; } diff --git a/wt-status.c b/wt-status.c index 3e0b5d8017..5c4cc4805f 100644 --- a/wt-status.c +++ b/wt-status.c @@ -782,9 +782,14 @@ void wt_status_collect(struct wt_status *s) void wt_status_collect_free_buffers(struct wt_status *s) { - free(s->state.branch); - free(s->state.onto); - free(s->state.detached_from); + wt_status_state_free_buffers(&s->state); +} + +void wt_status_state_free_buffers(struct wt_status_state *state) +{ + FREE_AND_NULL(state->branch); + FREE_AND_NULL(state->onto); + FREE_AND_NULL(state->detached_from); } static void wt_longstatus_print_unmerged(struct wt_status *s) -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren ` (2 preceding siblings ...) 2020-09-27 13:15 ` [PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers() Martin Ågren @ 2020-09-27 13:15 ` Martin Ågren 2020-09-28 5:30 ` Eric Sunshine 2020-09-27 13:15 ` [PATCH v2 5/7] worktree: update renamed variable in comment Martin Ågren ` (2 subsequent siblings) 6 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau We have `strbuf_worktree_ref()`, which works on a strbuf, and a wrapper for it, `worktree_ref()` which returns a string. We even make this wrapper available through worktree.h. But it only has a single caller, sitting right next to it in worktree.c. Just inline the wrapper into its only caller. This means the caller can quite naturally reuse a single strbuf. We currently achieve something similar by having a static strbuf in the wrapper. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.h | 7 ------- worktree.c | 17 ++++++----------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/worktree.h b/worktree.h index 516744c433..1449b6bf5d 100644 --- a/worktree.h +++ b/worktree.h @@ -136,11 +136,4 @@ void strbuf_worktree_ref(const struct worktree *wt, struct strbuf *sb, const char *refname); -/* - * Return a refname suitable for access from the current ref - * store. The result will be destroyed at the next call. - */ -const char *worktree_ref(const struct worktree *wt, - const char *refname); - #endif diff --git a/worktree.c b/worktree.c index 23dd547e44..a37d543394 100644 --- a/worktree.c +++ b/worktree.c @@ -548,18 +548,10 @@ void strbuf_worktree_ref(const struct worktree *wt, strbuf_addstr(sb, refname); } -const char *worktree_ref(const struct worktree *wt, const char *refname) -{ - static struct strbuf sb = STRBUF_INIT; - - strbuf_reset(&sb); - strbuf_worktree_ref(wt, &sb, refname); - return sb.buf; -} - int other_head_refs(each_ref_fn fn, void *cb_data) { struct worktree **worktrees, **p; + struct strbuf refname = STRBUF_INIT; int ret = 0; worktrees = get_worktrees(); @@ -571,14 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data) if (wt->is_current) continue; + strbuf_reset(&refname); + strbuf_worktree_ref(wt, &refname, "HEAD"); if (!refs_read_ref_full(get_main_ref_store(the_repository), - worktree_ref(wt, "HEAD"), + refname.buf, RESOLVE_REF_READING, &oid, &flag)) - ret = fn(worktree_ref(wt, "HEAD"), &oid, flag, cb_data); + ret = fn(refname.buf, &oid, flag, cb_data); if (ret) break; } free_worktrees(worktrees); + strbuf_release(&refname); return ret; } -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller 2020-09-27 13:15 ` [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller Martin Ågren @ 2020-09-28 5:30 ` Eric Sunshine 2020-09-28 6:57 ` Martin Ågren 0 siblings, 1 reply; 33+ messages in thread From: Eric Sunshine @ 2020-09-28 5:30 UTC (permalink / raw) To: Martin Ågren; +Cc: Git List, Junio C Hamano, Taylor Blau On Sun, Sep 27, 2020 at 9:16 AM Martin Ågren <martin.agren@gmail.com> wrote: > We have `strbuf_worktree_ref()`, which works on a strbuf, and a wrapper > for it, `worktree_ref()` which returns a string. We even make this > wrapper available through worktree.h. But it only has a single caller, > sitting right next to it in worktree.c. > > Just inline the wrapper into its only caller. This means the caller can > quite naturally reuse a single strbuf. We currently achieve something > similar by having a static strbuf in the wrapper. > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > diff --git a/worktree.c b/worktree.c > @@ -548,18 +548,10 @@ void strbuf_worktree_ref(const struct worktree *wt, > -const char *worktree_ref(const struct worktree *wt, const char *refname) > -{ > - static struct strbuf sb = STRBUF_INIT; > - > - strbuf_reset(&sb); > - strbuf_worktree_ref(wt, &sb, refname); > - return sb.buf; > -} > - > int other_head_refs(each_ref_fn fn, void *cb_data) > { > + struct strbuf refname = STRBUF_INIT; > @@ -571,14 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data) > + strbuf_reset(&refname); If I understand correctly, this strbuf_reset() -- which, I suppose, moved here from the retired worktree_ref() -- is no longer needed now that the strbuf stopped being static. So, this line should be dropped from the patch. > + strbuf_worktree_ref(wt, &refname, "HEAD"); > if (!refs_read_ref_full(get_main_ref_store(the_repository), > - worktree_ref(wt, "HEAD"), > + refname.buf, > RESOLVE_REF_READING, > &oid, &flag)) > - ret = fn(worktree_ref(wt, "HEAD"), &oid, flag, cb_data); > + ret = fn(refname.buf, &oid, flag, cb_data); One wonders why the original made two calls to worktree_ref() with identical arguments. Doing so seems to suggest that something about HEAD might change between the calls, but that doesn't seem to be the case. The message of the commit[1] which introduced the two calls to worktree_ref() doesn't explain the reason, and spelunking through the code doesn't immediately reveal why it was done that way either. So, presumably(?), it is indeed safe to fold them into a single call to strbuf_worktree_ref(). [1]: ab3e1f78ae (revision.c: better error reporting on ref from different worktrees, 2018-10-21) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller 2020-09-28 5:30 ` Eric Sunshine @ 2020-09-28 6:57 ` Martin Ågren 2020-09-28 7:16 ` Eric Sunshine 0 siblings, 1 reply; 33+ messages in thread From: Martin Ågren @ 2020-09-28 6:57 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Taylor Blau On Mon, 28 Sep 2020 at 07:30, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sun, Sep 27, 2020 at 9:16 AM Martin Ågren <martin.agren@gmail.com> wrote: > > We have `strbuf_worktree_ref()`, which works on a strbuf, and a wrapper > > for it, `worktree_ref()` which returns a string. We even make this > > wrapper available through worktree.h. But it only has a single caller, > > sitting right next to it in worktree.c. > > > > Just inline the wrapper into its only caller. This means the caller can > > quite naturally reuse a single strbuf. We currently achieve something > > similar by having a static strbuf in the wrapper. > > > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > > --- > > diff --git a/worktree.c b/worktree.c > > @@ -548,18 +548,10 @@ void strbuf_worktree_ref(const struct worktree *wt, > > -const char *worktree_ref(const struct worktree *wt, const char *refname) > > -{ > > - static struct strbuf sb = STRBUF_INIT; > > - > > - strbuf_reset(&sb); > > - strbuf_worktree_ref(wt, &sb, refname); > > - return sb.buf; > > -} > > - > > int other_head_refs(each_ref_fn fn, void *cb_data) > > { > > + struct strbuf refname = STRBUF_INIT; > > @@ -571,14 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data) > > + strbuf_reset(&refname); > > If I understand correctly, this strbuf_reset() -- which, I suppose, > moved here from the retired worktree_ref() -- is no longer needed now > that the strbuf stopped being static. So, this line should be dropped > from the patch. What's not obvious from the diff is that this happens inside a loop where we go through all worktrees. The strbuf could live one indentation level deeper, in which case we'd continuously initialize and release it. I placed it at the function-level instead, so that we initialize it once and release it once. The "cost" for that is this reset call. So it's sort of the same reset as before this patch, but it's local to this function. This reuse is what I tried to allude to in the second paragraph of the commit message. The original has a single static strbuf, which is an extreme form of reuse: subsequent calls to `other_head_refs()` will "benefit" if you will from the pre-allocated buffer. After this patch, this reuse "only" happens within a call. Subsequent calls will end up doing their own allocations. > > + strbuf_worktree_ref(wt, &refname, "HEAD"); > > if (!refs_read_ref_full(get_main_ref_store(the_repository), > > - worktree_ref(wt, "HEAD"), > > + refname.buf, > > RESOLVE_REF_READING, > > &oid, &flag)) > > - ret = fn(worktree_ref(wt, "HEAD"), &oid, flag, cb_data); > > + ret = fn(refname.buf, &oid, flag, cb_data); > > One wonders why the original made two calls to worktree_ref() with > identical arguments. Doing so seems to suggest that something about > HEAD might change between the calls, but that doesn't seem to be the > case. The message of the commit[1] which introduced the two calls to > worktree_ref() doesn't explain the reason, and spelunking through the > code doesn't immediately reveal why it was done that way either. So, > presumably(?), it is indeed safe to fold them into a single call to > strbuf_worktree_ref(). This matches my understanding. I'd be surprised if reading "HEAD" changes where it points to. And even if it *could* change for some reason, I'd actually expect this "let's call it again" to be a lurking bug that doesn't count on that, rather than a clever caller that chooses to rely on it. It could be that the extra call is because we know we're using a static buffer and we're nervous that the call we made could have ended up invalidating it, so we call it again to be on the safe side. But that seems sort of far-fetched. A better (IMHO) solution to that worry would have been something like what I'm doing here. > [1]: ab3e1f78ae (revision.c: better error reporting on ref from > different worktrees, 2018-10-21) Martin ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller 2020-09-28 6:57 ` Martin Ågren @ 2020-09-28 7:16 ` Eric Sunshine 0 siblings, 0 replies; 33+ messages in thread From: Eric Sunshine @ 2020-09-28 7:16 UTC (permalink / raw) To: Martin Ågren; +Cc: Git List, Junio C Hamano, Taylor Blau On Mon, Sep 28, 2020 at 2:57 AM Martin Ågren <martin.agren@gmail.com> wrote: > On Mon, 28 Sep 2020 at 07:30, Eric Sunshine <sunshine@sunshineco.com> wrote: > > If I understand correctly, this strbuf_reset() -- which, I suppose, > > moved here from the retired worktree_ref() -- is no longer needed now > > that the strbuf stopped being static. So, this line should be dropped > > from the patch. > > What's not obvious from the diff is that this happens inside a loop > where we go through all worktrees. The strbuf could live one indentation > level deeper, in which case we'd continuously initialize and release it. > I placed it at the function-level instead, so that we initialize it > once and release it once. The "cost" for that is this reset call. > > So it's sort of the same reset as before this patch, but it's local to > this function. Yep, ignore my stupid comment. I was reading both the patch and the code yet the reset-in-loop still failed to register. ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 5/7] worktree: update renamed variable in comment 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren ` (3 preceding siblings ...) 2020-09-27 13:15 ` [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller Martin Ågren @ 2020-09-27 13:15 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 6/7] worktree: rename copy-pasted variable Martin Ågren 2020-09-27 13:15 ` [PATCH v2 7/7] worktree: use skip_prefix to parse target Martin Ågren 6 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau The comment above `add_head_info()` mentions "head_sha1", but it was renamed to "head_oid" in 0f05154c70 ("worktree: convert struct worktree to object_id", 2017-10-15). Update the comment. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worktree.c b/worktree.c index a37d543394..5c9ecec466 100644 --- a/worktree.c +++ b/worktree.c @@ -21,7 +21,7 @@ void free_worktrees(struct worktree **worktrees) } /** - * Update head_sha1, head_ref and is_detached of the given worktree + * Update head_oid, head_ref and is_detached of the given worktree */ static void add_head_info(struct worktree *wt) { -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 6/7] worktree: rename copy-pasted variable 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren ` (4 preceding siblings ...) 2020-09-27 13:15 ` [PATCH v2 5/7] worktree: update renamed variable in comment Martin Ågren @ 2020-09-27 13:15 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 7/7] worktree: use skip_prefix to parse target Martin Ågren 6 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau As the commit message of 04a3dfb8b5 ("worktree.c: check whether branch is bisected in another worktree", 2016-04-22) indicates, the function `is_worktree_being_bisected()` is based on the older function `is_worktree_being_rebased()`. This heritage can also be seen in the name of the variable where we store our return value: It was never adapted while copy-editing and remains as `found_rebase`. Rename the variable to make clear that we're looking for a bisect(ion), nothing else. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/worktree.c b/worktree.c index 5c9ecec466..b5702aa4eb 100644 --- a/worktree.c +++ b/worktree.c @@ -377,15 +377,15 @@ int is_worktree_being_bisected(const struct worktree *wt, const char *target) { struct wt_status_state state; - int found_rebase; + int found_bisect; memset(&state, 0, sizeof(state)); - found_rebase = wt_status_check_bisect(wt, &state) && - state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/")); + found_bisect = wt_status_check_bisect(wt, &state) && + state.branch && + starts_with(target, "refs/heads/") && + !strcmp(state.branch, target + strlen("refs/heads/")); wt_status_state_free_buffers(&state); - return found_rebase; + return found_bisect; } /* -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 7/7] worktree: use skip_prefix to parse target 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren ` (5 preceding siblings ...) 2020-09-27 13:15 ` [PATCH v2 6/7] worktree: rename copy-pasted variable Martin Ågren @ 2020-09-27 13:15 ` Martin Ågren 6 siblings, 0 replies; 33+ messages in thread From: Martin Ågren @ 2020-09-27 13:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Sunshine, Taylor Blau Instead of checking for "refs/heads/" using `starts_with()`, then skipping past "refs/heads/" using `strlen()`, just use `skip_prefix()`. In `is_worktree_being_rebased()`, we can adjust the indentation while we're here and lose a pair of parentheses which isn't needed and which might even make the reader wonder what they're missing and why that grouping is there. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- worktree.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/worktree.c b/worktree.c index b5702aa4eb..a75230a950 100644 --- a/worktree.c +++ b/worktree.c @@ -364,11 +364,11 @@ int is_worktree_being_rebased(const struct worktree *wt, memset(&state, 0, sizeof(state)); found_rebase = wt_status_check_rebase(wt, &state) && - ((state.rebase_in_progress || - state.rebase_interactive_in_progress) && - state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/"))); + (state.rebase_in_progress || + state.rebase_interactive_in_progress) && + state.branch && + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); wt_status_state_free_buffers(&state); return found_rebase; } @@ -382,8 +382,8 @@ int is_worktree_being_bisected(const struct worktree *wt, memset(&state, 0, sizeof(state)); found_bisect = wt_status_check_bisect(wt, &state) && state.branch && - starts_with(target, "refs/heads/") && - !strcmp(state.branch, target + strlen("refs/heads/")); + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); wt_status_state_free_buffers(&state); return found_bisect; } -- 2.28.0.277.g9b3c35fffd ^ permalink raw reply related [flat|nested] 33+ messages in thread
end of thread, other threads:[~2020-09-28 7:16 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren 2020-09-10 19:03 ` [PATCH 1/8] wt-status: replace sha1 mentions with oid Martin Ågren 2020-09-10 19:03 ` [PATCH 2/8] wt-status: print to s->fp, not stdout Martin Ågren 2020-09-10 19:03 ` [PATCH 3/8] wt-status: introduce wt_status_state_free_buffers() Martin Ågren 2020-09-10 19:03 ` [PATCH 4/8] worktree: drop useless call to strbuf_reset Martin Ågren 2020-09-10 19:15 ` Eric Sunshine 2020-09-10 19:39 ` Martin Ågren 2020-09-10 19:49 ` Eric Sunshine 2020-09-12 14:02 ` Martin Ågren 2020-09-10 19:03 ` [PATCH 5/8] worktree: update renamed variable in comment Martin Ågren 2020-09-10 19:03 ` [PATCH 6/8] worktree: rename copy-pasted variable Martin Ågren 2020-09-10 20:29 ` Junio C Hamano 2020-09-12 14:01 ` Martin Ågren 2020-09-27 13:29 ` Martin Ågren 2020-09-10 19:03 ` [PATCH 7/8] worktree: use skip_prefix to parse target Martin Ågren 2020-09-10 19:03 ` [PATCH 8/8] worktree: simplify search for unique worktree Martin Ågren 2020-09-10 19:28 ` Eric Sunshine 2020-09-10 19:48 ` Martin Ågren 2020-09-10 20:01 ` Eric Sunshine 2020-09-10 21:08 ` Junio C Hamano 2020-09-12 3:49 ` [PATCH 0/8] various wt-status/worktree cleanups Taylor Blau 2020-09-12 14:03 ` Martin Ågren 2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren 2020-09-27 13:15 ` [PATCH v2 1/7] wt-status: replace sha1 mentions with oid Martin Ågren 2020-09-27 13:15 ` [PATCH v2 2/7] wt-status: print to s->fp, not stdout Martin Ågren 2020-09-27 13:15 ` [PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers() Martin Ågren 2020-09-27 13:15 ` [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller Martin Ågren 2020-09-28 5:30 ` Eric Sunshine 2020-09-28 6:57 ` Martin Ågren 2020-09-28 7:16 ` Eric Sunshine 2020-09-27 13:15 ` [PATCH v2 5/7] worktree: update renamed variable in comment Martin Ågren 2020-09-27 13:15 ` [PATCH v2 6/7] worktree: rename copy-pasted variable Martin Ågren 2020-09-27 13:15 ` [PATCH v2 7/7] worktree: use skip_prefix to parse target Martin Ågren
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.