rerere: update to modern representation of empty strbufs

Back when b4833a2c (rerere: Fix use of an empty strbuf.buf, 2007-09-26) was written, a freshly initialized empty strbuf had NULL in its .buf member, with .len set to 0. The code this patch touches in rerere.c was written to _fix_ the original code that assumed that the .buf member is always pointing at a NUL-terminated string, even for an empty string, which did not hold back then.

That changed in b315c5c0 (strbuf change: be sure ->buf is never ever NULL., 2007-09-27), and it has again become safe to assume that .buf is never NULL, and .buf[0] has '\0' for an empty string (i.e., a strbuf with its .len member set to 0).

rerere.c +0 -0

index 6ec55964e2..0296700f9f 100644
@@ -403,12 +403,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io,
 			strbuf_addbuf(out, &two);
 			rerere_strbuf_putconflict(out, '>', marker_size);

Subject: Re: [PATCH] rerere: update to modern representation of empty strbufs

Finally get rid of the special casing that was unnecessary for the last 19 years.

Signed-off-by Junio C Hamano <[email protected]>

---
 rerere.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

rerere.c +0 -1

index 6ec55964e2..0296700f9f 100644
@@ -403,12 +403,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io,
 			strbuf_addbuf(out, &two);
 			rerere_strbuf_putconflict(out, '>', marker_size);
 			if (ctx) {
-				git_hash_update(ctx, one.buf ?

One is in list-objects-filter-options.c::parse_list_objects_filter()

        void parse_list_objects_filter(
                struct list_objects_filter_options *filter_options,
                const char *arg)
        {
                struct strbuf errbuf = STRBUF_INIT;

                if (!filter_options->filter_spec.buf)
                        BUG("filter_options not properly initialized");

The filter_options variable points at a list_objects_filter_options structure, which has an embedded "struct strbuf". This BUG() is unnecessary if the structure is properly initialized, either by the LIST_OBJECTS_FILTER_INIT macro or a list_objects_filter_init() call. But it is easy to memset(&lofo, 0, sizeof(lofo)) or zero initialize with "= {0}", so I think it is OK to special case and allow for checking the possibility that .buf might be NULL.

Because strbuf_getwholeline() discards what is originally in sb and replaces it with what getdelim() returns, I have a suspicion that working with bare char * and size_t to interact with getdelim() and then using strbuf_attach() on the success case would be simpler to read and maintain. Once such a rewrite of this function is done (#leftoverbits), the special case we see in the Coccinelle rule can be lifted.

Quoted 7 lines

Because strbuf_getwholeline() discards what is originally in sb and replaces it with what getdelim() returns, I have a suspicion that working with bare char * and size_t to interact with getdelim() and then using strbuf_attach() on the success case would be simpler to read and maintain. Once such a rewrite of this function is done (#leftoverbits), the special case we see in the Coccinelle rule can be lifted.

Hmm. I think that is something like this:

strbuf.c +2 -0

index 3939863cf3..0333aea261 100644
@@ -631,6 +631,8 @@ int strbuf_getcwd(struct strbuf *sb)
 #ifdef HAVE_GETDELIM
 int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
+	char *buf;
+	size_t alloc;
 	ssize_t r;
 
 	if (feof(fp))

strbuf.c +0 -0

@@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	 * we can just re-init, but otherwise we should make sure that our
 	 * length is empty, and that the result is NUL-terminated.
 	 */

strbuf.c +0 -0

 	return EOF;
 }
 #else

So I don't know that it makes anything simpler. We have to copy the values back into the strbuf either way, and we still have to handle restoring the strbuf invariants. Even the strbuf_init() case is still needed, because we don't know whether getdelim() just didn't allocate (in which case we could leave the strbuf alone) or if it actually ate the allocation we passed in (which was just a copy of sb->buf).

-Peff

Quoted 16 lines

strbuf.c +2 -0

index 3939863cf3..0333aea261 100644
@@ -631,6 +631,8 @@ int strbuf_getcwd(struct strbuf *sb)
 #ifdef HAVE_GETDELIM
 int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
+	char *buf;
+	size_t alloc;
 	ssize_t r;
 
 	if (feof(fp))

@@ -639,12 +641,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	strbuf_reset(sb);
 

strbuf.c int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) · line 646

Quoted 6 lines

strbuf.c +2 -2

 	/* Translate slopbuf to NULL, as we cannot call realloc on it */
-	if (!sb->alloc)
-		sb->buf = NULL;
+	alloc = sb->alloc;
+	buf = alloc ? sb->buf : NULL;
 	errno = 0;

I actually was hoping that all lines in this hunk before this point can be removed, i.e., strbuf_release(sb), buf = NULL, alloc = 0.

Quoted 11 lines

strbuf.c +3 -3

@@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	 * we can just re-init, but otherwise we should make sure that our
 	 * length is empty, and that the result is NUL-terminated.
 	 */
-	if (!sb->buf)
+	if (!buf)
 		strbuf_init(sb, 0);
-	else
-		strbuf_reset(sb);
+	else {
+		sb->buf = buf;

strbuf.c int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) · line 682

Quoted 3 lines

strbuf.c +3 -0

+		sb->alloc = alloc;
+		strbuf_reset(&sb);
+	}

I do not get all these conditionals. This is an EOF code path; we have no data in buf to return. We resetted the caller's strbuf already. Can't we return buf (if allocated) to the system and return without doing any further damage to sb at this point?

I'm not quite sure what you mean. The function right now looks like:
...
But it's not quite enough to just do:

  free(buf);

Because "buf" is a copy of sb->buf,...

I may have phrased the idea very poorly. In short, the core of the idea is that we do not have to use the original content of the strbuf at all. I.e., "buf" does not have to be anything related to sb->buf.

I haven't measured to see what the exact cost is, but I know that looping over a strbuf (with a reset in the loop, or the implied reset from a getline call) is a common optimization trick that does have a measurable improvement for some cases.

I haven't measured to see what the exact cost is, but I know that looping over a strbuf (with a reset in the loop, or the implied reset from a getline call) is a common optimization trick that does have a measurable improvement for some cases.

          ssize_t r;
  
          if (feof(fp))
                  return EOF;
  
          strbuf_reset(sb);
  
          /* Translate slopbuf to NULL, as we cannot call realloc on it */
          if (!sb->alloc)
                  sb->buf = NULL;
          errno = 0;

I think the strbuf_reset() could go away even without any other changes. We always adjust sb->len in the end to match what happened with getdelim(), so there is no point in doing it up front.

Quoted 4 lines

          if (feof(fp))
                  return EOF;
  
          strbuf_reset(sb);

[...]

Quoted 3 lines

I think the strbuf_reset() could go away even without any other changes. We always adjust sb->len in the end to match what happened with getdelim(), so there is no point in doing it up front.

Yes. Same with the EOF check; getdelim(3) is (must be) prepared to handle that for us. An early return at the end of the file avoids the translate effort once per file, but adds the cost of checking for each line.

strbuf.c +6 -3

@@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	 * we can just re-init, but otherwise we should make sure that our
 	 * length is empty, and that the result is NUL-terminated.
 	 */
-	if (!sb->buf)
+	if (!buf)
 		strbuf_init(sb, 0);
-	else
-		strbuf_reset(sb);
+	else {
+		sb->buf = buf;
+		sb->alloc = alloc;
+		strbuf_reset(&sb);
+	}
 	return EOF;
 }
 #else

So I don't know that it makes anything simpler. We have to copy the values back into the strbuf either way, and we still have to handle restoring the strbuf invariants. Even the strbuf_init() case is still needed, because we don't know whether getdelim() just didn't allocate (in which case we could leave the strbuf alone) or if it actually ate the allocation we passed in (which was just a copy of sb->buf).

And yet this function can turn an empty strbuf into an allocated one without rolling it back on error, leaving code similar to this silly example here leaking:

	int copy_one_line(FILE *in, FILE *out, int term)
	{
		struct strbuf sb = STRBUF_INIT;
		if (strbuf_getwholeline(&sb, in, term))
			return -1;
		fwrite(sb.buf, 1, sb.len, out);
		strbuf_release(&sb);
		return 0;
	}

Some strbuf functions restore the original state in such a case by calling strbuf_release(), strbuf_getwholeline() doesn't. If we are OK with that then it could be simplified by growing the buffer upfront:

	int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
	{
		ssize_t r;

		strbuf_grow(sb, 0);
		errno = 0;
		r = getdelim(&sb->buf, &sb->alloc, term, fp);

Quoted 4 lines · René Scharfe

And yet this function can turn an empty strbuf into an allocated one without rolling it back on error, leaving code similar to this silly example here leaking:

Quoted 9 lines

	int copy_one_line(FILE *in, FILE *out, int term)
	{
		struct strbuf sb = STRBUF_INIT;
		if (strbuf_getwholeline(&sb, in, term))
			return -1;
		fwrite(sb.buf, 1, sb.len, out);
		strbuf_release(&sb);
		return 0;
	}

Yes, I almost pointed that out, but I think it's mostly a non-issue in practice because you'd generally call it multiple times (usually in a loop, but sometimes just multiple individual calls). And then you have to release if any call ever succeeded, which means either doing so after the loop ends or in a cleanup block.

Grepping for 'if (strbuf_get.*line', the closest I found was get_mail_commit_oid(), which reads a single line. It doesn't have an early return, though, since it has to clean up the FILE pointer anyway.

So I dunno. I don't think it's been a problem in practice, but I'm not opposed to future-proofing if it's easy to do.

I feel like there's a lot of discussion in this thread but we're not achieving anything practical.

If we do anything, I think it would be:

- drop the feof and reset at the top of the function, which are redundant

- make a noop read on an unallocated strbuf retain the unallocated state (your example above)

Quoted 2 lines

- make a noop read on an unallocated strbuf retain the unallocated state (your example above)

That makes the function conform to the convention of rolling back on error. This transactional behavior is a bit easier to understand. The non-getdelim(3) version doesn't do that, though. It returns whatever it got and leaves error checking and rollback to its callers.

getdelim(3) doesn't allow that -- it has no way to indicate the length of partial reads. If we are OK with throwing away partial lines then we better do that consistently in both versions? Sounds a bit messed up to bin perfectly good data just because some other platform has a fancy function that goes quiet when it stumbles. The alternative of having inconsistent behavior seems worse, though.

Could the function be rewritten differently, or maybe even made a little simpler? Perhaps, but who cares? The function has been largely untouched for a decade and the behavior is fine. And there are a bunch of pitfalls that a rewrite risks falling into.

Quoted 4 lines · Jeff King

Could the function be rewritten differently, or maybe even made a little simpler? Perhaps, but who cares? The function has been largely untouched for a decade and the behavior is fine. And there are a bunch of pitfalls that a rewrite risks falling into.

Well, my only interest in this codepath was to get rid of "if (!sb->buf)" so that I can lift the special case in the Coccinelle rule. Nothing else.

Here is my second try. strbuf_getwholeline() does not have to break strbuf invariants even tentatively. We just grab the guts of sb, let getdelim() possibly reallocate, and then return it in the normal case.

In the EOF code path, the only special thing we need is when we started with slopbuf[] and getdelim() allocated some bytes yet returned EOF. We are expected to free it before returning.

By the way, the big comment about xrealloc() in the middle, most of which is outside the post-context of the first hunk, should be updated, as our xrealloc() do not aggressively try to recover these days, if I understand correctly. I left it outside the scope of this patch, whose sole focus is to reduce the number of places in the codebase that check if sb->buf is NULL.