Re: [PATCH][merge-tracking]Data loss due to line buffered SAX stream on mergeinfo retrieval.

On Mon, 20 Nov 2006, Kamesh Jayachandran wrote:
...
> You could see this issue with url to url copy over ra_dav if source
> of copy is having multiple mergeinfos.

Nice find, Kamesh! I (and DannyB, apparently) didn't realize that our
SAX CDATA handlers were invoked once per line. Given that's the case,
we need to handle merge info with both:

o A single merge source (one line)
o Multiple merge sources (N lines, one per source)

I've committed a similar fix in r22369, which also addresses my
comments on the patch below.

> --- subversion/libsvn_ra_dav/mergeinfo.c (revision 22315)
> +++ subversion/libsvn_ra_dav/mergeinfo.c (working copy)
> @@ -127,6 +127,8 @@
> {
> struct mergeinfo_baton *mb = baton;
> apr_size_t nlen = len;
> + char tmpc;
> + char *mutable_cdata = (char*)cdata;
>
> switch (state)
> {
> @@ -134,7 +136,13 @@
> mb->curr_path = apr_pstrndup(mb->pool, cdata, nlen);
> break;
> case ELEM_merge_info_info:
> - mb->curr_info = apr_pstrndup(mb->pool, cdata, nlen);
> + /* if the data is having multiple lines we need to concatenate the
> + * current data with the previous one.
> + */
> + tmpc = mutable_cdata[nlen];

This will index off the end of the "mutable_cdata" string, which is
only "len" character long, rather than the "len + 1" characters this
would require.

Looks like an attempt to avoid copying "cdata"...

> + mutable_cdata[len]='\0';

We did it again, only this time we clobbered something else's memory.
This time, we used "len" rather than "nlen" as our offset variable.
Which we use doesn't really matter, but we should be consistent.

Spacing is also missing from around the "=" character.

> + mb->curr_info = apr_pstrcat(mb->pool, mb->curr_info, cdata, NULL);

What happens when mb->curr_info hasn't been set yet?

> + mutable_cdata[nlen] = tmpc;
> break;

Now we attempt to restore the data we previously clobbered, again
changing the offset variable we're using.