git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / 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>
Subject: Re: [PATCH 4/8] worktree: drop useless call to strbuf_reset
Date: Thu, 10 Sep 2020 21:39:56 +0200
Message-ID: <CAN0heSrQT9N3=e70qkgS_rOQ0oy0rrHqud=rRtr-r5JaL=ofNQ@mail.gmail.com> (raw)
In-Reply-To: <CAPig+cTZLqFayp0wZEFYkaXtoOx8HedUK1oQoOa+zq=Yrgvjbg@mail.gmail.com>

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

  reply	other threads:[~2020-09-10 19:40 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 [this message]
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
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='CAN0heSrQT9N3=e70qkgS_rOQ0oy0rrHqud=rRtr-r5JaL=ofNQ@mail.gmail.com' \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git