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

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 28, 2020, 06:57 UTC
Message-ID: <CAN0heSr0mZDpjJysvzeG-et3uxpzDWcP3AdPVxiE+KYeY-7nQA@mail.gmail.com>
In-Reply-To: <CAPig+cSBPFdF=hM5ho9_g6NCGAprh1mFFT5zX-C9huT3-Qkzow@mail.gmail.com>

On Mon, 28 Sep 2020 at 07:30, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 34 quoted lines

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

Show 17 quoted lines

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