[PATCH] sha1_name: simplify strbuf handling in interpret_nth_prior_checkout()

[PATCH] sha1_name: simplify strbuf handling in interpret_nth_prior_checkout()

[René Scharfe]

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(-)

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;
 }

--
2.23.0

Re: [PATCH] sha1_name: simplify strbuf handling in interpret_nth_prior_checkout()

[Derrick Stolee]

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