| Message ID | 58a2469cc18839e57b45f687b6e484d69161a34c.1599762679.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | various wt-status/worktree cleanups | expand |
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.
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
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.
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
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; }
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(-)