On Mon, 28 Sep 2020 at 07:30, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Sun, Sep 27, 2020 at 9:16 AM Martin Ågren <martin.agren@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
> > + 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
