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:
> > +	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.

> > +	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.

> >  	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 ...

> > -	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?
> 
> > +	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