Re: [PATCH] Simplify strbuf uses in archive-tar.c using the proper functions.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 19, 2007, 08:06 UTC
- Message-ID
- <7v8x73qdtr.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <11890199232128-git-send-email-madcoder@debian.org>
Pierre Habouzit <madcoder@debian.org> writes:
Show 38 quoted lines
> This is just cleaner way to deal with strbufs, using its API rather than
> reinventing it in the module (e.g. strbuf_append_string is just the plain
> strbuf_addstr function, and it was used to perform what strbuf_addch does
> anyways).
> ---
> archive-tar.c | 65 ++++++++++++++-------------------------------------------
> 1 files changed, 16 insertions(+), 49 deletions(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index a0763c5..c84d7c0 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1,
> const char *base, int baselen,
> const char *filename, unsigned mode, int stage)
> {
> - static struct strbuf path;
> + static struct strbuf path = STRBUF_INIT;
> int filenamelen = strlen(filename);
> void *buffer;
> enum object_type type;
> unsigned long size;
>
> - if (!path.alloc) {
> - path.buf = xmalloc(PATH_MAX);
> - path.alloc = PATH_MAX;
> - path.len = path.eof = 0;
> - }
> - if (path.alloc < baselen + filenamelen + 1) {
> - free(path.buf);
> - path.buf = xmalloc(baselen + filenamelen + 1);
> - path.alloc = baselen + filenamelen + 1;
> - }
> - 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));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.
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.
Show 9 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);