> I think we tend to list system headers before private headers, though
> it's probably not worth changing now.
Okay.
>
> > +/* If IN is a string that is > MIN_COMPRESS_SIZE, zlib compress it and
> > + place the result in OUT, with an integer preprended specifying the
> > + original size. If IN is not > MIN_COMPRESS_SIZE, or compression
> > + caused the string to be larger, OUT will be a copy of IN with the
> > + size prepended as an integer. */
> > +
> > +static svn_error_t *
> > +zlib_encode (svn_stringbuf_t *in, svn_stringbuf_t *out)
> > +{
>
> I updated the doc-string in r18366, mainly because strings equal to
> MIN_COMPRESS_SIZE will be compressed. I also changed the comments to
> note that the plaintext will be used if the compressed text is _equal_
> in size to the uncompressed text, since otherwise zlib_decode() won't
> be able to distinguish this condition.
Whoops, you are correct.
>
> > + if (compress2 ((unsigned char *)out->data + intlen, &endlen,
> > + (const unsigned char *)in->data, in->len,
> > + SVNDIFF1_COMPRESS_LEVEL) != Z_OK)
> > + return svn_error_create (SVN_ERR_SVNDIFF_INVALID_COMPRESSED_DATA,
> > + NULL,
> > + _("compression of svndiff data failed"));
>
> Is SVN_ERR_SVNDIFF_INVALID_COMPRESSED_DATA an appropriate error return
> here, or should we create an additional code?
Ideally, we would transform the error return from zlib into something we
can understand, which would require a bunch of codes. However, i don't
believe compression really ever fails unless it runs out of memory, and
because of it's bounded memory requirements, this should pretty much
never happen.
So i more or less punted, since this error should occur pretty much
never. :)
>
> > +
> > + /* Compression didn't help :(, just append the original text */
> > + if (endlen > in->len)
> > + {
> > + svn_stringbuf_appendstr (out, in);
> > + return SVN_NO_ERROR;
> > + }
>
> Presumably that condition should be '(endlen >= in->len)', so that
> zlib_decode() can unconditionally determine whether the data is compressed
> or not?
correct.
>
> > static svn_error_t *
> > window_handler (svn_txdelta_window_t *window, void *baton)
> > @@ -105,17 +156,23 @@
> > struct encoder_baton *eb = baton;
> > apr_pool_t *pool = svn_pool_create (eb->pool);
>
> > /* Write out the window. */
> > len = header->len;
> > - err = svn_stream_write (eb->output, header->data, &len);
> > - if (err == SVN_NO_ERROR && instructions->len > 0)
> > + SVN_ERR (svn_stream_write (eb->output, header->data, &len));
> > + if (instructions->len > 0)
> > {
>
> >
> > svn_pool_destroy (pool);
> > - return err;
> > + return SVN_NO_ERROR;
> > }
>
> The original code seems to ensure that the temporary subpool is destroyed
> in the event of an error, while the new code doesn't. I'm not sure
> whether this is particularly important though, especially since errors
> at this point aren't particularly likely.
I remember discussing this with someone when the branch was in
development. The answer I got was "but if it error'd out here, you are
going to get all the way back up the chain anyway, and something will
destroy the parent pool, so it shouldn't matter".
I am indifferent about whether we should destroy the pool ourselves in
the error case or not, leaning towards "we should".
>
> > @@ -650,7 +810,8 @@
> >
> > /* Read a window header from STREAM and check it for integer overflow. */
> > static svn_error_t *
> > -read_window_header (svn_stream_t *stream, svn_filesize_t *sview_offset,
> > +read_window_header (svn_stream_t *stream, unsigned int version,
> > + svn_filesize_t *sview_offset,
> > apr_size_t *sview_len, apr_size_t *tview_len,
> > apr_size_t *inslen, apr_size_t *newlen)
> > {
>
> Why add the version argument? It's not used anywhere, so it just bulks
> out the diff.
The version argument was originally there so we could issue an error if
the version number was higher than we knew how to handle. I later
removed the code to do that, and forgot to remove the argument.
If we want that error, i'm happy to readd it, if not, we should
certainly remove the argument (which should be int svndiff_version
anyway).
>
> > --- trunk/subversion/libsvn_repos/dump.c (original)
> > +++ trunk/subversion/libsvn_repos/dump.c Mon Feb 6 21:12:56 2006
> > @@ -148,7 +148,7 @@
> > /* Compute the delta and send it to the temporary file. */
> > SVN_ERR (svn_fs_get_file_delta_stream (&delta_stream, oldroot, oldpath,
> > newroot, newpath, pool));
> > - svn_txdelta_to_svndiff (temp_stream, pool, &wh, &whb);
> > + svn_txdelta_to_svndiff2 (temp_stream, pool, &wh, &whb, 1);
> > SVN_ERR (svn_txdelta_send_txstream (delta_stream, wh, whb, pool));
> >
> > /* Get the length of the temporary file and rewind it. */
> >
>
> This means that 'svnadmin dump --deltas' dumps won't be loadable by
> 1.3.x clients. Do we really want to do that?
I'm not sure.
Do people often dump and then load back into older versions?
If so, we probably should add a --no-svndiff1 option to dump as well.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 7 19:44:40 2006