{"thread":{"id":"51878","subject":"[PATCH] sha1_name: simplify strbuf handling in interpret_nth_prior_checkout()","startedAt":"2019-09-18T16:35:46Z","lastAt":"2019-09-18T20:02:12Z","messageCount":2,"participants":["René Scharfe","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"382558","messageId":"582d8659-dd5e-440e-6f00-a59c7017dff6@web.de","threadId":"51878","inReplyTo":null,"subject":"[PATCH] sha1_name: simplify strbuf handling in interpret_nth_prior_checkout()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-18T16:35:38Z","receivedAt":"2019-09-18T16:35:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Pass the target strbuf to the callback function grab_nth_branch_switch()\nby reference so that it can add the result string directly instead of\nhaving it put the string into a temporary strbuf first.  This gets rid\nof an extra allocation and a string copy.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nPatch formatted with --function-context for easier reviewing.\n\n sha1-name.c | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex c665e3f96d..85196929c7 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1286,70 +1286,67 @@ static int get_oid_oneline(struct repository *r,\n\n struct grab_nth_branch_switch_cbdata {\n \tint remaining;\n-\tstruct strbuf buf;\n+\tstruct strbuf *sb;\n };\n\n static int grab_nth_branch_switch(struct object_id *ooid, struct object_id *noid,\n \t\t\t\t  const char *email, timestamp_t timestamp, int tz,\n \t\t\t\t  const char *message, void *cb_data)\n {\n \tstruct grab_nth_branch_switch_cbdata *cb = cb_data;\n \tconst char *match = NULL, *target = NULL;\n \tsize_t len;\n\n \tif (skip_prefix(message, \"checkout: moving from \", &match))\n \t\ttarget = strstr(match, \" to \");\n\n \tif (!match || !target)\n \t\treturn 0;\n \tif (--(cb->remaining) == 0) {\n \t\tlen = target - match;\n-\t\tstrbuf_reset(&cb->buf);\n-\t\tstrbuf_add(&cb->buf, match, len);\n+\t\tstrbuf_reset(cb->sb);\n+\t\tstrbuf_add(cb->sb, match, len);\n \t\treturn 1; /* we are done */\n \t}\n \treturn 0;\n }\n\n /*\n  * Parse @{-N} syntax, return the number of characters parsed\n  * if successful; otherwise signal an error with negative value.\n  */\n static int interpret_nth_prior_checkout(struct repository *r,\n \t\t\t\t\tconst char *name, int namelen,\n \t\t\t\t\tstruct strbuf *buf)\n {\n \tlong nth;\n \tint retval;\n \tstruct grab_nth_branch_switch_cbdata cb;\n \tconst char *brace;\n \tchar *num_end;\n\n \tif (namelen < 4)\n \t\treturn -1;\n \tif (name[0] != '@' || name[1] != '{' || name[2] != '-')\n \t\treturn -1;\n \tbrace = memchr(name, '}', namelen);\n \tif (!brace)\n \t\treturn -1;\n \tnth = strtol(name + 3, &num_end, 10);\n \tif (num_end != brace)\n \t\treturn -1;\n \tif (nth <= 0)\n \t\treturn -1;\n \tcb.remaining = nth;\n-\tstrbuf_init(&cb.buf, 20);\n+\tcb.sb = buf;\n\n \tretval = refs_for_each_reflog_ent_reverse(get_main_ref_store(r),\n \t\t\t\"HEAD\", grab_nth_branch_switch, &cb);\n \tif (0 < retval) {\n-\t\tstrbuf_reset(buf);\n-\t\tstrbuf_addbuf(buf, &cb.buf);\n \t\tretval = brace - name + 1;\n \t} else\n \t\tretval = 0;\n\n-\tstrbuf_release(&cb.buf);\n \treturn retval;\n }\n\n--\n2.23.0\n"},{"id":"382570","messageId":"9a7e07ea-dd88-b1f0-125a-bc0f2a6d1fe9@gmail.com","threadId":"51878","inReplyTo":"582d8659-dd5e-440e-6f00-a59c7017dff6@web.de","subject":"Re: [PATCH] sha1_name: simplify strbuf handling in interpret_nth_prior_checkout()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-18T20:02:08Z","receivedAt":"2019-09-18T20:02:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/18/2019 12:35 PM, René Scharfe wrote:\n> Pass the target strbuf to the callback function grab_nth_branch_switch()\n> by reference so that it can add the result string directly instead of\n> having it put the string into a temporary strbuf first.  This gets rid\n> of an extra allocation and a string copy.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Patch formatted with --function-context for easier reviewing.\n\nI appreciate this. I needed to look through the whole method to see that\npreviously \"buf\" was used to take the resulting value on a successful\nparse.\n\n>  sha1-name.c | 11 ++++-------\n>  1 file changed, 4 insertions(+), 7 deletions(-)\n> \n> diff --git a/sha1-name.c b/sha1-name.c\n> index c665e3f96d..85196929c7 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1286,70 +1286,67 @@ static int get_oid_oneline(struct repository *r,\n> \n>  struct grab_nth_branch_switch_cbdata {\n>  \tint remaining;\n> -\tstruct strbuf buf;\n> +\tstruct strbuf *sb;\n>  };\n> \n>  static int grab_nth_branch_switch(struct object_id *ooid, struct object_id *noid,\n>  \t\t\t\t  const char *email, timestamp_t timestamp, int tz,\n>  \t\t\t\t  const char *message, void *cb_data)\n>  {\n>  \tstruct grab_nth_branch_switch_cbdata *cb = cb_data;\n>  \tconst char *match = NULL, *target = NULL;\n>  \tsize_t len;\n> \n>  \tif (skip_prefix(message, \"checkout: moving from \", &match))\n>  \t\ttarget = strstr(match, \" to \");\n> \n>  \tif (!match || !target)\n>  \t\treturn 0;\n>  \tif (--(cb->remaining) == 0) {\n>  \t\tlen = target - match;\n> -\t\tstrbuf_reset(&cb->buf);\n> -\t\tstrbuf_add(&cb->buf, match, len);\n> +\t\tstrbuf_reset(cb->sb);\n> +\t\tstrbuf_add(cb->sb, match, len);\n>  \t\treturn 1; /* we are done */\n\nIt seems that now \"buf\" is still only being replaced on a successful parse, since\nthis strbuf_reset and strbuf_add is happening only on a return 1, which terminates\nthe refs_for_each... method.\n\n>  \t}\n>  \treturn 0;\n>  }\n> \n>  /*\n>   * Parse @{-N} syntax, return the number of characters parsed\n>   * if successful; otherwise signal an error with negative value.\n>   */\n>  static int interpret_nth_prior_checkout(struct repository *r,\n>  \t\t\t\t\tconst char *name, int namelen,\n>  \t\t\t\t\tstruct strbuf *buf)\n>  {\n>  \tlong nth;\n>  \tint retval;\n>  \tstruct grab_nth_branch_switch_cbdata cb;\n>  \tconst char *brace;\n>  \tchar *num_end;\n> \n>  \tif (namelen < 4)\n>  \t\treturn -1;\n>  \tif (name[0] != '@' || name[1] != '{' || name[2] != '-')\n>  \t\treturn -1;\n>  \tbrace = memchr(name, '}', namelen);\n>  \tif (!brace)\n>  \t\treturn -1;\n>  \tnth = strtol(name + 3, &num_end, 10);\n>  \tif (num_end != brace)\n>  \t\treturn -1;\n>  \tif (nth <= 0)\n>  \t\treturn -1;\n>  \tcb.remaining = nth;\n> -\tstrbuf_init(&cb.buf, 20);\n> +\tcb.sb = buf;\n> \n>  \tretval = refs_for_each_reflog_ent_reverse(get_main_ref_store(r),\n>  \t\t\t\"HEAD\", grab_nth_branch_switch, &cb);\n>  \tif (0 < retval) {\n> -\t\tstrbuf_reset(buf);\n> -\t\tstrbuf_addbuf(buf, &cb.buf);\n>  \t\tretval = brace - name + 1;\n>  \t} else\n>  \t\tretval = 0;\n> \n> -\tstrbuf_release(&cb.buf);\n>  \treturn retval;\n>  }\n> \n\nThe reaction in this method appears correct, too.\n\nThanks,\n-Stolee\n\n"}]}