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

["Kristian Høgsberg" <krh@redhat.com>]

On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> 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
> @@ -78,19 +78,6 @@ static void write_trailer(void)
>  	}
>  }
>  
> -static void strbuf_append_string(struct strbuf *sb, const char *s)
> -{
> -	int slen = strlen(s);
> -	int total = sb->len + slen;
> -	if (total + 1 > sb->alloc) {
> -		sb->buf = xrealloc(sb->buf, total + 1);
> -		sb->alloc = total + 1;
> -	}
> -	memcpy(sb->buf + sb->len, s, slen);
> -	sb->len = total;
> -	sb->buf[total] = '\0';
> -}
> -
>  /*
>   * pax extended header records have the format "%u %s=%s\n".  %u contains
>   * the size of the whole string (including the %u), the first %s is the
> @@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)
>  static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,
>                                       const char *value, unsigned int valuelen)
>  {
> -	char *p;
> -	int len, total, tmp;
> +	int len, tmp;
>  
>  	/* "%u %s=%s\n" */
>  	len = 1 + 1 + strlen(keyword) + 1 + valuelen + 1;
>  	for (tmp = len; tmp > 9; tmp /= 10)
>  		len++;
>  
> -	total = sb->len + len;
> -	if (total > sb->alloc) {
> -		sb->buf = xrealloc(sb->buf, total);
> -		sb->alloc = total;
> -	}
> -
> -	p = sb->buf;
> -	p += sprintf(p, "%u %s=", len, keyword);
> -	memcpy(p, value, valuelen);
> -	p += valuelen;
> -	*p = '\n';
> -	sb->len = total;
> +	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);

>  
>  static unsigned int ustar_header_chksum(const struct ustar_header *header)
> @@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
>  	struct strbuf ext_header;
>  
>  	memset(&header, 0, sizeof(header));
> -	ext_header.buf = NULL;
> -	ext_header.len = ext_header.alloc = 0;
> +	strbuf_init(&ext_header);

Just use your STRBUF_INIT macro?

>  	if (!sha1) {
>  		*header.typeflag = TYPEFLAG_GLOBAL_HEADER;
> @@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
>  
>  	if (ext_header.len > 0) {
>  		write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
> -		free(ext_header.buf);
>  	}

Remove excess braces?

> +	strbuf_release(&ext_header);
>  	write_blocked(&header, sizeof(header));
>  	if (S_ISREG(mode) && buffer && size > 0)
>  		write_blocked(buffer, size);
> @@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
>  static void write_global_extended_header(const unsigned char *sha1)
>  {
>  	struct strbuf ext_header;
> -	ext_header.buf = NULL;
> -	ext_header.len = ext_header.alloc = 0;
> +
> +	strbuf_init(&ext_header);

STRBUF_INIT macro?

>  	strbuf_append_ext_header(&ext_header, "comment", sha1_to_hex(sha1), 40);
>  	write_entry(NULL, NULL, 0, ext_header.buf, ext_header.len);
> -	free(ext_header.buf);
> +	strbuf_release(&ext_header);
>  }
>  
>  static int git_tar_config(const char *var, const char *value)
> @@ -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));
> +	strbuf_reset(&path);

Does strbuf_reset() do anything here?

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