On Thu, Sep 10, 2020 at 3:40 PM Martin Ågren <martin.agren@xxxxxxxxx> wrote: > On Thu, 10 Sep 2020 at 21:15, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
