| Message ID | 582d8659-dd5e-440e-6f00-a59c7017dff6@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sha1_name: simplify strbuf handling in interpret_nth_prior_checkout() | expand |
On 9/18/2019 12:35 PM, René Scharfe wrote: > Pass the target strbuf to the callback function grab_nth_branch_switch() > by reference so that it can add the result string directly instead of > having it put the string into a temporary strbuf first. This gets rid > of an extra allocation and a string copy. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > Patch formatted with --function-context for easier reviewing. I appreciate this. I needed to look through the whole method to see that previously "buf" was used to take the resulting value on a successful parse. > sha1-name.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/sha1-name.c b/sha1-name.c > index c665e3f96d..85196929c7 100644 > --- a/sha1-name.c > +++ b/sha1-name.c > @@ -1286,70 +1286,67 @@ static int get_oid_oneline(struct repository *r, > > struct grab_nth_branch_switch_cbdata { > int remaining; > - struct strbuf buf; > + struct strbuf *sb; > }; > > static int grab_nth_branch_switch(struct object_id *ooid, struct object_id *noid, > const char *email, timestamp_t timestamp, int tz, > const char *message, void *cb_data) > { > struct grab_nth_branch_switch_cbdata *cb = cb_data; > const char *match = NULL, *target = NULL; > size_t len; > > if (skip_prefix(message, "checkout: moving from ", &match)) > target = strstr(match, " to "); > > if (!match || !target) > return 0; > if (--(cb->remaining) == 0) { > len = target - match; > - strbuf_reset(&cb->buf); > - strbuf_add(&cb->buf, match, len); > + strbuf_reset(cb->sb); > + strbuf_add(cb->sb, match, len); > return 1; /* we are done */ It seems that now "buf" is still only being replaced on a successful parse, since this strbuf_reset and strbuf_add is happening only on a return 1, which terminates the refs_for_each... method. > } > return 0; > } > > /* > * Parse @{-N} syntax, return the number of characters parsed > * if successful; otherwise signal an error with negative value. > */ > static int interpret_nth_prior_checkout(struct repository *r, > const char *name, int namelen, > struct strbuf *buf) > { > long nth; > int retval; > struct grab_nth_branch_switch_cbdata cb; > const char *brace; > char *num_end; > > if (namelen < 4) > return -1; > if (name[0] != '@' || name[1] != '{' || name[2] != '-') > return -1; > brace = memchr(name, '}', namelen); > if (!brace) > return -1; > nth = strtol(name + 3, &num_end, 10); > if (num_end != brace) > return -1; > if (nth <= 0) > return -1; > cb.remaining = nth; > - strbuf_init(&cb.buf, 20); > + cb.sb = buf; > > retval = refs_for_each_reflog_ent_reverse(get_main_ref_store(r), > "HEAD", grab_nth_branch_switch, &cb); > if (0 < retval) { > - strbuf_reset(buf); > - strbuf_addbuf(buf, &cb.buf); > retval = brace - name + 1; > } else > retval = 0; > > - strbuf_release(&cb.buf); > return retval; > } > The reaction in this method appears correct, too. Thanks, -Stolee
diff --git a/sha1-name.c b/sha1-name.c index c665e3f96d..85196929c7 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1286,70 +1286,67 @@ static int get_oid_oneline(struct repository *r, struct grab_nth_branch_switch_cbdata { int remaining; - struct strbuf buf; + struct strbuf *sb; }; static int grab_nth_branch_switch(struct object_id *ooid, struct object_id *noid, const char *email, timestamp_t timestamp, int tz, const char *message, void *cb_data) { struct grab_nth_branch_switch_cbdata *cb = cb_data; const char *match = NULL, *target = NULL; size_t len; if (skip_prefix(message, "checkout: moving from ", &match)) target = strstr(match, " to "); if (!match || !target) return 0; if (--(cb->remaining) == 0) { len = target - match; - strbuf_reset(&cb->buf); - strbuf_add(&cb->buf, match, len); + strbuf_reset(cb->sb); + strbuf_add(cb->sb, match, len); return 1; /* we are done */ } return 0; } /* * Parse @{-N} syntax, return the number of characters parsed * if successful; otherwise signal an error with negative value. */ static int interpret_nth_prior_checkout(struct repository *r, const char *name, int namelen, struct strbuf *buf) { long nth; int retval; struct grab_nth_branch_switch_cbdata cb; const char *brace; char *num_end; if (namelen < 4) return -1; if (name[0] != '@' || name[1] != '{' || name[2] != '-') return -1; brace = memchr(name, '}', namelen); if (!brace) return -1; nth = strtol(name + 3, &num_end, 10); if (num_end != brace) return -1; if (nth <= 0) return -1; cb.remaining = nth; - strbuf_init(&cb.buf, 20); + cb.sb = buf; retval = refs_for_each_reflog_ent_reverse(get_main_ref_store(r), "HEAD", grab_nth_branch_switch, &cb); if (0 < retval) { - strbuf_reset(buf); - strbuf_addbuf(buf, &cb.buf); retval = brace - name + 1; } else retval = 0; - strbuf_release(&cb.buf); return retval; }
Pass the target strbuf to the callback function grab_nth_branch_switch() by reference so that it can add the result string directly instead of having it put the string into a temporary strbuf first. This gets rid of an extra allocation and a string copy. Signed-off-by: René Scharfe <l.s.r@web.de> --- Patch formatted with --function-context for easier reviewing. sha1-name.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) -- 2.23.0