{"thread":{"id":"48854","subject":"[PATCH v1] convert log_ref_write_fd() to use strbuf","startedAt":"2018-07-10T18:20:27Z","lastAt":"2018-07-13T18:12:36Z","messageCount":8,"participants":["Ben Peart","Jeff King","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352149","messageId":"20180710182000.21404-1-benpeart@microsoft.com","threadId":"48854","inReplyTo":null,"subject":"[PATCH v1] convert log_ref_write_fd() to use strbuf","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-07-10T18:20:22Z","receivedAt":"2018-07-10T18:20:27Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"log_ref_write_fd() was written long before strbuf was fleshed out. Remove\nthe old manual buffer management code and replace it with strbuf(). Also\nupdate copy_reflog_msg() which is called only by log_ref_write_fd() to use\nstrbuf as it keeps things consistent.\n\nSigned-off-by: Ben Peart <Ben.Peart@microsoft.com>\n---\n\nNotes:\n    Base Ref: master\n    Web-Diff: https://github.com/benpeart/git/commit/c8d9c48adc\n    Checkout: git fetch https://github.com/benpeart/git refs-strbuf-v1 && git checkout c8d9c48adc\n\n refs.c               | 12 ++++--------\n refs/files-backend.c | 25 +++++++++----------------\n refs/refs-internal.h |  7 +++----\n 3 files changed, 16 insertions(+), 28 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0eb379f931..50fe5c5d2c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -786,25 +786,21 @@ int delete_ref(const char *msg, const char *refname,\n \t\t\t       old_oid, flags);\n }\n \n-int copy_reflog_msg(char *buf, const char *msg)\n+void copy_reflog_msg(struct strbuf *sb, const char *msg)\n {\n-\tchar *cp = buf;\n \tchar c;\n \tint wasspace = 1;\n \n-\t*cp++ = '\\t';\n+\tstrbuf_addch(sb, '\\t');\n \twhile ((c = *msg++)) {\n \t\tif (wasspace && isspace(c))\n \t\t\tcontinue;\n \t\twasspace = isspace(c);\n \t\tif (wasspace)\n \t\t\tc = ' ';\n-\t\t*cp++ = c;\n+\t\tstrbuf_addch(sb, c);\n \t}\n-\twhile (buf < cp && isspace(cp[-1]))\n-\t\tcp--;\n-\t*cp++ = '\\n';\n-\treturn cp - buf;\n+\tstrbuf_rtrim(sb);\n }\n \n int should_autocreate_reflog(const char *refname)\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a9a066dcfb..c0e892d0c8 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1582,22 +1582,15 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n \t\t\t    const struct object_id *new_oid,\n \t\t\t    const char *committer, const char *msg)\n {\n-\tint msglen, written;\n-\tunsigned maxlen, len;\n-\tchar *logrec;\n-\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = xsnprintf(logrec, maxlen, \"%s %s %s\\n\",\n-\t\t\toid_to_hex(old_oid),\n-\t\t\toid_to_hex(new_oid),\n-\t\t\tcommitter);\n-\tif (msglen)\n-\t\tlen += copy_reflog_msg(logrec + len - 1, msg) - 1;\n-\n-\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n-\tfree(logrec);\n+\tint written;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n+\tif (msg && *msg)\n+\t\tcopy_reflog_msg(&sb, msg);\n+\tstrbuf_addch(&sb, '\\n');\n+\twritten = write_in_full(fd, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n \tif (written < 0)\n \t\treturn -1;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex dd834314bd..17a526078f 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -91,11 +91,10 @@ enum peel_status {\n enum peel_status peel_object(const struct object_id *name, struct object_id *oid);\n \n /*\n- * Copy the reflog message msg to buf, which has been allocated sufficiently\n- * large, while cleaning up the whitespaces.  Especially, convert LF to space,\n- * because reflog file is one line per entry.\n+ * Copy the reflog message msg to sb while cleaning up the whitespaces.\n+ * Especially, convert LF to space, because reflog file is one line per entry.\n  */\n-int copy_reflog_msg(char *buf, const char *msg);\n+void copy_reflog_msg(struct strbuf *sb, const char *msg);\n \n /**\n  * Information needed for a single ref update. Set new_oid to the new\n\nbase-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9\n-- \n2.17.0.gvfs.1.123.g449c066\n\n"},{"id":"352160","messageId":"20180710184534.GA27535@sigill.intra.peff.net","threadId":"48854","inReplyTo":"20180710182000.21404-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] convert log_ref_write_fd() to use strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T18:45:34Z","receivedAt":"2018-07-10T18:45:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 06:20:22PM +0000, Ben Peart wrote:\n\n> log_ref_write_fd() was written long before strbuf was fleshed out. Remove\n> the old manual buffer management code and replace it with strbuf(). Also\n> update copy_reflog_msg() which is called only by log_ref_write_fd() to use\n> strbuf as it keeps things consistent.\n\nYay! In all of my buffer size auditing over the years, I've repeatedly\ncome across this \"+ 100\" but it never quite made the cut for fixing,\nsince it wasn't (yet) actually broken. Thanks for tackling it.\n\n> -int copy_reflog_msg(char *buf, const char *msg)\n> +void copy_reflog_msg(struct strbuf *sb, const char *msg)\n\nGlad to see this \"int\" go; it should have been size_t anyway.\n\n>  {\n> -\tchar *cp = buf;\n>  \tchar c;\n>  \tint wasspace = 1;\n>  \n> -\t*cp++ = '\\t';\n> +\tstrbuf_addch(sb, '\\t');\n>  \twhile ((c = *msg++)) {\n>  \t\tif (wasspace && isspace(c))\n>  \t\t\tcontinue;\n>  \t\twasspace = isspace(c);\n>  \t\tif (wasspace)\n>  \t\t\tc = ' ';\n> -\t\t*cp++ = c;\n> +\t\tstrbuf_addch(sb, c);\n>  \t}\n\nThis is all fairly straight-forward.\n\n> -\twhile (buf < cp && isspace(cp[-1]))\n> -\t\tcp--;\n> -\t*cp++ = '\\n';\n> -\treturn cp - buf;\n> +\tstrbuf_rtrim(sb);\n\nUsing rtrim is a nice reduction in complexity. A pure translation would\ninclude a final strbuf_addch(sb, '\\n'). It looks like you moved that to\nthe caller. There's only one, so that's OK now, but it may affect topics\nin flight (and I do in fact have an old topic that calls it).\n\nBut I think it's OK, as the change in function signature means that any\ncallers will need updated anyway. So there's little risk of a silent\nmis-merge.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index a9a066dcfb..c0e892d0c8 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1582,22 +1582,15 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n>  \t\t\t    const struct object_id *new_oid,\n>  \t\t\t    const char *committer, const char *msg)\n>  {\n> -\tint msglen, written;\n> -\tunsigned maxlen, len;\n> -\tchar *logrec;\n> -\n> -\tmsglen = msg ? strlen(msg) : 0;\n> -\tmaxlen = strlen(committer) + msglen + 100;\n> -\tlogrec = xmalloc(maxlen);\n> -\tlen = xsnprintf(logrec, maxlen, \"%s %s %s\\n\",\n> -\t\t\toid_to_hex(old_oid),\n> -\t\t\toid_to_hex(new_oid),\n> -\t\t\tcommitter);\n> -\tif (msglen)\n> -\t\tlen += copy_reflog_msg(logrec + len - 1, msg) - 1;\n> -\n> -\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n> -\tfree(logrec);\n> +\tint written;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n> +\tif (msg && *msg)\n> +\t\tcopy_reflog_msg(&sb, msg);\n> +\tstrbuf_addch(&sb, '\\n');\n> +\twritten = write_in_full(fd, sb.buf, sb.len);\n> +\tstrbuf_release(&sb);\n>  \tif (written < 0)\n>  \t\treturn -1;\n\nThis looks like another straight-forward translation.\n\nWhile we're here, is it worth turning \"written\" into an ssize_t, which\nis the correct return from write_in_full()? Alternatively, I wonder if\nthe logic would be simpler to follow with:\n\n  int ret;\n\n  ...strbuf bits...\n\n  if (write_in_full(fd, sb.buf, sb.len) < 0)\n\tret = -1;\n  else\n\tret = 0;\n\n  strbuf_release(&sb);\n  return ret;\n\nWe don't actually care about the number of bytes at all.\n\nThat's minor, though. With or without such a change, I'd be happy to see\nit applied.\n\n-Peff\n"},{"id":"352172","messageId":"xmqqsh4q6fpb.fsf@gitster-ct.c.googlers.com","threadId":"48854","inReplyTo":"20180710184534.GA27535@sigill.intra.peff.net","subject":"Re: [PATCH v1] convert log_ref_write_fd() to use strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T19:41:52Z","receivedAt":"2018-07-10T19:41:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> -\twhile (buf < cp && isspace(cp[-1]))\n>> -\t\tcp--;\n>> -\t*cp++ = '\\n';\n>> -\treturn cp - buf;\n>> +\tstrbuf_rtrim(sb);\n>\n> Using rtrim is a nice reduction in complexity. A pure translation would\n> include a final strbuf_addch(sb, '\\n'). It looks like you moved that to\n> the caller. There's only one, so that's OK now, but it may affect topics\n> in flight (and I do in fact have an old topic that calls it).\n>\n> But I think it's OK, as the change in function signature means that any\n> callers will need updated anyway. So there's little risk of a silent\n> mis-merge.\n\nIt is interesting that we came to a slightly different conclusion,\nafter doing pretty much the same analysis ;-).  Unless Ben has a\nplan to use a version that trims the trailing LF elsewhere, there is\nno point changing what the function does, especially because the\nexisting and only caller does want the terminating LF at the end.\n"},{"id":"352177","messageId":"20180710202128.GA6886@sigill.intra.peff.net","threadId":"48854","inReplyTo":"xmqqsh4q6fpb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] convert log_ref_write_fd() to use strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T20:21:28Z","receivedAt":"2018-07-10T20:21:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 12:41:52PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> -\twhile (buf < cp && isspace(cp[-1]))\n> >> -\t\tcp--;\n> >> -\t*cp++ = '\\n';\n> >> -\treturn cp - buf;\n> >> +\tstrbuf_rtrim(sb);\n> >\n> > Using rtrim is a nice reduction in complexity. A pure translation would\n> > include a final strbuf_addch(sb, '\\n'). It looks like you moved that to\n> > the caller. There's only one, so that's OK now, but it may affect topics\n> > in flight (and I do in fact have an old topic that calls it).\n> >\n> > But I think it's OK, as the change in function signature means that any\n> > callers will need updated anyway. So there's little risk of a silent\n> > mis-merge.\n> \n> It is interesting that we came to a slightly different conclusion,\n> after doing pretty much the same analysis ;-).  Unless Ben has a\n> plan to use a version that trims the trailing LF elsewhere, there is\n> no point changing what the function does, especially because the\n> existing and only caller does want the terminating LF at the end.\n\nThe original actually does a funny thing. It writes the newline into the\nbuffer, and then maybe calls copy_reflog_msg(). If it does, then we\nactually subtract one from the length we feed to the function, to roll\nback over the newline. That's harder to do with a strbuf, as those kinds\nof manual length shenanigans are discouraged (you'd use strbuf_setlen()\nto roll it back). At which point, you are much better off not adding it\nin the first place, and building the whole thing sequentially:\n\n  1. add the early bits that are in all entries\n\n  2. (maybe) add the tab and message if there is one\n\n  3. add the trailing newline\n\nAnd that's exactly what Ben's patch does.\n\nSo I think the end result is much cleaner that way. My concern was just\nthat the function semantics were changed.\n\n-Peff\n"},{"id":"352180","messageId":"20180710210806.23384-1-benpeart@microsoft.com","threadId":"48854","inReplyTo":"20180710182000.21404-1-benpeart@microsoft.com","subject":"[PATCH v2] convert log_ref_write_fd() to use strbuf","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-07-10T21:08:22Z","receivedAt":"2018-07-10T21:11:31Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Since we don't care about how many bytes were written, simplify the return\nvalue logic.\n\nlog_ref_write_fd() was written long before strbuf was fleshed out. Remove\nthe old manual buffer management code and replace it with strbuf(). Also\nupdate copy_reflog_msg() which is called only by log_ref_write_fd() to use\nstrbuf as it keeps things consistent.\n\nSigned-off-by: Ben Peart <Ben.Peart@microsoft.com>\n---\n\nNotes:\n    Base Ref: master\n    Web-Diff: https://github.com/benpeart/git/commit/b72cd95643\n    Checkout: git fetch https://github.com/benpeart/git refs-strbuf-v2 && git checkout b72cd95643\n    \n    ### Interdiff (v1..v2):\n    \n    diff --git a/refs/files-backend.c b/refs/files-backend.c\n    index c0e892d0c8..054306d779 100644\n    --- a/refs/files-backend.c\n    +++ b/refs/files-backend.c\n    @@ -1582,19 +1582,17 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n     \t\t\t    const struct object_id *new_oid,\n     \t\t\t    const char *committer, const char *msg)\n     {\n    -\tint written;\n     \tstruct strbuf sb = STRBUF_INIT;\n    +\tint ret = 0;\n    \n     \tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n     \tif (msg && *msg)\n     \t\tcopy_reflog_msg(&sb, msg);\n     \tstrbuf_addch(&sb, '\\n');\n    -\twritten = write_in_full(fd, sb.buf, sb.len);\n    +\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n    +\t\tret = -1;\n     \tstrbuf_release(&sb);\n    -\tif (written < 0)\n    -\t\treturn -1;\n    -\n    -\treturn 0;\n    +\treturn ret;\n     }\n    \n     static int files_log_ref_write(struct files_ref_store *refs,\n    \n    ### Patches\n\n refs.c               | 12 ++++--------\n refs/files-backend.c | 29 ++++++++++-------------------\n refs/refs-internal.h |  7 +++----\n 3 files changed, 17 insertions(+), 31 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0eb379f931..50fe5c5d2c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -786,25 +786,21 @@ int delete_ref(const char *msg, const char *refname,\n \t\t\t       old_oid, flags);\n }\n \n-int copy_reflog_msg(char *buf, const char *msg)\n+void copy_reflog_msg(struct strbuf *sb, const char *msg)\n {\n-\tchar *cp = buf;\n \tchar c;\n \tint wasspace = 1;\n \n-\t*cp++ = '\\t';\n+\tstrbuf_addch(sb, '\\t');\n \twhile ((c = *msg++)) {\n \t\tif (wasspace && isspace(c))\n \t\t\tcontinue;\n \t\twasspace = isspace(c);\n \t\tif (wasspace)\n \t\t\tc = ' ';\n-\t\t*cp++ = c;\n+\t\tstrbuf_addch(sb, c);\n \t}\n-\twhile (buf < cp && isspace(cp[-1]))\n-\t\tcp--;\n-\t*cp++ = '\\n';\n-\treturn cp - buf;\n+\tstrbuf_rtrim(sb);\n }\n \n int should_autocreate_reflog(const char *refname)\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a9a066dcfb..054306d779 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1582,26 +1582,17 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n \t\t\t    const struct object_id *new_oid,\n \t\t\t    const char *committer, const char *msg)\n {\n-\tint msglen, written;\n-\tunsigned maxlen, len;\n-\tchar *logrec;\n-\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = xsnprintf(logrec, maxlen, \"%s %s %s\\n\",\n-\t\t\toid_to_hex(old_oid),\n-\t\t\toid_to_hex(new_oid),\n-\t\t\tcommitter);\n-\tif (msglen)\n-\t\tlen += copy_reflog_msg(logrec + len - 1, msg) - 1;\n-\n-\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n-\tfree(logrec);\n-\tif (written < 0)\n-\t\treturn -1;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n \n-\treturn 0;\n+\tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n+\tif (msg && *msg)\n+\t\tcopy_reflog_msg(&sb, msg);\n+\tstrbuf_addch(&sb, '\\n');\n+\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n+\t\tret = -1;\n+\tstrbuf_release(&sb);\n+\treturn ret;\n }\n \n static int files_log_ref_write(struct files_ref_store *refs,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex dd834314bd..17a526078f 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -91,11 +91,10 @@ enum peel_status {\n enum peel_status peel_object(const struct object_id *name, struct object_id *oid);\n \n /*\n- * Copy the reflog message msg to buf, which has been allocated sufficiently\n- * large, while cleaning up the whitespaces.  Especially, convert LF to space,\n- * because reflog file is one line per entry.\n+ * Copy the reflog message msg to sb while cleaning up the whitespaces.\n+ * Especially, convert LF to space, because reflog file is one line per entry.\n  */\n-int copy_reflog_msg(char *buf, const char *msg);\n+void copy_reflog_msg(struct strbuf *sb, const char *msg);\n \n /**\n  * Information needed for a single ref update. Set new_oid to the new\n\nbase-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9\n-- \n2.17.0.gvfs.1.123.g449c066\n\n"},{"id":"352182","messageId":"xmqqefga6b1l.fsf@gitster-ct.c.googlers.com","threadId":"48854","inReplyTo":"20180710210806.23384-1-benpeart@microsoft.com","subject":"Re: [PATCH v2] convert log_ref_write_fd() to use strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T21:22:30Z","receivedAt":"2018-07-10T21:22:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <Ben.Peart@microsoft.com> writes:\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index a9a066dcfb..054306d779 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1582,26 +1582,17 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n>  \t\t\t    const struct object_id *new_oid,\n>  \t\t\t    const char *committer, const char *msg)\n>  {\n> -\tint msglen, written;\n> -\tunsigned maxlen, len;\n> -\tchar *logrec;\n> -\n> -\tmsglen = msg ? strlen(msg) : 0;\n> -\tmaxlen = strlen(committer) + msglen + 100;\n> -\tlogrec = xmalloc(maxlen);\n> -\tlen = xsnprintf(logrec, maxlen, \"%s %s %s\\n\",\n> -\t\t\toid_to_hex(old_oid),\n> -\t\t\toid_to_hex(new_oid),\n> -\t\t\tcommitter);\n> -\tif (msglen)\n> -\t\tlen += copy_reflog_msg(logrec + len - 1, msg) - 1;\n> -\n> -\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n> -\tfree(logrec);\n> -\tif (written < 0)\n> -\t\treturn -1;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tint ret = 0;\n>  \n> -\treturn 0;\n> +\tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n> +\tif (msg && *msg)\n> +\t\tcopy_reflog_msg(&sb, msg);\n> +\tstrbuf_addch(&sb, '\\n');\n> +\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n> +\t\tret = -1;\n> +\tstrbuf_release(&sb);\n> +\treturn ret;\n\nNicely plugged here.  Looks quite sensible.\n"},{"id":"352183","messageId":"9666fa00-607c-a212-3104-a9ef8ff2a60b@gmail.com","threadId":"48854","inReplyTo":"20180710202128.GA6886@sigill.intra.peff.net","subject":"Re: [PATCH v1] convert log_ref_write_fd() to use strbuf","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-10T20:57:09Z","receivedAt":"2018-07-10T21:26:49Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/10/2018 4:21 PM, Jeff King wrote:\n> On Tue, Jul 10, 2018 at 12:41:52PM -0700, Junio C Hamano wrote:\n> \n>> Jeff King <peff@peff.net> writes:\n>>\n>>>> -\twhile (buf < cp && isspace(cp[-1]))\n>>>> -\t\tcp--;\n>>>> -\t*cp++ = '\\n';\n>>>> -\treturn cp - buf;\n>>>> +\tstrbuf_rtrim(sb);\n>>>\n>>> Using rtrim is a nice reduction in complexity. A pure translation would\n>>> include a final strbuf_addch(sb, '\\n'). It looks like you moved that to\n>>> the caller. There's only one, so that's OK now, but it may affect topics\n>>> in flight (and I do in fact have an old topic that calls it).\n>>>\n>>> But I think it's OK, as the change in function signature means that any\n>>> callers will need updated anyway. So there's little risk of a silent\n>>> mis-merge.\n>>\n>> It is interesting that we came to a slightly different conclusion,\n>> after doing pretty much the same analysis ;-).  Unless Ben has a\n>> plan to use a version that trims the trailing LF elsewhere, there is\n>> no point changing what the function does, especially because the\n>> existing and only caller does want the terminating LF at the end.\n> \n> The original actually does a funny thing. It writes the newline into the\n> buffer, and then maybe calls copy_reflog_msg(). If it does, then we\n> actually subtract one from the length we feed to the function, to roll\n> back over the newline. That's harder to do with a strbuf, as those kinds\n> of manual length shenanigans are discouraged (you'd use strbuf_setlen()\n> to roll it back). At which point, you are much better off not adding it\n> in the first place, and building the whole thing sequentially:\n> \n>    1. add the early bits that are in all entries\n> \n>    2. (maybe) add the tab and message if there is one\n> \n>    3. add the trailing newline\n> \n> And that's exactly what Ben's patch does.\n> \n> So I think the end result is much cleaner that way. My concern was just\n> that the function semantics were changed.\n> \n> -Peff\n> \n\nAnd that is exactly why I ended up moving the logic to append the \nnewline out to the caller.  I wrote it the other way first but it was \npretty messy - since there were no other callers, it was cleaner/simpler \nto move it out. :)  For any future callers, it is pretty trivial to add \nthe call to strbuf_addch(&sb, '\\n') if they want a trailing newline.\n"},{"id":"352518","messageId":"20180713181228.GB968742@genre.crustytoothpaste.net","threadId":"48854","inReplyTo":"20180710210806.23384-1-benpeart@microsoft.com","subject":"Re: [PATCH v2] convert log_ref_write_fd() to use strbuf","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-13T18:12:28Z","receivedAt":"2018-07-13T18:12:36Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 10, 2018 at 09:08:22PM +0000, Ben Peart wrote:\n> Since we don't care about how many bytes were written, simplify the return\n> value logic.\n> \n> log_ref_write_fd() was written long before strbuf was fleshed out. Remove\n> the old manual buffer management code and replace it with strbuf(). Also\n> update copy_reflog_msg() which is called only by log_ref_write_fd() to use\n> strbuf as it keeps things consistent.\n> \n> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>\n\nThis looks good to me.  Thanks for the patch.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}