Hi Dan,
I committed some miscellaneous followups to this in r18365, r18366,
and r18371. They're fairly innocuous, but I've got a few further
questions and comments, below:
> --- trunk/subversion/libsvn_delta/svndiff.c (original)
> +++ trunk/subversion/libsvn_delta/svndiff.c Mon Feb 6 21:12:56 2006
> @@ -24,7 +24,16 @@
> #include "delta.h"
> #include "svn_pools.h"
> #include "svn_private_config.h"
> +#include <zlib.h>
>
I think we tend to list system headers before private headers, though
it's probably not worth changing now.
> +/* 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.
> + 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?
> +
> + /* 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?
> 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.
> @@ -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.
> --- 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?
(Maybe we do, but we should definitely mention the consequences in the
1.4.x release notes if so).
Regards,
Malcolm
---------------------------------------------------------------------
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:24:17 2006