Re: [PATCH] Simplify strbuf uses in archive-tar.c using the proper functions.
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:35
Pierre Habouzit [off-list ref] writes:
quoted hunk
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.
+ 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);