threads / patch / 48854

patchconvert log_ref_write_fd() to use strbuf

Subject: [PATCH v1] convert log_ref_write_fd() to use strbuf

## tl;dr

8 messages between Jul 10, 2018 and Jul 13, 2018. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Ben Peart· Jul 10, 2018, 18:20 UTC · lore

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(-)
Show changes to 3 files +16 −28

refs.c, refs/files-backend.c, refs/refs-internal.h

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
Jeff King· Jul 10, 2018, 18:45 UTC · re: Ben Peart · lore

Re: [PATCH v1] convert log_ref_write_fd() to use strbuf

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.
Show 16 quoted lines
>  {
> -	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.
Show 5 quoted lines
> -	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.

Show 35 quoted lines
> 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
Junio C Hamano· Jul 10, 2018, 19:41 UTC · re: Jeff King · lore

Re: [PATCH v1] convert log_ref_write_fd() to use strbuf

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
>> -	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.

Jeff King· Jul 10, 2018, 20:21 UTC · re: Junio C Hamano · lore

Re: [PATCH v1] convert log_ref_write_fd() to use strbuf

On Tue, Jul 10, 2018 at 12:41:52PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> 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
Ben Peart· Jul 10, 2018, 20:57 UTC · re: Jeff King · lore

Re: [PATCH v1] convert log_ref_write_fd() to use strbuf

On 7/10/2018 4:21 PM, Jeff King wrote:
Show 46 quoted lines
> 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.

Ben Peart· Jul 10, 2018, 21:08 UTC · re: Ben Peart · lore

[PATCH v2] convert log_ref_write_fd() to use strbuf

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(-)
Show changes to 3 files +17 −31

refs.c, refs/files-backend.c, refs/refs-internal.h

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
Junio C Hamano· Jul 10, 2018, 21:22 UTC · re: Ben Peart · lore

Re: [PATCH v2] convert log_ref_write_fd() to use strbuf

Ben Peart <Ben.Peart@microsoft.com> writes:
Show 38 quoted lines
> 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.
brian m. carlson· Jul 13, 2018, 18:12 UTC · re: Ben Peart · lore

Re: [PATCH v2] convert log_ref_write_fd() to use strbuf

On Tue, Jul 10, 2018 at 09:08:22PM +0000, Ben Peart wrote:
Show 9 quoted lines
> 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

← back to recent threads