Subject: Re: [PATCH 5/9] vcs-svn: factor out usage of string_pool

From: Jonathan Nieder <[email-masked]>
Date: Sat, 19 Mar 2011 04:08:21 -0500

David Barr wrote:

> [Subject: vcs-svn: factor out usage of string_pool]

This actually means something like: use strbufs and strings instead of
interned strings for values of rev, dump, and node fields that happen
to be strings.  After this change, there are no more users of the
string-pool library left.

> @@ -71,14 +71,16 @@ static void reset_rev_ctx(uint32_t revision)
>  	rev_ctx.revision = revision;
>  	rev_ctx.timestamp = 0;
>  	strbuf_reset(&rev_ctx.log);
> -	rev_ctx.author = ~0;
> +	strbuf_reset(&rev_ctx.author);

Side note: should the default timestamp really be the epoch?  I'd
rather the default timestamp be the timestamp of the parent revision,
to make out-of-order dates a little less likely.

>  }
>  
> -static void reset_dump_ctx(uint32_t url)
> +static void reset_dump_ctx(const char *url)
>  {
> -	dump_ctx.url = url;
> +	strbuf_reset(&dump_ctx.url);
> +	if (url)
> +		strbuf_addstr(&dump_ctx.url, url);

Good, we keep our own copy of the url still.

> @@ -91,13 +93,15 @@ static void handle_property(const char *key, const char *val, uint32_t len,
>  			break;
>  		if (!val)
>  			die("invalid dump: unsets svn:log");
> -		/* Value length excludes terminating nul. */
> -		strbuf_add(&rev_ctx.log, val, len + 1);
> +		strbuf_reset(&rev_ctx.log);
> +		strbuf_add(&rev_ctx.log, val, len);

What is this change about?

> @@ -447,5 +456,4 @@ void svndump_reset(void)
>  {
>  	fast_export_reset();
>  	buffer_reset(&input);
> -	pool_reset();

strbuf_release(&dump_ctx.url)?

Likewise for dump_ctx.uuid and the other one.

Thanks; except as noted above this looks good.