git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller
Date: Mon, 28 Sep 2020 08:57:33 +0200	[thread overview]
Message-ID: <CAN0heSr0mZDpjJysvzeG-et3uxpzDWcP3AdPVxiE+KYeY-7nQA@mail.gmail.com> (raw)
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:
>
> 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.

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

  reply	other threads:[~2020-09-28  6:57 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren
2020-09-10 19:03 ` [PATCH 1/8] wt-status: replace sha1 mentions with oid Martin Ågren
2020-09-10 19:03 ` [PATCH 2/8] wt-status: print to s->fp, not stdout Martin Ågren
2020-09-10 19:03 ` [PATCH 3/8] wt-status: introduce wt_status_state_free_buffers() Martin Ågren
2020-09-10 19:03 ` [PATCH 4/8] worktree: drop useless call to strbuf_reset Martin Ågren
2020-09-10 19:15   ` Eric Sunshine
2020-09-10 19:39     ` Martin Ågren
2020-09-10 19:49       ` Eric Sunshine
2020-09-12 14:02         ` Martin Ågren
2020-09-10 19:03 ` [PATCH 5/8] worktree: update renamed variable in comment Martin Ågren
2020-09-10 19:03 ` [PATCH 6/8] worktree: rename copy-pasted variable Martin Ågren
2020-09-10 20:29   ` Junio C Hamano
2020-09-12 14:01     ` Martin Ågren
2020-09-27 13:29       ` Martin Ågren
2020-09-10 19:03 ` [PATCH 7/8] worktree: use skip_prefix to parse target Martin Ågren
2020-09-10 19:03 ` [PATCH 8/8] worktree: simplify search for unique worktree Martin Ågren
2020-09-10 19:28   ` Eric Sunshine
2020-09-10 19:48     ` Martin Ågren
2020-09-10 20:01       ` Eric Sunshine
2020-09-10 21:08         ` Junio C Hamano
2020-09-12  3:49 ` [PATCH 0/8] various wt-status/worktree cleanups Taylor Blau
2020-09-12 14:03   ` Martin Ågren
2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren
2020-09-27 13:15   ` [PATCH v2 1/7] wt-status: replace sha1 mentions with oid Martin Ågren
2020-09-27 13:15   ` [PATCH v2 2/7] wt-status: print to s->fp, not stdout Martin Ågren
2020-09-27 13:15   ` [PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers() Martin Ågren
2020-09-27 13:15   ` [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller Martin Ågren
2020-09-28  5:30     ` Eric Sunshine
2020-09-28  6:57       ` Martin Ågren [this message]
2020-09-28  7:16         ` Eric Sunshine
2020-09-27 13:15   ` [PATCH v2 5/7] worktree: update renamed variable in comment Martin Ågren
2020-09-27 13:15   ` [PATCH v2 6/7] worktree: rename copy-pasted variable Martin Ågren
2020-09-27 13:15   ` [PATCH v2 7/7] worktree: use skip_prefix to parse target Martin Ågren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAN0heSr0mZDpjJysvzeG-et3uxpzDWcP3AdPVxiE+KYeY-7nQA@mail.gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).