Re: [PATCH] Simplify strbuf uses in archive-tar.c using the proper functions.
On Wed, Sep 19, 2007 at 08:06:24AM +0000, Junio C Hamano wrote:
Show 6 quoted lines
> 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.
Show 5 quoted lines
> 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.
Show 17 quoted lines
> > + 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
Pierre Habouzit <madcoder@debian.org>