[PATCH 0/8] various wt-status/worktree cleanups

[PATCH 0/8] various wt-status/worktree cleanups

[Martin Ågren]

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

[PATCH 1/8] wt-status: replace sha1 mentions with oid

[Martin Ågren]

`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

[PATCH 2/8] wt-status: print to s->fp, not stdout

[Martin Ågren]

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

[PATCH 3/8] wt-status: introduce wt_status_state_free_buffers()

[Martin Ågren]

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

[PATCH 4/8] worktree: drop useless call to strbuf_reset

[Martin Ågren]

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

[PATCH 5/8] worktree: update renamed variable in comment

[Martin Ågren]

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

[PATCH 6/8] worktree: rename copy-pasted variable

[Martin Ågren]

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

[PATCH 7/8] worktree: use skip_prefix to parse target

[Martin Ågren]

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

[PATCH 8/8] worktree: simplify search for unique worktree

[Martin Ågren]

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

Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset

[Eric Sunshine]

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.

Re: [PATCH 8/8] worktree: simplify search for unique worktree

[Eric Sunshine]

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. */

Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset

[Martin Ågren]

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

Re: [PATCH 8/8] worktree: simplify search for unique worktree

[Martin Ågren]

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

Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset

[Eric Sunshine]

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.

Re: [PATCH 8/8] worktree: simplify search for unique worktree

[Eric Sunshine]

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.

Re: [PATCH 6/8] worktree: rename copy-pasted variable

[Junio C Hamano]

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?

Re: [PATCH 8/8] worktree: simplify search for unique worktree

[Junio C Hamano]

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.

Re: [PATCH 0/8] various wt-status/worktree cleanups

[Taylor Blau]

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

Re: [PATCH 6/8] worktree: rename copy-pasted variable

[Martin Ågren]

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

Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset

[Martin Ågren]

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

Re: [PATCH 0/8] various wt-status/worktree cleanups

[Martin Ågren]

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

[PATCH v2 0/7] various wt-status/worktree cleanups

[Martin Ågren]

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

[PATCH v2 1/7] wt-status: replace sha1 mentions with oid

[Martin Ågren]

`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

[PATCH v2 2/7] wt-status: print to s->fp, not stdout

[Martin Ågren]

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

[PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers()

[Martin Ågren]

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

[PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller

[Martin Ågren]

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

[PATCH v2 5/7] worktree: update renamed variable in comment

[Martin Ågren]

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

[PATCH v2 6/7] worktree: rename copy-pasted variable

[Martin Ågren]

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

[PATCH v2 7/7] worktree: use skip_prefix to parse target

[Martin Ågren]

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

Re: [PATCH 6/8] worktree: rename copy-pasted variable

[Martin Ågren]

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

Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller

[Eric Sunshine]

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)

Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller

[Martin Ågren]

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

Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller

[Eric Sunshine]

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.