Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:

On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:

quoted

On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:

quoted

+	strbuf_grow(sb, len);
+	strbuf_addf(sb, "%u %s=", len, keyword);
+	strbuf_add(sb, value, valuelen);
+	strbuf_addch(sb, '\n');
 }

This entire function can be collapsed to just:

	strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);

  yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds
a \0 before. And the strbuf_grow has sense because the extend policy at
snprintf time is optimistic: we try to write, and if it didn't fit, we
try again. So there is a huge benefit if we have a clue of the final
size.

Yeah, my strbuf_addf() suggestion doesn't work, since the format seems
to indicate the embedded NULs indeed is allowed (the %u is the length of
the value), so *.% could truncate data.

quoted

quoted

-	memcpy(path.buf, base, baselen);
-	memcpy(path.buf + baselen, filename, filenamelen);
-	path.len = baselen + filenamelen;
-	path.buf[path.len] = '\0';
+	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
+	strbuf_reset(&path);

Does strbuf_reset() do anything here?

quoted

+	strbuf_add(&path, base, baselen);

  Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !

But where was length set to non-zero?  path is initialized on entry to
the function, and strbuf_grow() just increases the allocation, not
length, right?

Kristian