diff mbox series

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

Message ID 58a2469cc18839e57b45f687b6e484d69161a34c.1599762679.git.martin.agren@gmail.com
State Superseded
Headers show
Series various wt-status/worktree cleanups | expand

Commit Message

Martin Ågren Sept. 10, 2020, 7:03 p.m. UTC
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(-)

Comments

Eric Sunshine Sept. 10, 2020, 7:15 p.m. UTC | #1
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.
Martin Ågren Sept. 10, 2020, 7:39 p.m. UTC | #2
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
Eric Sunshine Sept. 10, 2020, 7:49 p.m. UTC | #3
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.
Martin Ågren Sept. 12, 2020, 2:02 p.m. UTC | #4
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 mbox series

Patch

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