* [PATCH v1] convert log_ref_write_fd() to use strbuf @ 2018-07-10 18:20 Ben Peart 2018-07-10 18:45 ` Jeff King 2018-07-10 21:08 ` [PATCH v2] " Ben Peart 0 siblings, 2 replies; 8+ messages in thread From: Ben Peart @ 2018-07-10 18:20 UTC (permalink / raw) To: git@vger.kernel.org, gitster@pobox.com, sandals@crustytoothpaste.net, peff@peff.net, stolee@gmail.com Cc: Ben Peart, Ben Peart log_ref_write_fd() was written long before strbuf was fleshed out. Remove the old manual buffer management code and replace it with strbuf(). Also update copy_reflog_msg() which is called only by log_ref_write_fd() to use strbuf as it keeps things consistent. Signed-off-by: Ben Peart <Ben.Peart@microsoft.com> --- Notes: Base Ref: master Web-Diff: https://github.com/benpeart/git/commit/c8d9c48adc Checkout: git fetch https://github.com/benpeart/git refs-strbuf-v1 && git checkout c8d9c48adc refs.c | 12 ++++-------- refs/files-backend.c | 25 +++++++++---------------- refs/refs-internal.h | 7 +++---- 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/refs.c b/refs.c index 0eb379f931..50fe5c5d2c 100644 --- a/refs.c +++ b/refs.c @@ -786,25 +786,21 @@ int delete_ref(const char *msg, const char *refname, old_oid, flags); } -int copy_reflog_msg(char *buf, const char *msg) +void copy_reflog_msg(struct strbuf *sb, const char *msg) { - char *cp = buf; char c; int wasspace = 1; - *cp++ = '\t'; + strbuf_addch(sb, '\t'); while ((c = *msg++)) { if (wasspace && isspace(c)) continue; wasspace = isspace(c); if (wasspace) c = ' '; - *cp++ = c; + strbuf_addch(sb, c); } - while (buf < cp && isspace(cp[-1])) - cp--; - *cp++ = '\n'; - return cp - buf; + strbuf_rtrim(sb); } int should_autocreate_reflog(const char *refname) diff --git a/refs/files-backend.c b/refs/files-backend.c index a9a066dcfb..c0e892d0c8 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1582,22 +1582,15 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, const struct object_id *new_oid, const char *committer, const char *msg) { - int msglen, written; - unsigned maxlen, len; - char *logrec; - - msglen = msg ? strlen(msg) : 0; - maxlen = strlen(committer) + msglen + 100; - logrec = xmalloc(maxlen); - len = xsnprintf(logrec, maxlen, "%s %s %s\n", - oid_to_hex(old_oid), - oid_to_hex(new_oid), - committer); - if (msglen) - len += copy_reflog_msg(logrec + len - 1, msg) - 1; - - written = len <= maxlen ? write_in_full(fd, logrec, len) : -1; - free(logrec); + int written; + struct strbuf sb = STRBUF_INIT; + + strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); + if (msg && *msg) + copy_reflog_msg(&sb, msg); + strbuf_addch(&sb, '\n'); + written = write_in_full(fd, sb.buf, sb.len); + strbuf_release(&sb); if (written < 0) return -1; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index dd834314bd..17a526078f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -91,11 +91,10 @@ enum peel_status { enum peel_status peel_object(const struct object_id *name, struct object_id *oid); /* - * Copy the reflog message msg to buf, which has been allocated sufficiently - * large, while cleaning up the whitespaces. Especially, convert LF to space, - * because reflog file is one line per entry. + * Copy the reflog message msg to sb while cleaning up the whitespaces. + * Especially, convert LF to space, because reflog file is one line per entry. */ -int copy_reflog_msg(char *buf, const char *msg); +void copy_reflog_msg(struct strbuf *sb, const char *msg); /** * Information needed for a single ref update. Set new_oid to the new base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9 -- 2.17.0.gvfs.1.123.g449c066 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1] convert log_ref_write_fd() to use strbuf 2018-07-10 18:20 [PATCH v1] convert log_ref_write_fd() to use strbuf Ben Peart @ 2018-07-10 18:45 ` Jeff King 2018-07-10 19:41 ` Junio C Hamano 2018-07-10 21:08 ` [PATCH v2] " Ben Peart 1 sibling, 1 reply; 8+ messages in thread From: Jeff King @ 2018-07-10 18:45 UTC (permalink / raw) To: Ben Peart Cc: git@vger.kernel.org, gitster@pobox.com, sandals@crustytoothpaste.net, stolee@gmail.com On Tue, Jul 10, 2018 at 06:20:22PM +0000, Ben Peart wrote: > log_ref_write_fd() was written long before strbuf was fleshed out. Remove > the old manual buffer management code and replace it with strbuf(). Also > update copy_reflog_msg() which is called only by log_ref_write_fd() to use > strbuf as it keeps things consistent. Yay! In all of my buffer size auditing over the years, I've repeatedly come across this "+ 100" but it never quite made the cut for fixing, since it wasn't (yet) actually broken. Thanks for tackling it. > -int copy_reflog_msg(char *buf, const char *msg) > +void copy_reflog_msg(struct strbuf *sb, const char *msg) Glad to see this "int" go; it should have been size_t anyway. > { > - char *cp = buf; > char c; > int wasspace = 1; > > - *cp++ = '\t'; > + strbuf_addch(sb, '\t'); > while ((c = *msg++)) { > if (wasspace && isspace(c)) > continue; > wasspace = isspace(c); > if (wasspace) > c = ' '; > - *cp++ = c; > + strbuf_addch(sb, c); > } This is all fairly straight-forward. > - while (buf < cp && isspace(cp[-1])) > - cp--; > - *cp++ = '\n'; > - return cp - buf; > + strbuf_rtrim(sb); Using rtrim is a nice reduction in complexity. A pure translation would include a final strbuf_addch(sb, '\n'). It looks like you moved that to the caller. There's only one, so that's OK now, but it may affect topics in flight (and I do in fact have an old topic that calls it). But I think it's OK, as the change in function signature means that any callers will need updated anyway. So there's little risk of a silent mis-merge. > diff --git a/refs/files-backend.c b/refs/files-backend.c > index a9a066dcfb..c0e892d0c8 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -1582,22 +1582,15 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, > const struct object_id *new_oid, > const char *committer, const char *msg) > { > - int msglen, written; > - unsigned maxlen, len; > - char *logrec; > - > - msglen = msg ? strlen(msg) : 0; > - maxlen = strlen(committer) + msglen + 100; > - logrec = xmalloc(maxlen); > - len = xsnprintf(logrec, maxlen, "%s %s %s\n", > - oid_to_hex(old_oid), > - oid_to_hex(new_oid), > - committer); > - if (msglen) > - len += copy_reflog_msg(logrec + len - 1, msg) - 1; > - > - written = len <= maxlen ? write_in_full(fd, logrec, len) : -1; > - free(logrec); > + int written; > + struct strbuf sb = STRBUF_INIT; > + > + strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); > + if (msg && *msg) > + copy_reflog_msg(&sb, msg); > + strbuf_addch(&sb, '\n'); > + written = write_in_full(fd, sb.buf, sb.len); > + strbuf_release(&sb); > if (written < 0) > return -1; This looks like another straight-forward translation. While we're here, is it worth turning "written" into an ssize_t, which is the correct return from write_in_full()? Alternatively, I wonder if the logic would be simpler to follow with: int ret; ...strbuf bits... if (write_in_full(fd, sb.buf, sb.len) < 0) ret = -1; else ret = 0; strbuf_release(&sb); return ret; We don't actually care about the number of bytes at all. That's minor, though. With or without such a change, I'd be happy to see it applied. -Peff ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] convert log_ref_write_fd() to use strbuf 2018-07-10 18:45 ` Jeff King @ 2018-07-10 19:41 ` Junio C Hamano 2018-07-10 20:21 ` Jeff King 0 siblings, 1 reply; 8+ messages in thread From: Junio C Hamano @ 2018-07-10 19:41 UTC (permalink / raw) To: Jeff King Cc: Ben Peart, git@vger.kernel.org, sandals@crustytoothpaste.net, stolee@gmail.com Jeff King <peff@peff.net> writes: >> - while (buf < cp && isspace(cp[-1])) >> - cp--; >> - *cp++ = '\n'; >> - return cp - buf; >> + strbuf_rtrim(sb); > > Using rtrim is a nice reduction in complexity. A pure translation would > include a final strbuf_addch(sb, '\n'). It looks like you moved that to > the caller. There's only one, so that's OK now, but it may affect topics > in flight (and I do in fact have an old topic that calls it). > > But I think it's OK, as the change in function signature means that any > callers will need updated anyway. So there's little risk of a silent > mis-merge. It is interesting that we came to a slightly different conclusion, after doing pretty much the same analysis ;-). Unless Ben has a plan to use a version that trims the trailing LF elsewhere, there is no point changing what the function does, especially because the existing and only caller does want the terminating LF at the end. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] convert log_ref_write_fd() to use strbuf 2018-07-10 19:41 ` Junio C Hamano @ 2018-07-10 20:21 ` Jeff King 2018-07-10 20:57 ` Ben Peart 0 siblings, 1 reply; 8+ messages in thread From: Jeff King @ 2018-07-10 20:21 UTC (permalink / raw) To: Junio C Hamano Cc: Ben Peart, git@vger.kernel.org, sandals@crustytoothpaste.net, stolee@gmail.com On Tue, Jul 10, 2018 at 12:41:52PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> - while (buf < cp && isspace(cp[-1])) > >> - cp--; > >> - *cp++ = '\n'; > >> - return cp - buf; > >> + strbuf_rtrim(sb); > > > > Using rtrim is a nice reduction in complexity. A pure translation would > > include a final strbuf_addch(sb, '\n'). It looks like you moved that to > > the caller. There's only one, so that's OK now, but it may affect topics > > in flight (and I do in fact have an old topic that calls it). > > > > But I think it's OK, as the change in function signature means that any > > callers will need updated anyway. So there's little risk of a silent > > mis-merge. > > It is interesting that we came to a slightly different conclusion, > after doing pretty much the same analysis ;-). Unless Ben has a > plan to use a version that trims the trailing LF elsewhere, there is > no point changing what the function does, especially because the > existing and only caller does want the terminating LF at the end. The original actually does a funny thing. It writes the newline into the buffer, and then maybe calls copy_reflog_msg(). If it does, then we actually subtract one from the length we feed to the function, to roll back over the newline. That's harder to do with a strbuf, as those kinds of manual length shenanigans are discouraged (you'd use strbuf_setlen() to roll it back). At which point, you are much better off not adding it in the first place, and building the whole thing sequentially: 1. add the early bits that are in all entries 2. (maybe) add the tab and message if there is one 3. add the trailing newline And that's exactly what Ben's patch does. So I think the end result is much cleaner that way. My concern was just that the function semantics were changed. -Peff ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] convert log_ref_write_fd() to use strbuf 2018-07-10 20:21 ` Jeff King @ 2018-07-10 20:57 ` Ben Peart 0 siblings, 0 replies; 8+ messages in thread From: Ben Peart @ 2018-07-10 20:57 UTC (permalink / raw) To: Jeff King, Junio C Hamano Cc: Ben Peart, git@vger.kernel.org, sandals@crustytoothpaste.net, stolee@gmail.com On 7/10/2018 4:21 PM, Jeff King wrote: > On Tue, Jul 10, 2018 at 12:41:52PM -0700, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >>>> - while (buf < cp && isspace(cp[-1])) >>>> - cp--; >>>> - *cp++ = '\n'; >>>> - return cp - buf; >>>> + strbuf_rtrim(sb); >>> >>> Using rtrim is a nice reduction in complexity. A pure translation would >>> include a final strbuf_addch(sb, '\n'). It looks like you moved that to >>> the caller. There's only one, so that's OK now, but it may affect topics >>> in flight (and I do in fact have an old topic that calls it). >>> >>> But I think it's OK, as the change in function signature means that any >>> callers will need updated anyway. So there's little risk of a silent >>> mis-merge. >> >> It is interesting that we came to a slightly different conclusion, >> after doing pretty much the same analysis ;-). Unless Ben has a >> plan to use a version that trims the trailing LF elsewhere, there is >> no point changing what the function does, especially because the >> existing and only caller does want the terminating LF at the end. > > The original actually does a funny thing. It writes the newline into the > buffer, and then maybe calls copy_reflog_msg(). If it does, then we > actually subtract one from the length we feed to the function, to roll > back over the newline. That's harder to do with a strbuf, as those kinds > of manual length shenanigans are discouraged (you'd use strbuf_setlen() > to roll it back). At which point, you are much better off not adding it > in the first place, and building the whole thing sequentially: > > 1. add the early bits that are in all entries > > 2. (maybe) add the tab and message if there is one > > 3. add the trailing newline > > And that's exactly what Ben's patch does. > > So I think the end result is much cleaner that way. My concern was just > that the function semantics were changed. > > -Peff > And that is exactly why I ended up moving the logic to append the newline out to the caller. I wrote it the other way first but it was pretty messy - since there were no other callers, it was cleaner/simpler to move it out. :) For any future callers, it is pretty trivial to add the call to strbuf_addch(&sb, '\n') if they want a trailing newline. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] convert log_ref_write_fd() to use strbuf 2018-07-10 18:20 [PATCH v1] convert log_ref_write_fd() to use strbuf Ben Peart 2018-07-10 18:45 ` Jeff King @ 2018-07-10 21:08 ` Ben Peart 2018-07-10 21:22 ` Junio C Hamano 2018-07-13 18:12 ` brian m. carlson 1 sibling, 2 replies; 8+ messages in thread From: Ben Peart @ 2018-07-10 21:08 UTC (permalink / raw) To: Ben Peart Cc: git@vger.kernel.org, gitster@pobox.com, peff@peff.net, sandals@crustytoothpaste.net, stolee@gmail.com, Ben Peart, Ben Peart Since we don't care about how many bytes were written, simplify the return value logic. log_ref_write_fd() was written long before strbuf was fleshed out. Remove the old manual buffer management code and replace it with strbuf(). Also update copy_reflog_msg() which is called only by log_ref_write_fd() to use strbuf as it keeps things consistent. Signed-off-by: Ben Peart <Ben.Peart@microsoft.com> --- Notes: Base Ref: master Web-Diff: https://github.com/benpeart/git/commit/b72cd95643 Checkout: git fetch https://github.com/benpeart/git refs-strbuf-v2 && git checkout b72cd95643 ### Interdiff (v1..v2): diff --git a/refs/files-backend.c b/refs/files-backend.c index c0e892d0c8..054306d779 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1582,19 +1582,17 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, const struct object_id *new_oid, const char *committer, const char *msg) { - int written; struct strbuf sb = STRBUF_INIT; + int ret = 0; strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); if (msg && *msg) copy_reflog_msg(&sb, msg); strbuf_addch(&sb, '\n'); - written = write_in_full(fd, sb.buf, sb.len); + if (write_in_full(fd, sb.buf, sb.len) < 0) + ret = -1; strbuf_release(&sb); - if (written < 0) - return -1; - - return 0; + return ret; } static int files_log_ref_write(struct files_ref_store *refs, ### Patches refs.c | 12 ++++-------- refs/files-backend.c | 29 ++++++++++------------------- refs/refs-internal.h | 7 +++---- 3 files changed, 17 insertions(+), 31 deletions(-) diff --git a/refs.c b/refs.c index 0eb379f931..50fe5c5d2c 100644 --- a/refs.c +++ b/refs.c @@ -786,25 +786,21 @@ int delete_ref(const char *msg, const char *refname, old_oid, flags); } -int copy_reflog_msg(char *buf, const char *msg) +void copy_reflog_msg(struct strbuf *sb, const char *msg) { - char *cp = buf; char c; int wasspace = 1; - *cp++ = '\t'; + strbuf_addch(sb, '\t'); while ((c = *msg++)) { if (wasspace && isspace(c)) continue; wasspace = isspace(c); if (wasspace) c = ' '; - *cp++ = c; + strbuf_addch(sb, c); } - while (buf < cp && isspace(cp[-1])) - cp--; - *cp++ = '\n'; - return cp - buf; + strbuf_rtrim(sb); } int should_autocreate_reflog(const char *refname) diff --git a/refs/files-backend.c b/refs/files-backend.c index a9a066dcfb..054306d779 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1582,26 +1582,17 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, const struct object_id *new_oid, const char *committer, const char *msg) { - int msglen, written; - unsigned maxlen, len; - char *logrec; - - msglen = msg ? strlen(msg) : 0; - maxlen = strlen(committer) + msglen + 100; - logrec = xmalloc(maxlen); - len = xsnprintf(logrec, maxlen, "%s %s %s\n", - oid_to_hex(old_oid), - oid_to_hex(new_oid), - committer); - if (msglen) - len += copy_reflog_msg(logrec + len - 1, msg) - 1; - - written = len <= maxlen ? write_in_full(fd, logrec, len) : -1; - free(logrec); - if (written < 0) - return -1; + struct strbuf sb = STRBUF_INIT; + int ret = 0; - return 0; + strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); + if (msg && *msg) + copy_reflog_msg(&sb, msg); + strbuf_addch(&sb, '\n'); + if (write_in_full(fd, sb.buf, sb.len) < 0) + ret = -1; + strbuf_release(&sb); + return ret; } static int files_log_ref_write(struct files_ref_store *refs, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index dd834314bd..17a526078f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -91,11 +91,10 @@ enum peel_status { enum peel_status peel_object(const struct object_id *name, struct object_id *oid); /* - * Copy the reflog message msg to buf, which has been allocated sufficiently - * large, while cleaning up the whitespaces. Especially, convert LF to space, - * because reflog file is one line per entry. + * Copy the reflog message msg to sb while cleaning up the whitespaces. + * Especially, convert LF to space, because reflog file is one line per entry. */ -int copy_reflog_msg(char *buf, const char *msg); +void copy_reflog_msg(struct strbuf *sb, const char *msg); /** * Information needed for a single ref update. Set new_oid to the new base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9 -- 2.17.0.gvfs.1.123.g449c066 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] convert log_ref_write_fd() to use strbuf 2018-07-10 21:08 ` [PATCH v2] " Ben Peart @ 2018-07-10 21:22 ` Junio C Hamano 2018-07-13 18:12 ` brian m. carlson 1 sibling, 0 replies; 8+ messages in thread From: Junio C Hamano @ 2018-07-10 21:22 UTC (permalink / raw) To: Ben Peart Cc: git@vger.kernel.org, peff@peff.net, sandals@crustytoothpaste.net, stolee@gmail.com Ben Peart <Ben.Peart@microsoft.com> writes: > diff --git a/refs/files-backend.c b/refs/files-backend.c > index a9a066dcfb..054306d779 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -1582,26 +1582,17 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, > const struct object_id *new_oid, > const char *committer, const char *msg) > { > - int msglen, written; > - unsigned maxlen, len; > - char *logrec; > - > - msglen = msg ? strlen(msg) : 0; > - maxlen = strlen(committer) + msglen + 100; > - logrec = xmalloc(maxlen); > - len = xsnprintf(logrec, maxlen, "%s %s %s\n", > - oid_to_hex(old_oid), > - oid_to_hex(new_oid), > - committer); > - if (msglen) > - len += copy_reflog_msg(logrec + len - 1, msg) - 1; > - > - written = len <= maxlen ? write_in_full(fd, logrec, len) : -1; > - free(logrec); > - if (written < 0) > - return -1; > + struct strbuf sb = STRBUF_INIT; > + int ret = 0; > > - return 0; > + strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); > + if (msg && *msg) > + copy_reflog_msg(&sb, msg); > + strbuf_addch(&sb, '\n'); > + if (write_in_full(fd, sb.buf, sb.len) < 0) > + ret = -1; > + strbuf_release(&sb); > + return ret; Nicely plugged here. Looks quite sensible. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] convert log_ref_write_fd() to use strbuf 2018-07-10 21:08 ` [PATCH v2] " Ben Peart 2018-07-10 21:22 ` Junio C Hamano @ 2018-07-13 18:12 ` brian m. carlson 1 sibling, 0 replies; 8+ messages in thread From: brian m. carlson @ 2018-07-13 18:12 UTC (permalink / raw) To: Ben Peart Cc: git@vger.kernel.org, gitster@pobox.com, peff@peff.net, stolee@gmail.com [-- Attachment #1: Type: text/plain, Size: 616 bytes --] On Tue, Jul 10, 2018 at 09:08:22PM +0000, Ben Peart wrote: > Since we don't care about how many bytes were written, simplify the return > value logic. > > log_ref_write_fd() was written long before strbuf was fleshed out. Remove > the old manual buffer management code and replace it with strbuf(). Also > update copy_reflog_msg() which is called only by log_ref_write_fd() to use > strbuf as it keeps things consistent. > > Signed-off-by: Ben Peart <Ben.Peart@microsoft.com> This looks good to me. Thanks for the patch. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 867 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-13 18:12 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-10 18:20 [PATCH v1] convert log_ref_write_fd() to use strbuf Ben Peart 2018-07-10 18:45 ` Jeff King 2018-07-10 19:41 ` Junio C Hamano 2018-07-10 20:21 ` Jeff King 2018-07-10 20:57 ` Ben Peart 2018-07-10 21:08 ` [PATCH v2] " Ben Peart 2018-07-10 21:22 ` Junio C Hamano 2018-07-13 18:12 ` brian m. carlson
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).