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

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 10, 2020, 19:39 UTC
Message-ID: <CAN0heSrQT9N3=e70qkgS_rOQ0oy0rrHqud=rRtr-r5JaL=ofNQ@mail.gmail.com>
In-Reply-To: <CAPig+cTZLqFayp0wZEFYkaXtoOx8HedUK1oQoOa+zq=Yrgvjbg@mail.gmail.com>

On Thu, 10 Sep 2020 at 21:15, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 20 quoted lines

>
> 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