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

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

On Wed, Sep 19, 2007 at 08:06:24AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> > +	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> 
> Where are you getting the MAX() macro from?  On my Linux box
> it appears that <sys/params.h> happens to define it but I do not
> think that is something we can rely upon portably.

  indeed.

> Moreover, isn't this allocation wrong?  I thought "grow" was
> about "we want this much more in addition to the existing
> length", not "reserve at least this much", and this "path" is a
> static that will keep the buffer and length from the previous
> invocation.

  I'm a nitwit, the strbuf_reset() should be done _before_ the grow
indeed.

> > +	strbuf_reset(&path);
> > +	strbuf_add(&path, base, baselen);
> > +	strbuf_add(&path, filename, filenamelen);
> >  	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
> > -		strbuf_append_string(&path, "/");
> > +		strbuf_addch(&path, '/');
> >  		buffer = NULL;
> >  		size = 0;
> >  	} else {
> 
> Having said that, I suspect that the preallocation does not
> really matter in practice.  How about doing something like:
> 
> 	strbuf_reset(&path);
>         strbuf_grow(&path, PATH_MAX);
>         strbuf_add(&path, base, baselen);
>         strbuf_add(&path, filename, filenamelen);

  yeah, I was about to propose the same, I'll add a patch in my current
series to fix that this way.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]