{"thread":{"id":"54222","subject":"[PATCH 6/8] worktree: rename copy-pasted variable","startedAt":"2020-09-10T19:05:23Z","lastAt":"2020-09-28T07:16:39Z","messageCount":33,"participants":["Martin Ågren","Eric Sunshine","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"405353","messageId":"6d913ea3e0c30cc1dbcff05974b5d990797e8dc2.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 6/8] worktree: rename copy-pasted variable","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:40Z","receivedAt":"2020-09-10T19:05:23Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"As the commit message of 04a3dfb8b5 (\"worktree.c: check whether branch\nis bisected in another worktree\", 2016-04-22) indicates, the function\n`is_worktree_being_bisected()` is based on the older function\n`is_worktree_being_rebased()`. This heritage can also be seen in the\nname of the variable where we store our return value: It was never\nadapted while copy-editing and remains as `found_rebase`.\n\nRename the variable to make clear that we're looking for a bisect(ion),\nnothing else.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 050f22dd65..a2d0d20564 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -377,15 +377,15 @@ int is_worktree_being_bisected(const struct worktree *wt,\n \t\t\t const char *target)\n {\n \tstruct wt_status_state state;\n-\tint found_rebase;\n+\tint found_bisect;\n \n \tmemset(&state, 0, sizeof(state));\n-\tfound_rebase = wt_status_check_bisect(wt, &state) &&\n-\t\tstate.branch &&\n-\t\tstarts_with(target, \"refs/heads/\") &&\n-\t\t!strcmp(state.branch, target + strlen(\"refs/heads/\"));\n+\tfound_bisect = wt_status_check_bisect(wt, &state) &&\n+\t\t state.branch &&\n+\t\t starts_with(target, \"refs/heads/\") &&\n+\t\t !strcmp(state.branch, target + strlen(\"refs/heads/\"));\n \twt_status_state_free_buffers(&state);\n-\treturn found_rebase;\n+\treturn found_bisect;\n }\n \n /*\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405354","messageId":"872dc384c53dac61717639cdeccb44a08d5f024d.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 7/8] worktree: use skip_prefix to parse target","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:41Z","receivedAt":"2020-09-10T19:05:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of checking for \"refs/heads/\" using `starts_with()`, then\nskipping past \"refs/heads/\" using `strlen()`, just use `skip_prefix()`.\n\nIn `is_worktree_being_rebased()`, we can adjust the indentation while\nwe're here and lose a pair of parentheses which isn't needed and which\nmight even make the reader wonder what they're missing and why that\ngrouping is there.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex a2d0d20564..faac87422c 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -364,11 +364,11 @@ int is_worktree_being_rebased(const struct worktree *wt,\n \n \tmemset(&state, 0, sizeof(state));\n \tfound_rebase = wt_status_check_rebase(wt, &state) &&\n-\t\t((state.rebase_in_progress ||\n-\t\t state.rebase_interactive_in_progress) &&\n-\t\t state.branch &&\n-\t\t starts_with(target, \"refs/heads/\") &&\n-\t\t !strcmp(state.branch, target + strlen(\"refs/heads/\")));\n+\t\t (state.rebase_in_progress ||\n+\t\t\tstate.rebase_interactive_in_progress) &&\n+\t\t state.branch &&\n+\t\t skip_prefix(target, \"refs/heads/\", &target) &&\n+\t\t !strcmp(state.branch, target);\n \twt_status_state_free_buffers(&state);\n \treturn found_rebase;\n }\n@@ -382,8 +382,8 @@ int is_worktree_being_bisected(const struct worktree *wt,\n \tmemset(&state, 0, sizeof(state));\n \tfound_bisect = wt_status_check_bisect(wt, &state) &&\n \t\t state.branch &&\n-\t\t starts_with(target, \"refs/heads/\") &&\n-\t\t !strcmp(state.branch, target + strlen(\"refs/heads/\"));\n+\t\t skip_prefix(target, \"refs/heads/\", &target) &&\n+\t\t !strcmp(state.branch, target);\n \twt_status_state_free_buffers(&state);\n \treturn found_bisect;\n }\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405355","messageId":"cover.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":null,"subject":"[PATCH 0/8] various wt-status/worktree cleanups","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:34Z","receivedAt":"2020-09-10T19:06:21Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Junio noticed that wt-status.c is a bit inconsistent about printing to\n`s->fp` vs `stdout`. That's patch two. The third patch tries to\nrobustify some freeing of memory. While working on that one, I ended up\nin the sibling file worktree.c.\n\nThis merges pretty well with `seen` (`jc/quote-path-cleanup`) (and the\ntests still pass). These patches are obviously low priority, nothing\nrevolutionary here.\n\nMartin Ågren (8):\n wt-status: replace sha1 mentions with oid\n wt-status: print to s->fp, not stdout\n wt-status: introduce wt_status_state_free_buffers()\n worktree: drop useless call to strbuf_reset\n worktree: update renamed variable in comment\n worktree: rename copy-pasted variable\n worktree: use skip_prefix to parse target\n worktree: simplify search for unique worktree\n\n wt-status.h | 7 +++++\n ref-filter.c | 4 +--\n worktree.c | 39 +++++++++++++--------------\n wt-status.c | 76 ++++++++++++++++++++++++++++++----------------------\n 4 files changed, 71 insertions(+), 55 deletions(-)\n\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405356","messageId":"734ea28effdcb8650ab5e6ac2aa5398400faf1c5.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 1/8] wt-status: replace sha1 mentions with oid","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:35Z","receivedAt":"2020-09-10T19:06:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`abbrev_sha1_in_line()` uses a `struct object_id oid` and should be\nfully prepared to handle non-SHA1 object ids. Rename it to\n`abbrev_oid_in_line()`.\n\nA few comments in `wt_status_get_detached_from()` mention \"sha1\". The\nvariable they refer to was renamed in e86ab2c1cd (\"wt-status: convert to\nstruct object_id\", 2017-02-21). Update the comments to reference \"oid\"\ninstead.\n\nSigned-off-by: Martin Ågren \n---\n wt-status.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex c560cbe860..59be457015 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1232,7 +1232,7 @@ static int split_commit_in_progress(struct wt_status *s)\n * The function assumes that the line does not contain useless spaces\n * before or after the command.\n */\n-static void abbrev_sha1_in_line(struct strbuf *line)\n+static void abbrev_oid_in_line(struct strbuf *line)\n {\n \tstruct strbuf **split;\n \tint i;\n@@ -1282,7 +1282,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)\n \t\tstrbuf_trim(&line);\n \t\tif (!line.len)\n \t\t\tcontinue;\n-\t\tabbrev_sha1_in_line(&line);\n+\t\tabbrev_oid_in_line(&line);\n \t\tstring_list_append(lines, line.buf);\n \t}\n \tfclose(f);\n@@ -1575,9 +1575,9 @@ static void wt_status_get_detached_from(struct repository *r,\n \t}\n \n \tif (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&\n-\t /* sha1 is a commit? match without further lookup */\n+\t /* oid is a commit? match without further lookup */\n \t (oideq(&cb.noid, &oid) ||\n-\t /* perhaps sha1 is a tag, try to dereference to a commit */\n+\t /* perhaps oid is a tag, try to dereference to a commit */\n \t ((commit = lookup_commit_reference_gently(r, &oid, 1)) != NULL &&\n \t oideq(&cb.noid, &commit->object.oid)))) {\n \t\tconst char *from = ref;\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405357","messageId":"7ada884d7c61b4ae6fd002d93a5ac982a63071b7.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 2/8] wt-status: print to s->fp, not stdout","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:36Z","receivedAt":"2020-09-10T19:06:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We pass around a `FILE *` in the `struct wt_status` and almost always\nprint to it. But in a few places, we write to `stdout` instead, either\nexplicitly through `fprintf(stdout, ...)` or implicitly with\n`printf(...)` (and a few `putchar(...)`).\n\nAlways be explicit about writing to `s->fp`. To the best of my\nunderstanding, this never mattered in practice because these spots are\ninvolved in various forms of `git status` which always end up at\nstandard output anyway. When we do write to another file, it's because\nwe're creating a commit message template, and these code paths aren't\ninvolved.\n\nBut let's be consistent to help future readers and avoid future bugs.\n\nSigned-off-by: Martin Ågren \n---\n wt-status.c | 57 ++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 32 insertions(+), 25 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 59be457015..3e0b5d8017 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1806,29 +1806,36 @@ static void wt_longstatus_print(struct wt_status *s)\n \t\t\t; /* nothing */\n \t\telse if (s->workdir_dirty) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"no changes added to commit \"\n-\t\t\t\t\t \"(use \\\"git add\\\" and/or \\\"git commit -a\\\")\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"no changes added to commit \"\n+\t\t\t\t\t\t \"(use \\\"git add\\\" and/or \"\n+\t\t\t\t\t\t \"\\\"git commit -a\\\")\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"no changes added to commit\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"no changes added to \"\n+\t\t\t\t\t\t \"commit\\n\"));\n \t\t} else if (s->untracked.nr) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"nothing added to commit but untracked files \"\n-\t\t\t\t\t \"present (use \\\"git add\\\" to track)\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing added to commit but \"\n+\t\t\t\t\t\t \"untracked files present (use \"\n+\t\t\t\t\t\t \"\\\"git add\\\" to track)\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"nothing added to commit but untracked files present\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing added to commit but \"\n+\t\t\t\t\t\t \"untracked files present\\n\"));\n \t\t} else if (s->is_initial) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"nothing to commit (create/copy files \"\n-\t\t\t\t\t \"and use \\\"git add\\\" to track)\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit (create/\"\n+\t\t\t\t\t\t \"copy files and use \\\"git \"\n+\t\t\t\t\t\t \"add\\\" to track)\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"nothing to commit\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit\\n\"));\n \t\t} else if (!s->show_untracked_files) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"nothing to commit (use -u to show untracked files)\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit (use -u to \"\n+\t\t\t\t\t\t \"show untracked files)\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"nothing to commit\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit\\n\"));\n \t\t} else\n-\t\t\tprintf(_(\"nothing to commit, working tree clean\\n\"));\n+\t\t\tfprintf(s->fp, _(\"nothing to commit, working tree \"\n+\t\t\t\t\t \"clean\\n\"));\n \t}\n \tif(s->show_stash)\n \t\twt_longstatus_print_stash_summary(s);\n@@ -1851,12 +1858,12 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,\n \t}\n \tcolor_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), \"%s\", how);\n \tif (s->null_termination) {\n-\t\tfprintf(stdout, \" %s%c\", it->string, 0);\n+\t\tfprintf(s->fp, \" %s%c\", it->string, 0);\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n \t\tone = quote_path(it->string, s->prefix, &onebuf);\n-\t\tprintf(\" %s\\n\", one);\n+\t\tfprintf(s->fp, \" %s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n@@ -1869,16 +1876,16 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \tif (d->index_status)\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_UPDATED, s), \"%c\", d->index_status);\n \telse\n-\t\tputchar(' ');\n+\t\tfputc(' ', s->fp);\n \tif (d->worktree_status)\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_CHANGED, s), \"%c\", d->worktree_status);\n \telse\n-\t\tputchar(' ');\n-\tputchar(' ');\n+\t\tfputc(' ', s->fp);\n+\tfputc(' ', s->fp);\n \tif (s->null_termination) {\n-\t\tfprintf(stdout, \"%s%c\", it->string, 0);\n+\t\tfprintf(s->fp, \"%s%c\", it->string, 0);\n \t\tif (d->rename_source)\n-\t\t\tfprintf(stdout, \"%s%c\", d->rename_source, 0);\n+\t\t\tfprintf(s->fp, \"%s%c\", d->rename_source, 0);\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n@@ -1886,20 +1893,20 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \t\tif (d->rename_source) {\n \t\t\tone = quote_path(d->rename_source, s->prefix, &onebuf);\n \t\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\t\tputchar('\"');\n+\t\t\t\tfputc('\"', s->fp);\n \t\t\t\tstrbuf_addch(&onebuf, '\"');\n \t\t\t\tone = onebuf.buf;\n \t\t\t}\n-\t\t\tprintf(\"%s -> \", one);\n+\t\t\tfprintf(s->fp, \"%s -> \", one);\n \t\t\tstrbuf_release(&onebuf);\n \t\t}\n \t\tone = quote_path(it->string, s->prefix, &onebuf);\n \t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\tputchar('\"');\n+\t\t\tfputc('\"', s->fp);\n \t\t\tstrbuf_addch(&onebuf, '\"');\n \t\t\tone = onebuf.buf;\n \t\t}\n-\t\tprintf(\"%s\\n\", one);\n+\t\tfprintf(s->fp, \"%s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n@@ -1908,13 +1915,13 @@ static void wt_shortstatus_other(struct string_list_item *it,\n \t\t\t\t struct wt_status *s, const char *sign)\n {\n \tif (s->null_termination) {\n-\t\tfprintf(stdout, \"%s %s%c\", sign, it->string, 0);\n+\t\tfprintf(s->fp, \"%s %s%c\", sign, it->string, 0);\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n \t\tone = quote_path(it->string, s->prefix, &onebuf);\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), \"%s\", sign);\n-\t\tprintf(\" %s\\n\", one);\n+\t\tfprintf(s->fp, \" %s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405358","messageId":"c08fed5a01dabcd02a0f0f5dfaf0f87ce3824ca1.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 3/8] wt-status: introduce wt_status_state_free_buffers()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:37Z","receivedAt":"2020-09-10T19:07:01Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When we have a `struct wt_status_state`, we manually free its `branch`,\n`onto` and `detached_from`, or sometimes just one or two of them.\nProvide a function `wt_status_state_free_buffers()` which does the\nfreeing.\n\nThe callers are still aware of these fields, e.g., they check whether\n`branch` was populated or not. But this way, they don't need to know\nabout *all* of them, and if `struct wt_status_state` gets more fields,\nthey will not need to learn to free them.\n\nUsers of `struct wt_status` (which contains a `wt_status_state`) already\nhave `wt_status_collect_free_buffers()` (corresponding to\n`wt_status_collect()`) which we can also teach to use this new helper.\n\nFinally, note that we're currently leaving dangling pointers behind.\nSome callers work on a stack-allocated struct, where this is obviously\nok. But for the users of `run_status()` in builtin/commit.c, there are\nample opportunities for someone to mistakenly use those dangling\npointers. We seem to be ok for now, but it's a use-after-free waiting to\nhappen. Let's leave NULL-pointers behind instead.\n\nSigned-off-by: Martin Ågren \n---\n wt-status.h | 7 +++++++\n ref-filter.c | 4 +---\n worktree.c | 5 ++---\n wt-status.c | 11 ++++++++---\n 4 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/wt-status.h b/wt-status.h\nindex f1fa0ec1a7..35b44c388e 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -151,7 +151,14 @@ void wt_status_add_cut_line(FILE *fp);\n void wt_status_prepare(struct repository *r, struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n void wt_status_collect(struct wt_status *s);\n+/*\n+ * Frees the buffers allocated by wt_status_collect.\n+ */\n void wt_status_collect_free_buffers(struct wt_status *s);\n+/*\n+ * Frees the buffers of the wt_status_state.\n+ */\n+void wt_status_state_free_buffers(struct wt_status_state *s);\n void wt_status_get_state(struct repository *repo,\n \t\t\t struct wt_status_state *state,\n \t\t\t int get_detached_from);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8447cb09be..3f63bb01de 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1502,9 +1502,7 @@ char *get_head_description(void)\n \t\tstrbuf_addstr(&desc, _(\"no branch\"));\n \tstrbuf_addch(&desc, ')');\n \n-\tfree(state.branch);\n-\tfree(state.onto);\n-\tfree(state.detached_from);\n+\twt_status_state_free_buffers(&state);\n \treturn strbuf_detach(&desc, NULL);\n }\n \ndiff --git a/worktree.c b/worktree.c\nindex cba2e54598..23dd547e44 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -369,8 +369,7 @@ int is_worktree_being_rebased(const struct worktree *wt,\n \t\t state.branch &&\n \t\t starts_with(target, \"refs/heads/\") &&\n \t\t !strcmp(state.branch, target + strlen(\"refs/heads/\")));\n-\tfree(state.branch);\n-\tfree(state.onto);\n+\twt_status_state_free_buffers(&state);\n \treturn found_rebase;\n }\n \n@@ -385,7 +384,7 @@ int is_worktree_being_bisected(const struct worktree *wt,\n \t\tstate.branch &&\n \t\tstarts_with(target, \"refs/heads/\") &&\n \t\t!strcmp(state.branch, target + strlen(\"refs/heads/\"));\n-\tfree(state.branch);\n+\twt_status_state_free_buffers(&state);\n \treturn found_rebase;\n }\n \ndiff --git a/wt-status.c b/wt-status.c\nindex 3e0b5d8017..5c4cc4805f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -782,9 +782,14 @@ void wt_status_collect(struct wt_status *s)\n \n void wt_status_collect_free_buffers(struct wt_status *s)\n {\n-\tfree(s->state.branch);\n-\tfree(s->state.onto);\n-\tfree(s->state.detached_from);\n+\twt_status_state_free_buffers(&s->state);\n+}\n+\n+void wt_status_state_free_buffers(struct wt_status_state *state)\n+{\n+\tFREE_AND_NULL(state->branch);\n+\tFREE_AND_NULL(state->onto);\n+\tFREE_AND_NULL(state->detached_from);\n }\n \n static void wt_longstatus_print_unmerged(struct wt_status *s)\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405359","messageId":"8383c246f8c23e61dedd69d6e69c72d51fd6b469.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 8/8] worktree: simplify search for unique worktree","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:42Z","receivedAt":"2020-09-10T19:07:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We track the number of worktrees we've found and break out of the loop\nearly once we hit 2. This is because we're not really interested in the\nnumber of matches -- we just want to make sure that we don't find more\nthan one worktree that matches the suffix. This can be done just as well\nby checking the NULL-ness of the pointer where we collect our\nanswer-to-be. Drop the redundant `nr_found` variable.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex faac87422c..ac754b88ff 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -172,13 +172,13 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list,\n \t\t\t\t\t\tconst char *suffix)\n {\n \tstruct worktree *found = NULL;\n-\tint nr_found = 0, suffixlen;\n+\tint suffixlen;\n \n \tsuffixlen = strlen(suffix);\n \tif (!suffixlen)\n \t\treturn NULL;\n \n-\tfor (; *list && nr_found < 2; list++) {\n+\tfor (; *list; list++) {\n \t\tconst char\t*path\t = (*list)->path;\n \t\tint\t\t pathlen = strlen(path);\n \t\tint\t\t start\t = pathlen - suffixlen;\n@@ -186,11 +186,12 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list,\n \t\t/* suffix must start at directory boundary */\n \t\tif ((!start || (start > 0 && is_dir_sep(path[start - 1]))) &&\n \t\t !fspathcmp(suffix, path + start)) {\n+\t\t\tif (found)\n+\t\t\t\treturn NULL;\n \t\t\tfound = *list;\n-\t\t\tnr_found++;\n \t\t}\n \t}\n-\treturn nr_found == 1 ? found : NULL;\n+\treturn found;\n }\n \n struct worktree *find_worktree(struct worktree **list,\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405360","messageId":"58a2469cc18839e57b45f687b6e484d69161a34c.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 4/8] worktree: drop useless call to strbuf_reset","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:38Z","receivedAt":"2020-09-10T19:08:19Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"There's no need to reset the strbuf immediately after initializing it.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 23dd547e44..64a9e78997 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -552,7 +552,6 @@ const char *worktree_ref(const struct worktree *wt, const char *refname)\n {\n \tstatic struct strbuf sb = STRBUF_INIT;\n \n-\tstrbuf_reset(&sb);\n \tstrbuf_worktree_ref(wt, &sb, refname);\n \treturn sb.buf;\n }\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405361","messageId":"9c1e321fbc281414e5d4202e83790ae08dce11da.1599762679.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH 5/8] worktree: update renamed variable in comment","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:03:39Z","receivedAt":"2020-09-10T19:08:21Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The comment above `add_head_info()` mentions \"head_sha1\", but it was\nrenamed to \"head_oid\" in 0f05154c70 (\"worktree: convert struct worktree\nto object_id\", 2017-10-15). Update the comment.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 64a9e78997..050f22dd65 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -21,7 +21,7 @@ void free_worktrees(struct worktree **worktrees)\n }\n \n /**\n- * Update head_sha1, head_ref and is_detached of the given worktree\n+ * Update head_oid, head_ref and is_detached of the given worktree\n */\n static void add_head_info(struct worktree *wt)\n {\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"405363","messageId":"CAPig+cTZLqFayp0wZEFYkaXtoOx8HedUK1oQoOa+zq=Yrgvjbg@mail.gmail.com","threadId":"54222","inReplyTo":"58a2469cc18839e57b45f687b6e484d69161a34c.1599762679.git.martin.agren@gmail.com","subject":"Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-10T19:15:09Z","receivedAt":"2020-09-10T19:17:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren wrote:\n> There's no need to reset the strbuf immediately after initializing it.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> diff --git a/worktree.c b/worktree.c\n> @@ -552,7 +552,6 @@ const char *worktree_ref(const struct worktree *wt, const char *refname)\n> {\n> static struct strbuf sb = STRBUF_INIT;\n>\n> - strbuf_reset(&sb);\n> strbuf_worktree_ref(wt, &sb, refname);\n> return sb.buf;\n> }\n\nI think this patch is wrong and should be dropped. That strbuf is\nstatic, and the called strbuf_worktree_ref() does not reset it, so\neach call to worktree_ref() will now merely append to the existing\ncontent (which is undesirable) following this change.\n"},{"id":"405367","messageId":"CAPig+cT_VvOiKBCC=E_P0R8SXkoWPVUxxOOLovOmr8N377YNdQ@mail.gmail.com","threadId":"54222","inReplyTo":"8383c246f8c23e61dedd69d6e69c72d51fd6b469.1599762679.git.martin.agren@gmail.com","subject":"Re: [PATCH 8/8] worktree: simplify search for unique worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-10T19:28:41Z","receivedAt":"2020-09-10T19:34:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren wrote:\n> We track the number of worktrees we've found and break out of the loop\n> early once we hit 2. This is because we're not really interested in the\n> number of matches -- we just want to make sure that we don't find more\n> than one worktree that matches the suffix. This can be done just as well\n> by checking the NULL-ness of the pointer where we collect our\n> answer-to-be. Drop the redundant `nr_found` variable.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> diff --git a/worktree.c b/worktree.c\n> @@ -172,13 +172,13 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list,\n> const char *suffix)\n> {\n> - for (; *list && nr_found < 2; list++) {\n> + for (; *list; list++) {\n> @@ -186,11 +186,12 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list,\n> if ((!start || (start > 0 && is_dir_sep(path[start - 1]))) &&\n> !fspathcmp(suffix, path + start)) {\n> + if (found)\n> + return NULL;\n> found = *list;\n> - nr_found++;\n> }\n> }\n> - return nr_found == 1 ? found : NULL;\n> + return found;\n\nAlthough this change appears to be correct and does simplify the code,\nI think it also makes it a bit more opaque. With the explicit\n`nr_found == 1`, it is quite obvious that the function considers\n\"success\" to be when one and only one entry is found and any other\nnumber is failure. But with the revised code, it is harder to work out\nprecisely what the conditions are. Having said that, I think a simple\ncomment before the function would suffice to clear up the opaqueness.\nPerhaps something like:\n\n /* If exactly one worktree matches 'target', return it, else NULL. */\n"},{"id":"405369","messageId":"CAN0heSrQT9N3=e70qkgS_rOQ0oy0rrHqud=rRtr-r5JaL=ofNQ@mail.gmail.com","threadId":"54222","inReplyTo":"CAPig+cTZLqFayp0wZEFYkaXtoOx8HedUK1oQoOa+zq=Yrgvjbg@mail.gmail.com","subject":"Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:39:56Z","receivedAt":"2020-09-10T19:40:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 10 Sep 2020 at 21:15, Eric Sunshine wrote:\n>\n> On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren wrote:\n> > There's no need to reset the strbuf immediately after initializing it.\n> >\n> > Signed-off-by: Martin Ågren \n> > ---\n> > diff --git a/worktree.c b/worktree.c\n> > @@ -552,7 +552,6 @@ const char *worktree_ref(const struct worktree *wt, const char *refname)\n> > {\n> > static struct strbuf sb = STRBUF_INIT;\n> >\n> > - strbuf_reset(&sb);\n> > strbuf_worktree_ref(wt, &sb, refname);\n> > return sb.buf;\n> > }\n>\n> I think this patch is wrong and should be dropped. That strbuf is\n> static, and the called strbuf_worktree_ref() does not reset it, so\n> each call to worktree_ref() will now merely append to the existing\n> content (which is undesirable) following this change.\n\nOh wow, that's embarrassing. Thank you so much for spotting.\n\nI wonder how many worktrees you need before this optimization to avoid\ncontinuous allocation starts paying off. I guess our test runs never\nactually hit this. Unless the tests are loose enough that my bug manages\nto go unnoticed even if it actually triggers during a test run.\n\nThat's not to say this optimization won't ever be useful, of course. I\nalso begin to hope that no caller keeps their returned pointer around\nfor long. It only seems to be used from `other_ref_heads()` and that\nlooks ok. If we do want this strbuf reuse, maybe that function could\njust keep its own strbuf and reuse it (not necessarily having it be\nstatic) and learn not to call `worktree_ref(wt, \"HEAD\")` twice.\n\nBut anyway, this patch should definitely be dropped.\n\nThanks\nMartin\n"},{"id":"405373","messageId":"CAN0heSrUWiZ_xar3G5rZG-c=8OVp5-eByS6rMXOw9wfTA8kmbA@mail.gmail.com","threadId":"54222","inReplyTo":"CAPig+cT_VvOiKBCC=E_P0R8SXkoWPVUxxOOLovOmr8N377YNdQ@mail.gmail.com","subject":"Re: [PATCH 8/8] worktree: simplify search for unique worktree","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-10T19:48:13Z","receivedAt":"2020-09-10T19:48:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 10 Sep 2020 at 21:28, Eric Sunshine wrote:\n>\n> On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren wrote:\n> > We track the number of worktrees we've found and break out of the loop\n> > early once we hit 2. This is because we're not really interested in the\n> > number of matches -- we just want to make sure that we don't find more\n> > than one worktree that matches the suffix. This can be done just as well\n> > by checking the NULL-ness of the pointer where we collect our\n> > answer-to-be. Drop the redundant `nr_found` variable.\n> >\n> > Signed-off-by: Martin Ågren \n> > ---\n> > diff --git a/worktree.c b/worktree.c\n> > @@ -172,13 +172,13 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list,\n> > const char *suffix)\n> > {\n> > - for (; *list && nr_found < 2; list++) {\n> > + for (; *list; list++) {\n> > @@ -186,11 +186,12 @@ static struct worktree *find_worktree_by_suffix(struct worktree **list,\n> > if ((!start || (start > 0 && is_dir_sep(path[start - 1]))) &&\n> > !fspathcmp(suffix, path + start)) {\n> > + if (found)\n> > + return NULL;\n> > found = *list;\n> > - nr_found++;\n> > }\n> > }\n> > - return nr_found == 1 ? found : NULL;\n> > + return found;\n>\n> Although this change appears to be correct and does simplify the code,\n> I think it also makes it a bit more opaque. With the explicit\n> `nr_found == 1`, it is quite obvious that the function considers\n> \"success\" to be when one and only one entry is found and any other\n> number is failure. But with the revised code, it is harder to work out\n> precisely what the conditions are.\n\nThanks for commenting. I found the original trickier than it had to be.\nIt spreads out the logic in several places and is careful to short-cut\nthe loop. My first thought was \"why doesn't this just use the standard\nform?\". But I'm open to the idea that it might be a fairly personal\nstandard form... If there's any risk that someone else comes along and\nsimplifies this to use a `nr_found` variable, then maybe file this under\ncode churning?\n\n> Having said that, I think a simple\n> comment before the function would suffice to clear up the opaqueness.\n> Perhaps something like:\n>\n> /* If exactly one worktree matches 'target', return it, else NULL. */\n\nThat's a good suggestion regardless.\n\nThanks\nMartin\n"},{"id":"405374","messageId":"CAPig+cSEOUebNV90Gec2KqskZgAmX=jd++s4tfAyYN6Up4_O3A@mail.gmail.com","threadId":"54222","inReplyTo":"CAN0heSrQT9N3=e70qkgS_rOQ0oy0rrHqud=rRtr-r5JaL=ofNQ@mail.gmail.com","subject":"Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-10T19:49:07Z","receivedAt":"2020-09-10T19:51:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 10, 2020 at 3:40 PM Martin Ågren wrote:\n> On Thu, 10 Sep 2020 at 21:15, Eric Sunshine wrote:\n> > I think this patch is wrong and should be dropped. That strbuf is\n> > static, and the called strbuf_worktree_ref() does not reset it, so\n> > each call to worktree_ref() will now merely append to the existing\n> > content (which is undesirable) following this change.\n>\n> That's not to say this optimization won't ever be useful, of course. I\n> also begin to hope that no caller keeps their returned pointer around\n> for long. It only seems to be used from `other_ref_heads()` and that\n> looks ok. If we do want this strbuf reuse, maybe that function could\n> just keep its own strbuf and reuse it (not necessarily having it be\n> static) and learn not to call `worktree_ref(wt, \"HEAD\")` twice.\n\nYep, I wouldn't be unhappy to see worktree_ref() disappear altogether.\nThere are no external callers and it would be easy enough to retrofit\nthe lone internal caller to use the safer strbuf_worktree_ref()\nanyhow. Plus, both calls to worktree_ref() in other_head_refs() invoke\nit with the exact same arguments, `worktree_ref(wt, \"HEAD\")`, which\nmakes one wonder if it need be called twice at all in that particular\nscenario.\n"},{"id":"405375","messageId":"CAPig+cQ871p8+oQdBBY=ebDdjDWfa5NvEMroitEmk4Db8DfLvA@mail.gmail.com","threadId":"54222","inReplyTo":"CAN0heSrUWiZ_xar3G5rZG-c=8OVp5-eByS6rMXOw9wfTA8kmbA@mail.gmail.com","subject":"Re: [PATCH 8/8] worktree: simplify search for unique worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-10T20:01:57Z","receivedAt":"2020-09-10T20:03:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 10, 2020 at 3:48 PM Martin Ågren wrote:\n> On Thu, 10 Sep 2020 at 21:28, Eric Sunshine wrote:\n> > Although this change appears to be correct and does simplify the code,\n> > I think it also makes it a bit more opaque. With the explicit\n> > `nr_found == 1`, it is quite obvious that the function considers\n> > \"success\" to be when one and only one entry is found and any other\n> > number is failure. But with the revised code, it is harder to work out\n> > precisely what the conditions are.\n>\n> Thanks for commenting. I found the original trickier than it had to be.\n> It spreads out the logic in several places and is careful to short-cut\n> the loop. My first thought was \"why doesn't this just use the standard\n> form?\". But I'm open to the idea that it might be a fairly personal\n> standard form... If there's any risk that someone else comes along and\n> simplifies this to use a `nr_found` variable, then maybe file this under\n> code churning?\n\nMaybe. Dunno. Even with the suggested function comment, I still find\nthat the revised code has a higher cognitive load because the reader\nhas to remember or figure out mentally what `found` contains by the\n`return found;` at the end of the function. Compare that with the old\ncode, in which the reader doesn't have to remember or figure out\nanything. The final `return nr_found == 1 ? found : NULL;` condition\nspells out everything the reader needs to know -- even if the reader\ndidn't pay close attention to the meat of the function. So, we each\nhave a different take on the apparent complexity.\n\n> > Having said that, I think a simple\n> > comment before the function would suffice to clear up the opaqueness.\n> > Perhaps something like:\n> >\n> > /* If exactly one worktree matches 'target', return it, else NULL. */\n>\n> That's a good suggestion regardless.\n\nThe function is so small that the increased cognitive load (for me) in\nthe rewrite probably doesn't matter at all, so I don't feel strongly\none way or the other. Keeping the patch (amended with the suggested\ncomment) or dropping it are both suitable options.\n"},{"id":"405376","messageId":"xmqq7dt1pe8r.fsf@gitster.c.googlers.com","threadId":"54222","inReplyTo":"6d913ea3e0c30cc1dbcff05974b5d990797e8dc2.1599762679.git.martin.agren@gmail.com","subject":"Re: [PATCH 6/8] worktree: rename copy-pasted variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-10T20:29:24Z","receivedAt":"2020-09-10T20:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> As the commit message of 04a3dfb8b5 (\"worktree.c: check whether branch\n> is bisected in another worktree\", 2016-04-22) indicates, the function\n> `is_worktree_being_bisected()` is based on the older function\n> `is_worktree_being_rebased()`. This heritage can also be seen in the\n> name of the variable where we store our return value: It was never\n> adapted while copy-editing and remains as `found_rebase`.\n>\n> Rename the variable to make clear that we're looking for a bisect(ion),\n> nothing else.\n\nHow bad is this copy and paste? Is it a possibility to make a\nsingle helper function and these existing two a thin wrapper around\nthe helper that passes customization between bisect and rebase?\n\n"},{"id":"405380","messageId":"xmqq363ppcg7.fsf@gitster.c.googlers.com","threadId":"54222","inReplyTo":"CAPig+cQ871p8+oQdBBY=ebDdjDWfa5NvEMroitEmk4Db8DfLvA@mail.gmail.com","subject":"Re: [PATCH 8/8] worktree: simplify search for unique worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-10T21:08:08Z","receivedAt":"2020-09-10T21:10:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Thanks for commenting. I found the original trickier than it had to be.\n>> It spreads out the logic in several places and is careful to short-cut\n>> the loop. My first thought was \"why doesn't this just use the standard\n>> form?\". But I'm open to the idea that it might be a fairly personal\n>> standard form... If there's any risk that someone else comes along and\n>> simplifies this to use a `nr_found` variable, then maybe file this under\n>> code churning?\n>\n> Maybe. Dunno. Even with the suggested function comment, I still find\n> that the revised code has a higher cognitive load because the reader\n> has to remember or figure out mentally what `found` contains by the\n> `return found;` at the end of the function.\n\nJudging from the phrase \"standard form\", I have a feeling that\nMartin thinks that the updated code is more intuitive (i.e. \"we see\nanother one while keeping the one we already found, so we know there\nis no unique answer\"). I do not claim that would be the standard way\nand using a counter clamped to 2 is substandard, but I find the code\nmore readable with the patch than the original.\n\nEven though it helps somewhat that the counter is named \"nr_found\"\nand not \"nr_match\", I found it a bit awkward that the counter in the\noriginal pretends to count all the answers, yet does not really\ncount all of them and stops at 2.\n\nI agree with Martin that this would probably be subjective.\n\nThanks.\n\n\n"},{"id":"405465","messageId":"20200912034901.GA4101@nand.local","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/8] various wt-status/worktree cleanups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-12T03:49:01Z","receivedAt":"2020-09-12T03:49:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 10, 2020 at 09:03:34PM +0200, Martin Ågren wrote:\n> This merges pretty well with `seen` (`jc/quote-path-cleanup`) (and the\n> tests still pass). These patches are obviously low priority, nothing\n> revolutionary here.\n\nWhat revision is this series based on, again? Applying the first patch\non top of 'seen' (bf3e2864f3, at the time of writing) produces a\nconflict due to the dropped parameter in 'dwim_ref()'.\n\nBut, applying in directly on top of Junio's 'jc/quote-path-cleanup' from\nhis repository handles the first patch just fine, but fails in the\nsecond patch. The conflicts here are in 'wt_shortstatus_unmerged' and\n'wt_shortstatus_status'.\n\nI didn't look deeply to figure out what exactly was going on here, but\nit would be good to know so that I can play with these patches a bit\nmyself.\n\nThanks,\nTaylor\n"},{"id":"405468","messageId":"CAN0heSrFQFRPnXzp-UT6_jrwvVh+4EQMdOvkJaQCQxeg8GUG4g@mail.gmail.com","threadId":"54222","inReplyTo":"xmqq7dt1pe8r.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 6/8] worktree: rename copy-pasted variable","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-12T14:01:22Z","receivedAt":"2020-09-12T14:01:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 10 Sep 2020 at 22:29, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > As the commit message of 04a3dfb8b5 (\"worktree.c: check whether branch\n> > is bisected in another worktree\", 2016-04-22) indicates, the function\n> > `is_worktree_being_bisected()` is based on the older function\n> > `is_worktree_being_rebased()`. This heritage can also be seen in the\n> > name of the variable where we store our return value: It was never\n> > adapted while copy-editing and remains as `found_rebase`.\n> >\n> > Rename the variable to make clear that we're looking for a bisect(ion),\n> > nothing else.\n>\n> How bad is this copy and paste? Is it a possibility to make a\n> single helper function and these existing two a thin wrapper around\n> the helper that passes customization between bisect and rebase?\n\nThat's a good point. I'll look into it.\n\nThanks\nMartin\n"},{"id":"405469","messageId":"CAN0heSo92MWy=o8aQ22i0dHa1bgTuWo9Sc=X3bqvLxquiJsREA@mail.gmail.com","threadId":"54222","inReplyTo":"CAPig+cSEOUebNV90Gec2KqskZgAmX=jd++s4tfAyYN6Up4_O3A@mail.gmail.com","subject":"Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-12T14:02:13Z","receivedAt":"2020-09-12T14:02:43Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 10 Sep 2020 at 21:49, Eric Sunshine wrote:\n>\n> On Thu, Sep 10, 2020 at 3:40 PM Martin Ågren wrote:\n> > That's not to say this optimization won't ever be useful, of course. I\n> > also begin to hope that no caller keeps their returned pointer around\n> > for long. It only seems to be used from `other_ref_heads()` and that\n> > looks ok. If we do want this strbuf reuse, maybe that function could\n> > just keep its own strbuf and reuse it (not necessarily having it be\n> > static) and learn not to call `worktree_ref(wt, \"HEAD\")` twice.\n>\n> Yep, I wouldn't be unhappy to see worktree_ref() disappear altogether.\n> There are no external callers and it would be easy enough to retrofit\n> the lone internal caller to use the safer strbuf_worktree_ref()\n> anyhow. Plus, both calls to worktree_ref() in other_head_refs() invoke\n> it with the exact same arguments, `worktree_ref(wt, \"HEAD\")`, which\n> makes one wonder if it need be called twice at all in that particular\n> scenario.\n\nI'll look at this hopefully tomorrow. Thanks for all your comments.\n\n\nMartin\n"},{"id":"405470","messageId":"CAN0heSp9Lk8nNVR7svGn-vCS_40bvWtChXBsZa9WPzbw9XqNEQ@mail.gmail.com","threadId":"54222","inReplyTo":"20200912034901.GA4101@nand.local","subject":"Re: [PATCH 0/8] various wt-status/worktree cleanups","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-12T14:03:13Z","receivedAt":"2020-09-12T14:04:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 12 Sep 2020 at 05:49, Taylor Blau wrote:\n>\n> On Thu, Sep 10, 2020 at 09:03:34PM +0200, Martin Ågren wrote:\n> > This merges pretty well with `seen` (`jc/quote-path-cleanup`) (and the\n> > tests still pass). These patches are obviously low priority, nothing\n> > revolutionary here.\n>\n> What revision is this series based on, again? [ ... ]\n\nThis is on top of current maint: 47ae905ffb (\"Git 2.28\", 2020-07-26)\n\n> I didn't look deeply to figure out what exactly was going on here, but\n> it would be good to know so that I can play with these patches a bit\n> myself.\n\nOk, beware that there's at least one bug, so you're better off dropping\n\"worktree: drop useless call to strbuf_reset\". I'll probably replace it\nwith something to remove that function and basically inline it into\nits only caller.\n\n\nMartin\n"},{"id":"406487","messageId":"cover.1600281350.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1599762679.git.martin.agren@gmail.com","subject":"[PATCH v2 0/7] various wt-status/worktree cleanups","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:40Z","receivedAt":"2020-09-27T13:16:08Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Here's a belated update of my series to clean up wt-status.c and\nworktree.c a bit.\n\nThe buggy fourth patch has been removed and replaced with one that\ninlines `worktree_ref()` into its only caller. Thanks Eric for spotting\nmy initial bug and for the very helpful discussion.\n\nThe final patch, \"worktree: simplify search for unique worktree\" is no\nlonger. It felt like it was quite subjective whether it was a\n\"simplification\", so let's not spend reviewer quality time on it.\n\nMartin Ågren (7):\n wt-status: replace sha1 mentions with oid\n wt-status: print to s->fp, not stdout\n wt-status: introduce wt_status_state_free_buffers()\n worktree: inline `worktree_ref()` into its only caller\n worktree: update renamed variable in comment\n worktree: rename copy-pasted variable\n worktree: use skip_prefix to parse target\n\n worktree.h | 7 -----\n wt-status.h | 7 +++++\n ref-filter.c | 4 +--\n worktree.c | 46 ++++++++++++++-----------------\n wt-status.c | 76 ++++++++++++++++++++++++++++++----------------------\n 5 files changed, 72 insertions(+), 68 deletions(-)\n\nRange-diff against v1:\n1: 734ea28eff = 1: 734ea28eff wt-status: replace sha1 mentions with oid\n2: 7ada884d7c = 2: 7ada884d7c wt-status: print to s->fp, not stdout\n3: c08fed5a01 = 3: c08fed5a01 wt-status: introduce wt_status_state_free_buffers()\n4: 58a2469cc1 < -: ---------- worktree: drop useless call to strbuf_reset\n-: ---------- > 4: c4825f461e worktree: inline `worktree_ref()` into its only caller\n5: 9c1e321fbc = 5: dbbc57cd83 worktree: update renamed variable in comment\n6: 6d913ea3e0 = 6: 70e6cea331 worktree: rename copy-pasted variable\n7: 872dc384c5 = 7: f8fbee6234 worktree: use skip_prefix to parse target\n8: 8383c246f8 < -: ---------- worktree: simplify search for unique worktree\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406488","messageId":"734ea28effdcb8650ab5e6ac2aa5398400faf1c5.1600281351.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1600281350.git.martin.agren@gmail.com","subject":"[PATCH v2 1/7] wt-status: replace sha1 mentions with oid","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:41Z","receivedAt":"2020-09-27T13:16:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`abbrev_sha1_in_line()` uses a `struct object_id oid` and should be\nfully prepared to handle non-SHA1 object ids. Rename it to\n`abbrev_oid_in_line()`.\n\nA few comments in `wt_status_get_detached_from()` mention \"sha1\". The\nvariable they refer to was renamed in e86ab2c1cd (\"wt-status: convert to\nstruct object_id\", 2017-02-21). Update the comments to reference \"oid\"\ninstead.\n\nSigned-off-by: Martin Ågren \n---\n wt-status.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex c560cbe860..59be457015 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1232,7 +1232,7 @@ static int split_commit_in_progress(struct wt_status *s)\n * The function assumes that the line does not contain useless spaces\n * before or after the command.\n */\n-static void abbrev_sha1_in_line(struct strbuf *line)\n+static void abbrev_oid_in_line(struct strbuf *line)\n {\n \tstruct strbuf **split;\n \tint i;\n@@ -1282,7 +1282,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)\n \t\tstrbuf_trim(&line);\n \t\tif (!line.len)\n \t\t\tcontinue;\n-\t\tabbrev_sha1_in_line(&line);\n+\t\tabbrev_oid_in_line(&line);\n \t\tstring_list_append(lines, line.buf);\n \t}\n \tfclose(f);\n@@ -1575,9 +1575,9 @@ static void wt_status_get_detached_from(struct repository *r,\n \t}\n \n \tif (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&\n-\t /* sha1 is a commit? match without further lookup */\n+\t /* oid is a commit? match without further lookup */\n \t (oideq(&cb.noid, &oid) ||\n-\t /* perhaps sha1 is a tag, try to dereference to a commit */\n+\t /* perhaps oid is a tag, try to dereference to a commit */\n \t ((commit = lookup_commit_reference_gently(r, &oid, 1)) != NULL &&\n \t oideq(&cb.noid, &commit->object.oid)))) {\n \t\tconst char *from = ref;\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406489","messageId":"7ada884d7c61b4ae6fd002d93a5ac982a63071b7.1600281351.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1600281350.git.martin.agren@gmail.com","subject":"[PATCH v2 2/7] wt-status: print to s->fp, not stdout","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:42Z","receivedAt":"2020-09-27T13:16:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We pass around a `FILE *` in the `struct wt_status` and almost always\nprint to it. But in a few places, we write to `stdout` instead, either\nexplicitly through `fprintf(stdout, ...)` or implicitly with\n`printf(...)` (and a few `putchar(...)`).\n\nAlways be explicit about writing to `s->fp`. To the best of my\nunderstanding, this never mattered in practice because these spots are\ninvolved in various forms of `git status` which always end up at\nstandard output anyway. When we do write to another file, it's because\nwe're creating a commit message template, and these code paths aren't\ninvolved.\n\nBut let's be consistent to help future readers and avoid future bugs.\n\nSigned-off-by: Martin Ågren \n---\n wt-status.c | 57 ++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 32 insertions(+), 25 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 59be457015..3e0b5d8017 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1806,29 +1806,36 @@ static void wt_longstatus_print(struct wt_status *s)\n \t\t\t; /* nothing */\n \t\telse if (s->workdir_dirty) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"no changes added to commit \"\n-\t\t\t\t\t \"(use \\\"git add\\\" and/or \\\"git commit -a\\\")\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"no changes added to commit \"\n+\t\t\t\t\t\t \"(use \\\"git add\\\" and/or \"\n+\t\t\t\t\t\t \"\\\"git commit -a\\\")\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"no changes added to commit\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"no changes added to \"\n+\t\t\t\t\t\t \"commit\\n\"));\n \t\t} else if (s->untracked.nr) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"nothing added to commit but untracked files \"\n-\t\t\t\t\t \"present (use \\\"git add\\\" to track)\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing added to commit but \"\n+\t\t\t\t\t\t \"untracked files present (use \"\n+\t\t\t\t\t\t \"\\\"git add\\\" to track)\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"nothing added to commit but untracked files present\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing added to commit but \"\n+\t\t\t\t\t\t \"untracked files present\\n\"));\n \t\t} else if (s->is_initial) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"nothing to commit (create/copy files \"\n-\t\t\t\t\t \"and use \\\"git add\\\" to track)\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit (create/\"\n+\t\t\t\t\t\t \"copy files and use \\\"git \"\n+\t\t\t\t\t\t \"add\\\" to track)\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"nothing to commit\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit\\n\"));\n \t\t} else if (!s->show_untracked_files) {\n \t\t\tif (s->hints)\n-\t\t\t\tprintf(_(\"nothing to commit (use -u to show untracked files)\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit (use -u to \"\n+\t\t\t\t\t\t \"show untracked files)\\n\"));\n \t\t\telse\n-\t\t\t\tprintf(_(\"nothing to commit\\n\"));\n+\t\t\t\tfprintf(s->fp, _(\"nothing to commit\\n\"));\n \t\t} else\n-\t\t\tprintf(_(\"nothing to commit, working tree clean\\n\"));\n+\t\t\tfprintf(s->fp, _(\"nothing to commit, working tree \"\n+\t\t\t\t\t \"clean\\n\"));\n \t}\n \tif(s->show_stash)\n \t\twt_longstatus_print_stash_summary(s);\n@@ -1851,12 +1858,12 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,\n \t}\n \tcolor_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), \"%s\", how);\n \tif (s->null_termination) {\n-\t\tfprintf(stdout, \" %s%c\", it->string, 0);\n+\t\tfprintf(s->fp, \" %s%c\", it->string, 0);\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n \t\tone = quote_path(it->string, s->prefix, &onebuf);\n-\t\tprintf(\" %s\\n\", one);\n+\t\tfprintf(s->fp, \" %s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n@@ -1869,16 +1876,16 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \tif (d->index_status)\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_UPDATED, s), \"%c\", d->index_status);\n \telse\n-\t\tputchar(' ');\n+\t\tfputc(' ', s->fp);\n \tif (d->worktree_status)\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_CHANGED, s), \"%c\", d->worktree_status);\n \telse\n-\t\tputchar(' ');\n-\tputchar(' ');\n+\t\tfputc(' ', s->fp);\n+\tfputc(' ', s->fp);\n \tif (s->null_termination) {\n-\t\tfprintf(stdout, \"%s%c\", it->string, 0);\n+\t\tfprintf(s->fp, \"%s%c\", it->string, 0);\n \t\tif (d->rename_source)\n-\t\t\tfprintf(stdout, \"%s%c\", d->rename_source, 0);\n+\t\t\tfprintf(s->fp, \"%s%c\", d->rename_source, 0);\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n@@ -1886,20 +1893,20 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \t\tif (d->rename_source) {\n \t\t\tone = quote_path(d->rename_source, s->prefix, &onebuf);\n \t\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\t\tputchar('\"');\n+\t\t\t\tfputc('\"', s->fp);\n \t\t\t\tstrbuf_addch(&onebuf, '\"');\n \t\t\t\tone = onebuf.buf;\n \t\t\t}\n-\t\t\tprintf(\"%s -> \", one);\n+\t\t\tfprintf(s->fp, \"%s -> \", one);\n \t\t\tstrbuf_release(&onebuf);\n \t\t}\n \t\tone = quote_path(it->string, s->prefix, &onebuf);\n \t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\tputchar('\"');\n+\t\t\tfputc('\"', s->fp);\n \t\t\tstrbuf_addch(&onebuf, '\"');\n \t\t\tone = onebuf.buf;\n \t\t}\n-\t\tprintf(\"%s\\n\", one);\n+\t\tfprintf(s->fp, \"%s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n@@ -1908,13 +1915,13 @@ static void wt_shortstatus_other(struct string_list_item *it,\n \t\t\t\t struct wt_status *s, const char *sign)\n {\n \tif (s->null_termination) {\n-\t\tfprintf(stdout, \"%s %s%c\", sign, it->string, 0);\n+\t\tfprintf(s->fp, \"%s %s%c\", sign, it->string, 0);\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n \t\tone = quote_path(it->string, s->prefix, &onebuf);\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), \"%s\", sign);\n-\t\tprintf(\" %s\\n\", one);\n+\t\tfprintf(s->fp, \" %s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406490","messageId":"c4825f461e0408970f2adb272098bd6f2a80ef78.1600281351.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1600281350.git.martin.agren@gmail.com","subject":"[PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:44Z","receivedAt":"2020-09-27T13:16:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We have `strbuf_worktree_ref()`, which works on a strbuf, and a wrapper\nfor it, `worktree_ref()` which returns a string. We even make this\nwrapper available through worktree.h. But it only has a single caller,\nsitting right next to it in worktree.c.\n\nJust inline the wrapper into its only caller. This means the caller can\nquite naturally reuse a single strbuf. We currently achieve something\nsimilar by having a static strbuf in the wrapper.\n\nSigned-off-by: Martin Ågren \n---\n worktree.h | 7 -------\n worktree.c | 17 ++++++-----------\n 2 files changed, 6 insertions(+), 18 deletions(-)\n\ndiff --git a/worktree.h b/worktree.h\nindex 516744c433..1449b6bf5d 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -136,11 +136,4 @@ void strbuf_worktree_ref(const struct worktree *wt,\n \t\t\t struct strbuf *sb,\n \t\t\t const char *refname);\n \n-/*\n- * Return a refname suitable for access from the current ref\n- * store. The result will be destroyed at the next call.\n- */\n-const char *worktree_ref(const struct worktree *wt,\n-\t\t\t const char *refname);\n-\n #endif\ndiff --git a/worktree.c b/worktree.c\nindex 23dd547e44..a37d543394 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -548,18 +548,10 @@ void strbuf_worktree_ref(const struct worktree *wt,\n \tstrbuf_addstr(sb, refname);\n }\n \n-const char *worktree_ref(const struct worktree *wt, const char *refname)\n-{\n-\tstatic struct strbuf sb = STRBUF_INIT;\n-\n-\tstrbuf_reset(&sb);\n-\tstrbuf_worktree_ref(wt, &sb, refname);\n-\treturn sb.buf;\n-}\n-\n int other_head_refs(each_ref_fn fn, void *cb_data)\n {\n \tstruct worktree **worktrees, **p;\n+\tstruct strbuf refname = STRBUF_INIT;\n \tint ret = 0;\n \n \tworktrees = get_worktrees();\n@@ -571,14 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n \t\tif (wt->is_current)\n \t\t\tcontinue;\n \n+\t\tstrbuf_reset(&refname);\n+\t\tstrbuf_worktree_ref(wt, &refname, \"HEAD\");\n \t\tif (!refs_read_ref_full(get_main_ref_store(the_repository),\n-\t\t\t\t\tworktree_ref(wt, \"HEAD\"),\n+\t\t\t\t\trefname.buf,\n \t\t\t\t\tRESOLVE_REF_READING,\n \t\t\t\t\t&oid, &flag))\n-\t\t\tret = fn(worktree_ref(wt, \"HEAD\"), &oid, flag, cb_data);\n+\t\t\tret = fn(refname.buf, &oid, flag, cb_data);\n \t\tif (ret)\n \t\t\tbreak;\n \t}\n \tfree_worktrees(worktrees);\n+\tstrbuf_release(&refname);\n \treturn ret;\n }\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406491","messageId":"c08fed5a01dabcd02a0f0f5dfaf0f87ce3824ca1.1600281351.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1600281350.git.martin.agren@gmail.com","subject":"[PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:43Z","receivedAt":"2020-09-27T13:16:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When we have a `struct wt_status_state`, we manually free its `branch`,\n`onto` and `detached_from`, or sometimes just one or two of them.\nProvide a function `wt_status_state_free_buffers()` which does the\nfreeing.\n\nThe callers are still aware of these fields, e.g., they check whether\n`branch` was populated or not. But this way, they don't need to know\nabout *all* of them, and if `struct wt_status_state` gets more fields,\nthey will not need to learn to free them.\n\nUsers of `struct wt_status` (which contains a `wt_status_state`) already\nhave `wt_status_collect_free_buffers()` (corresponding to\n`wt_status_collect()`) which we can also teach to use this new helper.\n\nFinally, note that we're currently leaving dangling pointers behind.\nSome callers work on a stack-allocated struct, where this is obviously\nok. But for the users of `run_status()` in builtin/commit.c, there are\nample opportunities for someone to mistakenly use those dangling\npointers. We seem to be ok for now, but it's a use-after-free waiting to\nhappen. Let's leave NULL-pointers behind instead.\n\nSigned-off-by: Martin Ågren \n---\n wt-status.h | 7 +++++++\n ref-filter.c | 4 +---\n worktree.c | 5 ++---\n wt-status.c | 11 ++++++++---\n 4 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/wt-status.h b/wt-status.h\nindex f1fa0ec1a7..35b44c388e 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -151,7 +151,14 @@ void wt_status_add_cut_line(FILE *fp);\n void wt_status_prepare(struct repository *r, struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n void wt_status_collect(struct wt_status *s);\n+/*\n+ * Frees the buffers allocated by wt_status_collect.\n+ */\n void wt_status_collect_free_buffers(struct wt_status *s);\n+/*\n+ * Frees the buffers of the wt_status_state.\n+ */\n+void wt_status_state_free_buffers(struct wt_status_state *s);\n void wt_status_get_state(struct repository *repo,\n \t\t\t struct wt_status_state *state,\n \t\t\t int get_detached_from);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8447cb09be..3f63bb01de 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1502,9 +1502,7 @@ char *get_head_description(void)\n \t\tstrbuf_addstr(&desc, _(\"no branch\"));\n \tstrbuf_addch(&desc, ')');\n \n-\tfree(state.branch);\n-\tfree(state.onto);\n-\tfree(state.detached_from);\n+\twt_status_state_free_buffers(&state);\n \treturn strbuf_detach(&desc, NULL);\n }\n \ndiff --git a/worktree.c b/worktree.c\nindex cba2e54598..23dd547e44 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -369,8 +369,7 @@ int is_worktree_being_rebased(const struct worktree *wt,\n \t\t state.branch &&\n \t\t starts_with(target, \"refs/heads/\") &&\n \t\t !strcmp(state.branch, target + strlen(\"refs/heads/\")));\n-\tfree(state.branch);\n-\tfree(state.onto);\n+\twt_status_state_free_buffers(&state);\n \treturn found_rebase;\n }\n \n@@ -385,7 +384,7 @@ int is_worktree_being_bisected(const struct worktree *wt,\n \t\tstate.branch &&\n \t\tstarts_with(target, \"refs/heads/\") &&\n \t\t!strcmp(state.branch, target + strlen(\"refs/heads/\"));\n-\tfree(state.branch);\n+\twt_status_state_free_buffers(&state);\n \treturn found_rebase;\n }\n \ndiff --git a/wt-status.c b/wt-status.c\nindex 3e0b5d8017..5c4cc4805f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -782,9 +782,14 @@ void wt_status_collect(struct wt_status *s)\n \n void wt_status_collect_free_buffers(struct wt_status *s)\n {\n-\tfree(s->state.branch);\n-\tfree(s->state.onto);\n-\tfree(s->state.detached_from);\n+\twt_status_state_free_buffers(&s->state);\n+}\n+\n+void wt_status_state_free_buffers(struct wt_status_state *state)\n+{\n+\tFREE_AND_NULL(state->branch);\n+\tFREE_AND_NULL(state->onto);\n+\tFREE_AND_NULL(state->detached_from);\n }\n \n static void wt_longstatus_print_unmerged(struct wt_status *s)\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406492","messageId":"dbbc57cd833f95a59ef82ecac6046a43cd88cc31.1600281351.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1600281350.git.martin.agren@gmail.com","subject":"[PATCH v2 5/7] worktree: update renamed variable in comment","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:45Z","receivedAt":"2020-09-27T13:16:21Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The comment above `add_head_info()` mentions \"head_sha1\", but it was\nrenamed to \"head_oid\" in 0f05154c70 (\"worktree: convert struct worktree\nto object_id\", 2017-10-15). Update the comment.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex a37d543394..5c9ecec466 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -21,7 +21,7 @@ void free_worktrees(struct worktree **worktrees)\n }\n \n /**\n- * Update head_sha1, head_ref and is_detached of the given worktree\n+ * Update head_oid, head_ref and is_detached of the given worktree\n */\n static void add_head_info(struct worktree *wt)\n {\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406493","messageId":"70e6cea3312bf0a587e6614ae06fb5fab17163ba.1600281351.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1600281350.git.martin.agren@gmail.com","subject":"[PATCH v2 6/7] worktree: rename copy-pasted variable","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:46Z","receivedAt":"2020-09-27T13:16:25Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"As the commit message of 04a3dfb8b5 (\"worktree.c: check whether branch\nis bisected in another worktree\", 2016-04-22) indicates, the function\n`is_worktree_being_bisected()` is based on the older function\n`is_worktree_being_rebased()`. This heritage can also be seen in the\nname of the variable where we store our return value: It was never\nadapted while copy-editing and remains as `found_rebase`.\n\nRename the variable to make clear that we're looking for a bisect(ion),\nnothing else.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 5c9ecec466..b5702aa4eb 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -377,15 +377,15 @@ int is_worktree_being_bisected(const struct worktree *wt,\n \t\t\t const char *target)\n {\n \tstruct wt_status_state state;\n-\tint found_rebase;\n+\tint found_bisect;\n \n \tmemset(&state, 0, sizeof(state));\n-\tfound_rebase = wt_status_check_bisect(wt, &state) &&\n-\t\tstate.branch &&\n-\t\tstarts_with(target, \"refs/heads/\") &&\n-\t\t!strcmp(state.branch, target + strlen(\"refs/heads/\"));\n+\tfound_bisect = wt_status_check_bisect(wt, &state) &&\n+\t\t state.branch &&\n+\t\t starts_with(target, \"refs/heads/\") &&\n+\t\t !strcmp(state.branch, target + strlen(\"refs/heads/\"));\n \twt_status_state_free_buffers(&state);\n-\treturn found_rebase;\n+\treturn found_bisect;\n }\n \n /*\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406494","messageId":"f8fbee62347f4b19cdf12612e5bfc82ae1cea453.1600281351.git.martin.agren@gmail.com","threadId":"54222","inReplyTo":"cover.1600281350.git.martin.agren@gmail.com","subject":"[PATCH v2 7/7] worktree: use skip_prefix to parse target","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:15:47Z","receivedAt":"2020-09-27T13:16:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of checking for \"refs/heads/\" using `starts_with()`, then\nskipping past \"refs/heads/\" using `strlen()`, just use `skip_prefix()`.\n\nIn `is_worktree_being_rebased()`, we can adjust the indentation while\nwe're here and lose a pair of parentheses which isn't needed and which\nmight even make the reader wonder what they're missing and why that\ngrouping is there.\n\nSigned-off-by: Martin Ågren \n---\n worktree.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex b5702aa4eb..a75230a950 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -364,11 +364,11 @@ int is_worktree_being_rebased(const struct worktree *wt,\n \n \tmemset(&state, 0, sizeof(state));\n \tfound_rebase = wt_status_check_rebase(wt, &state) &&\n-\t\t((state.rebase_in_progress ||\n-\t\t state.rebase_interactive_in_progress) &&\n-\t\t state.branch &&\n-\t\t starts_with(target, \"refs/heads/\") &&\n-\t\t !strcmp(state.branch, target + strlen(\"refs/heads/\")));\n+\t\t (state.rebase_in_progress ||\n+\t\t\tstate.rebase_interactive_in_progress) &&\n+\t\t state.branch &&\n+\t\t skip_prefix(target, \"refs/heads/\", &target) &&\n+\t\t !strcmp(state.branch, target);\n \twt_status_state_free_buffers(&state);\n \treturn found_rebase;\n }\n@@ -382,8 +382,8 @@ int is_worktree_being_bisected(const struct worktree *wt,\n \tmemset(&state, 0, sizeof(state));\n \tfound_bisect = wt_status_check_bisect(wt, &state) &&\n \t\t state.branch &&\n-\t\t starts_with(target, \"refs/heads/\") &&\n-\t\t !strcmp(state.branch, target + strlen(\"refs/heads/\"));\n+\t\t skip_prefix(target, \"refs/heads/\", &target) &&\n+\t\t !strcmp(state.branch, target);\n \twt_status_state_free_buffers(&state);\n \treturn found_bisect;\n }\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406495","messageId":"20200927132910.20515-1-martin.agren@gmail.com","threadId":"54222","inReplyTo":"CAN0heSrFQFRPnXzp-UT6_jrwvVh+4EQMdOvkJaQCQxeg8GUG4g@mail.gmail.com","subject":"Re: [PATCH 6/8] worktree: rename copy-pasted variable","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-27T13:29:10Z","receivedAt":"2020-09-27T13:29:24Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 12 Sep 2020 at 16:01, Martin Ågren wrote:\n>\n> On Thu, 10 Sep 2020 at 22:29, Junio C Hamano wrote:\n> >\n> > Martin Ågren writes:\n> >\n> > > As the commit message of 04a3dfb8b5 (\"worktree.c: check whether branch\n> > > is bisected in another worktree\", 2016-04-22) indicates, the function\n> > > `is_worktree_being_bisected()` is based on the older function\n> > > `is_worktree_being_rebased()`. This heritage can also be seen in the\n> > > name of the variable where we store our return value: It was never\n> > > adapted while copy-editing and remains as `found_rebase`.\n> >\n> > How bad is this copy and paste? Is it a possibility to make a\n> > single helper function and these existing two a thin wrapper around\n> > the helper that passes customization between bisect and rebase?\n>\n> That's a good point. I'll look into it.\n\nI did look into this, and here's what I came up with (this is on top of\nv2 that I just sent out, since that's how I tried it out). If there\nwould be three or four such similar functions, I would feel a lot more\nconfident going with this approach, since it'd be sort of obvious that\nthey were all the same. But for \"only\" two it somehow feels a bit\nbrittle. If any of those two functions need to do something differently,\nwe might need to start shuffling logic between the helper and the\ncallers. Or we'll end up with just a single caller, the other having\nbroken away from the helper.\n\nSo in the end I didn't take the plunge for v2.\n\n(BTW, the naming in this diff is clearly w-i-p grade...)\n\nMartin\n\ndiff --git a/worktree.c b/worktree.c\nindex a75230a950..4009856b54 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -356,36 +356,40 @@ void update_worktree_location(struct worktree *wt, const char *path_)\n \tstrbuf_release(&path);\n }\n \n+static int is_worktree_being_x(int (*check_fn)(const struct worktree *wt,\n+\t\t\t\t\t struct wt_status_state *state),\n+\t\t\t const struct worktree *wt,\n+\t\t\t const char *target)\n+{\n+\tstruct wt_status_state state = { 0 };\n+\tint found;\n+\n+\tfound = check_fn(wt, &state) &&\n+\t\tstate.branch &&\n+\t\tskip_prefix(target, \"refs/heads/\", &target) &&\n+\t\t!strcmp(state.branch, target);\n+\twt_status_state_free_buffers(&state);\n+\treturn found;\n+}\n+\n+static int check_rebase_in_progress(const struct worktree *wt,\n+\t\t\t\t struct wt_status_state *state)\n+{\n+\treturn wt_status_check_rebase(wt, state) &&\n+\t (state->rebase_in_progress ||\n+\t\tstate->rebase_interactive_in_progress);\n+}\n+\n int is_worktree_being_rebased(const struct worktree *wt,\n \t\t\t const char *target)\n {\n-\tstruct wt_status_state state;\n-\tint found_rebase;\n-\n-\tmemset(&state, 0, sizeof(state));\n-\tfound_rebase = wt_status_check_rebase(wt, &state) &&\n-\t\t (state.rebase_in_progress ||\n-\t\t\tstate.rebase_interactive_in_progress) &&\n-\t\t state.branch &&\n-\t\t skip_prefix(target, \"refs/heads/\", &target) &&\n-\t\t !strcmp(state.branch, target);\n-\twt_status_state_free_buffers(&state);\n-\treturn found_rebase;\n+\treturn is_worktree_being_x(check_rebase_in_progress, wt, target);\n }\n \n int is_worktree_being_bisected(const struct worktree *wt,\n \t\t\t const char *target)\n {\n-\tstruct wt_status_state state;\n-\tint found_bisect;\n-\n-\tmemset(&state, 0, sizeof(state));\n-\tfound_bisect = wt_status_check_bisect(wt, &state) &&\n-\t\t state.branch &&\n-\t\t skip_prefix(target, \"refs/heads/\", &target) &&\n-\t\t !strcmp(state.branch, target);\n-\twt_status_state_free_buffers(&state);\n-\treturn found_bisect;\n+\treturn is_worktree_being_x(wt_status_check_bisect, wt, target);\n }\n \n /*\n-- \n2.28.0.277.g9b3c35fffd\n\n"},{"id":"406522","messageId":"CAPig+cSBPFdF=hM5ho9_g6NCGAprh1mFFT5zX-C9huT3-Qkzow@mail.gmail.com","threadId":"54222","inReplyTo":"c4825f461e0408970f2adb272098bd6f2a80ef78.1600281351.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-28T05:30:24Z","receivedAt":"2020-09-28T05:30:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 27, 2020 at 9:16 AM Martin Ågren wrote:\n> We have `strbuf_worktree_ref()`, which works on a strbuf, and a wrapper\n> for it, `worktree_ref()` which returns a string. We even make this\n> wrapper available through worktree.h. But it only has a single caller,\n> sitting right next to it in worktree.c.\n>\n> Just inline the wrapper into its only caller. This means the caller can\n> quite naturally reuse a single strbuf. We currently achieve something\n> similar by having a static strbuf in the wrapper.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> diff --git a/worktree.c b/worktree.c\n> @@ -548,18 +548,10 @@ void strbuf_worktree_ref(const struct worktree *wt,\n> -const char *worktree_ref(const struct worktree *wt, const char *refname)\n> -{\n> - static struct strbuf sb = STRBUF_INIT;\n> -\n> - strbuf_reset(&sb);\n> - strbuf_worktree_ref(wt, &sb, refname);\n> - return sb.buf;\n> -}\n> -\n> int other_head_refs(each_ref_fn fn, void *cb_data)\n> {\n> + struct strbuf refname = STRBUF_INIT;\n> @@ -571,14 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n> + strbuf_reset(&refname);\n\nIf I understand correctly, this strbuf_reset() -- which, I suppose,\nmoved here from the retired worktree_ref() -- is no longer needed now\nthat the strbuf stopped being static. So, this line should be dropped\nfrom the patch.\n\n> + strbuf_worktree_ref(wt, &refname, \"HEAD\");\n> if (!refs_read_ref_full(get_main_ref_store(the_repository),\n> - worktree_ref(wt, \"HEAD\"),\n> + refname.buf,\n> RESOLVE_REF_READING,\n> &oid, &flag))\n> - ret = fn(worktree_ref(wt, \"HEAD\"), &oid, flag, cb_data);\n> + ret = fn(refname.buf, &oid, flag, cb_data);\n\nOne wonders why the original made two calls to worktree_ref() with\nidentical arguments. Doing so seems to suggest that something about\nHEAD might change between the calls, but that doesn't seem to be the\ncase. The message of the commit[1] which introduced the two calls to\nworktree_ref() doesn't explain the reason, and spelunking through the\ncode doesn't immediately reveal why it was done that way either. So,\npresumably(?), it is indeed safe to fold them into a single call to\nstrbuf_worktree_ref().\n\n[1]: ab3e1f78ae (revision.c: better error reporting on ref from\ndifferent worktrees, 2018-10-21)\n"},{"id":"406524","messageId":"CAN0heSr0mZDpjJysvzeG-et3uxpzDWcP3AdPVxiE+KYeY-7nQA@mail.gmail.com","threadId":"54222","inReplyTo":"CAPig+cSBPFdF=hM5ho9_g6NCGAprh1mFFT5zX-C9huT3-Qkzow@mail.gmail.com","subject":"Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-28T06:57:33Z","receivedAt":"2020-09-28T06:57:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Mon, 28 Sep 2020 at 07:30, Eric Sunshine wrote:\n>\n> On Sun, Sep 27, 2020 at 9:16 AM Martin Ågren wrote:\n> > We have `strbuf_worktree_ref()`, which works on a strbuf, and a wrapper\n> > for it, `worktree_ref()` which returns a string. We even make this\n> > wrapper available through worktree.h. But it only has a single caller,\n> > sitting right next to it in worktree.c.\n> >\n> > Just inline the wrapper into its only caller. This means the caller can\n> > quite naturally reuse a single strbuf. We currently achieve something\n> > similar by having a static strbuf in the wrapper.\n> >\n> > Signed-off-by: Martin Ågren \n> > ---\n> > diff --git a/worktree.c b/worktree.c\n> > @@ -548,18 +548,10 @@ void strbuf_worktree_ref(const struct worktree *wt,\n> > -const char *worktree_ref(const struct worktree *wt, const char *refname)\n> > -{\n> > - static struct strbuf sb = STRBUF_INIT;\n> > -\n> > - strbuf_reset(&sb);\n> > - strbuf_worktree_ref(wt, &sb, refname);\n> > - return sb.buf;\n> > -}\n> > -\n> > int other_head_refs(each_ref_fn fn, void *cb_data)\n> > {\n> > + struct strbuf refname = STRBUF_INIT;\n> > @@ -571,14 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n> > + strbuf_reset(&refname);\n>\n> If I understand correctly, this strbuf_reset() -- which, I suppose,\n> moved here from the retired worktree_ref() -- is no longer needed now\n> that the strbuf stopped being static. So, this line should be dropped\n> from the patch.\n\nWhat's not obvious from the diff is that this happens inside a loop\nwhere we go through all worktrees. The strbuf could live one indentation\nlevel deeper, in which case we'd continuously initialize and release it.\nI placed it at the function-level instead, so that we initialize it\nonce and release it once. The \"cost\" for that is this reset call.\n\nSo it's sort of the same reset as before this patch, but it's local to\nthis function.\n\nThis reuse is what I tried to allude to in the second paragraph of the\ncommit message. The original has a single static strbuf, which is an\nextreme form of reuse: subsequent calls to `other_head_refs()` will\n\"benefit\" if you will from the pre-allocated buffer. After this patch,\nthis reuse \"only\" happens within a call. Subsequent calls will end up\ndoing their own allocations.\n\n> > + strbuf_worktree_ref(wt, &refname, \"HEAD\");\n> > if (!refs_read_ref_full(get_main_ref_store(the_repository),\n> > - worktree_ref(wt, \"HEAD\"),\n> > + refname.buf,\n> > RESOLVE_REF_READING,\n> > &oid, &flag))\n> > - ret = fn(worktree_ref(wt, \"HEAD\"), &oid, flag, cb_data);\n> > + ret = fn(refname.buf, &oid, flag, cb_data);\n>\n> One wonders why the original made two calls to worktree_ref() with\n> identical arguments. Doing so seems to suggest that something about\n> HEAD might change between the calls, but that doesn't seem to be the\n> case. The message of the commit[1] which introduced the two calls to\n> worktree_ref() doesn't explain the reason, and spelunking through the\n> code doesn't immediately reveal why it was done that way either. So,\n> presumably(?), it is indeed safe to fold them into a single call to\n> strbuf_worktree_ref().\n\nThis matches my understanding. I'd be surprised if reading \"HEAD\"\nchanges where it points to. And even if it *could* change for some\nreason, I'd actually expect this \"let's call it again\" to be a lurking\nbug that doesn't count on that, rather than a clever caller that chooses\nto rely on it. It could be that the extra call is because we know we're\nusing a static buffer and we're nervous that the call we made could have\nended up invalidating it, so we call it again to be on the safe side.\nBut that seems sort of far-fetched. A better (IMHO) solution to that\nworry would have been something like what I'm doing here.\n\n> [1]: ab3e1f78ae (revision.c: better error reporting on ref from\n> different worktrees, 2018-10-21)\n\nMartin\n"},{"id":"406525","messageId":"CAPig+cS3x1PtNeiMU5a7Dfa8-cCvs4K=LRwELEcb3DQ+ahfeWA@mail.gmail.com","threadId":"54222","inReplyTo":"CAN0heSr0mZDpjJysvzeG-et3uxpzDWcP3AdPVxiE+KYeY-7nQA@mail.gmail.com","subject":"Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-28T07:16:25Z","receivedAt":"2020-09-28T07:16:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 28, 2020 at 2:57 AM Martin Ågren wrote:\n> On Mon, 28 Sep 2020 at 07:30, Eric Sunshine wrote:\n> > If I understand correctly, this strbuf_reset() -- which, I suppose,\n> > moved here from the retired worktree_ref() -- is no longer needed now\n> > that the strbuf stopped being static. So, this line should be dropped\n> > from the patch.\n>\n> What's not obvious from the diff is that this happens inside a loop\n> where we go through all worktrees. The strbuf could live one indentation\n> level deeper, in which case we'd continuously initialize and release it.\n> I placed it at the function-level instead, so that we initialize it\n> once and release it once. The \"cost\" for that is this reset call.\n>\n> So it's sort of the same reset as before this patch, but it's local to\n> this function.\n\nYep, ignore my stupid comment. I was reading both the patch and the\ncode yet the reset-in-loop still failed to register.\n"}]}