[PATCH v1] convert log_ref_write_fd() to use strbuf

[PATCH v1] convert log_ref_write_fd() to use strbuf

[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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Ben Peart]

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.

[PATCH v2] convert log_ref_write_fd() to use strbuf

[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

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

[Junio C Hamano]

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.

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

[brian m. carlson]

[-- 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 --]