On Thu, 10 Sep 2020 at 21:15, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 10, 2020 at 3:08 PM Martin Ågren <martin.agren@xxxxxxxxx> wrote:
> > There's no need to reset the strbuf immediately after initializing it.
> >
> > Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
> > ---
> > 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
