Hi, Erik!
On 12/24/05, Erik Huelsmann <ehuels@gmail.com> wrote:
> In one of the changes I have in my working copy (and posted recently
> right here), I created a new libsvn_wc interface to return a stream
> for access to wc files. The interface (svn_wc_translated_stream())
> returns a stream from a wc path or text base.
>
> Thinking about the future - especially issue #908 (compressed text
> bases) - this interface probably conflicts with changes we may want to
> make for that issue: to make compression transparent to other parts of
> libsvn_wc, the only solution to create modularity is by returning a
> stream - either compressed or uncompressed. Currently, our file
> opening routines open file handles for reading / writing raw apr
> files.
>
> So, while not immediately addressing issue #908, I would like to keep
> options open for developments like these. Does anybody have a better
> idea than to apply the patch below?
I don't understand your question completly or rather I don't see
problem here. But I have some comments to patch:
>
> Log:
> [[[
> Create svn_wc_translated_stream() in order to be able to stack
> additional transformations on top of an APR file.
>
> This patch is part of a larger change which reduces the number of full file
> reads in svn_wc_transmit_text_deltas().
>
> ]]]
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 17918)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -3262,6 +3262,33 @@
> svn_boolean_t force_repair,
> apr_pool_t *pool);
>
> +
> +/** Returns a @a stream allocated in @a pool with access to the given
> + * @a path taking the file properties from @a versioned_file using
> + * @a adm_access.
> + *
> + * When translation from normal form is requested
> + * (@c SVN_WC_TRANSLATE_FROM_NF is specified in @a flags), @a path
> + * is used as target path and stream read operations are not supported.
> + * Conversely, if translation to normal form is requested
> + * (@c SVN_WC_TRANSLATE_TO_NF is specified in @a flags), @a path is
> + * used as source path and stream write operations are not supported.
> + *
> + * The @a flags are the same constants as those used for
> + * svn_wc_translated_file().
> + *
> + *
> + *
> + * @since New in 1.4.
> + */
> +svn_error_t *
> +svn_wc_translated_stream (svn_stream_t **stream,
> + const char *path,
> + const char *versioned_file,
> + svn_wc_adm_access_t *adm_access,
> + apr_uint32_t flags,
> + apr_pool_t *pool);
> +
>
> /* Text/Prop Deltas Using an Editor */
>
> Index: subversion/libsvn_wc/translate.c
> ===================================================================
> --- subversion/libsvn_wc/translate.c (revision 17918)
> +++ subversion/libsvn_wc/translate.c (working copy)
> @@ -41,7 +41,72 @@
>
> #include "svn_private_config.h"
>
> +
> +
> +static svn_error_t *
> +read_handler_unsupported (void *baton, char *buffer, apr_size_t *len)
> +{
> + abort();
> +}
> +
> +static svn_error_t *
> +write_handler_unsupported (void *baton, const char *buffer, apr_size_t *len)
> +{
> + abort();
> +}
> +
> svn_error_t *
> +svn_wc_translated_stream (svn_stream_t **stream,
> + const char *path,
> + const char *versioned_file,
> + svn_wc_adm_access_t *adm_access,
> + apr_uint32_t flags,
> + apr_pool_t *pool)
> +{
> + svn_subst_eol_style_t style;
> + const char *eol;
> + apr_hash_t *keywords;
> + svn_boolean_t special;
> + svn_boolean_t to_nf = flags & SVN_WC_TRANSLATE_TO_NF;
> +
> + SVN_ERR (svn_wc__get_eol_style (&style, &eol, versioned_file,
> + adm_access, pool));
> + SVN_ERR (svn_wc__get_keywords (&keywords, versioned_file,
> + adm_access, NULL, pool));
Variables keywords, eol and eol_style used only when !special, so for
me better to move these variables to else body. I didn't see
recommendation in hacking about this, but I prefer to minimize scope
of variables in such case: it is improves readability and performance
(a bit of course).
> + SVN_ERR (svn_wc__get_special (&special, versioned_file, adm_access, pool));
> +
> + if (special)
> + SVN_ERR (svn_subst_stream_from_specialfile (stream, path, pool));
> + else
> + {
> + apr_file_t *file;
> + svn_boolean_t repair_forced = flags & SVN_WC_TRANSLATE_FORCE_EOL_REPAIR;
> +
> + SVN_ERR (svn_io_file_open (&file, path,
> + to_nf ? (APR_READ | APR_BUFFERED)
> + : (APR_CREATE | APR_WRITE | APR_BUFFERED),
> + APR_OS_DEFAULT, pool));
> +
> + *stream = svn_stream_from_aprfile2 (file, FALSE, pool);
> + if (to_nf)
> + SVN_ERR (svn_subst_stream_translated_to_normal_form
> + (stream, *stream, style, eol, repair_forced,
> keywords, pool));+ else
> + *stream = svn_subst_stream_translated
> + (*stream, eol, repair_forced, keywords, TRUE, pool);
> + }
> +
> + /* Enfore our contract, because a specialfile stream won't */
> + if (to_nf)
> + svn_stream_set_write (*stream, write_handler_unsupported);
> + else
> + svn_stream_set_read (*stream, read_handler_unsupported);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +svn_error_t *
> svn_wc_translated_file2 (const char **xlated_path,
> const char *src,
> const char *versioned_file,
>
I consider svn_wc_translated_file2() should use
svn_wc_translated_stream() for implemention to eliminate duplicated
and uncovered by test suite source code. Yes, I see that
svn_wc_translated_file2() is no-op in some cases, so we loose *some*
performance, but I consider it's acceptable tradeoff.
--
Ivan Zhakov
Received on Wed Dec 28 15:52:30 2005