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

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

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.

  I would not change a thing.

quoted

+	strbuf_init(&ext_header);

Just use your STRBUF_INIT macro?

  Many people dilike it, I'm not sure it's a good idea either, and the
performance hit should be negligible, if it's not, then we can still
make the _init() function an inline.

quoted

 	if (ext_header.len > 0) {
 		write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
-		free(ext_header.buf);
 	}

Remove excess braces?

  bah, I don't like to strip braces so I won't do that, else you end up
with stupidities like:

  if (foo)
    // bar();

  do_some_very_important_stuff();

  Call me paranoid but well, it saved me so many times ...

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 !

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org