| Message ID | c4825f461e0408970f2adb272098bd6f2a80ef78.1600281351.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ef2d5547fa342197befd4be599438d7a7fa41e04 |
| Headers | show |
| Series | various wt-status/worktree cleanups | expand |
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. > + 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(). [1]: ab3e1f78ae (revision.c: better error reporting on ref from different worktrees, 2018-10-21)
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
On Mon, Sep 28, 2020 at 2:57 AM Martin Ågren <martin.agren@gmail.com> wrote: > On Mon, 28 Sep 2020 at 07:30, Eric Sunshine <sunshine@sunshineco.com> wrote: > > 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. Yep, ignore my stupid comment. I was reading both the patch and the code yet the reset-in-loop still failed to register.
diff --git a/worktree.h b/worktree.h index 516744c433..1449b6bf5d 100644 --- a/worktree.h +++ b/worktree.h @@ -136,11 +136,4 @@ void strbuf_worktree_ref(const struct worktree *wt, struct strbuf *sb, const char *refname); -/* - * Return a refname suitable for access from the current ref - * store. The result will be destroyed at the next call. - */ -const char *worktree_ref(const struct worktree *wt, - const char *refname); - #endif diff --git a/worktree.c b/worktree.c index 23dd547e44..a37d543394 100644 --- a/worktree.c +++ b/worktree.c @@ -548,18 +548,10 @@ void strbuf_worktree_ref(const struct worktree *wt, strbuf_addstr(sb, refname); } -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 worktree **worktrees, **p; + struct strbuf refname = STRBUF_INIT; int ret = 0; worktrees = get_worktrees(); @@ -571,14 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data) if (wt->is_current) continue; + strbuf_reset(&refname); + 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); if (ret) break; } free_worktrees(worktrees); + strbuf_release(&refname); return ret; }
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> --- worktree.h | 7 ------- worktree.c | 17 ++++++----------- 2 files changed, 6 insertions(+), 18 deletions(-)