Ivan Zhakov writes:
> Might be I superfluously strict for self, but I don't sure in some
> code cosmetics aspects in my patch:
>
Looks good in general. Some nits below.
> [[
> Use stream translation API for check file modifications.
>
s/for check/to check for/
> * subversion/libsvn_subr/subst.c
> (detranslated_stream_special): Factor out implementation from
> detranslate_special_file().
> (detranslate_special_file): Use factored out detranslated_stream_special().
> (svn_subst_detranslated_stream): New function. Returns stream which performs
> translation file from working copy form to normal form. Handles
s/translation file/translation of file/
(I don't think you need to describe the function, at least not in this
detail in the log message, but there are varying opinions on that
one...)
> special files correctly.
>
> * subversion/libsvn_wc/questions.c
> (compare_streams): Helper for compare streams content.
> (svn_wc__versioned_file_modcheck): If translation is required than
s/than/then/
(Or just "Avoid creating temporary file if...")
> compare versioned and
> base files using streamed translation API, instead of creating
> temporary files.
> ]]
>
> +/**
> + * Sets a pointer STREAM_P to stream which performs converting file SRC in
> + * working copy form to normal form.
> + *
An alternative:
Set @a *stream_p to a stream that translates the file @a src from
working copy form to normal form, allocated in @a pool.
> + * The values specified for @a eol_style, @a *eol_str, @a keywords and
> + * @a special, should be the ones used to translate the file to its
> + * working copy form. Usually, these are the values specified by the
> + * user in the files' properties.
> + *
> + * The stream returned is allocated in @a pool.
> + *
If you choose my alternative, this para could go.
> + * @since New in 1.4
> + *
I put a period at the end, but we're not consistent.
> + */
> +svn_error_t *
> +svn_subst_detranslated_stream(svn_stream_t **stream_p,
> + const char *src,
> + svn_subst_eol_style_t eol_style,
> + const char *eol_str,
> + svn_boolean_t repair,
The repair argument isn't mentioned in the docstring, but do we need
it for detranslating streams?
> + apr_hash_t *keywords,
> + svn_boolean_t special,
> + apr_pool_t *pool);
> +
>
> /* EOL conversion and character encodings */
>
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 18619)
> +++ subversion/libsvn_subr/subst.c (working copy)
> @@ -1200,7 +1200,78 @@
> return SVN_NO_ERROR;
> }
>
> +/* Given a special file at SRC, sets TRANSLATED_STREAM_P to stream
> + with textual representation of it. Perform all allocations in POOL. */
s/sets/set/
s/to stream/to a stream/
> +static svn_error_t *
> +detranslated_stream_special(const char *src,
> + svn_stream_t **translated_stream_p,
> + apr_pool_t *pool)
Could put TRANSLATED_STREAM_P argument first.
>
> -/* Given a special file at SRC, generate a textual representation of
> - it in a normal file at DST. Perform all allocations in POOL. */
> static svn_error_t *
> detranslate_special_file(const char *src,
> const char *dst,
> apr_pool_t *pool)
Why remove that docstring?
> Index: subversion/libsvn_wc/questions.c
> ===================================================================
> --- subversion/libsvn_wc/questions.c (revision 18619)
> +++ subversion/libsvn_wc/questions.c (working copy)
> @@ -214,7 +214,35 @@
> return SVN_NO_ERROR;
> }
>
> +/** Set @a *same to non-zero if @a stream1 and @a stream2 have the same
> + * contents, else set it to zero. Use @a pool for temporary allocations.
> + */
Don't use doxygen comments for internal functions.
> +static svn_error_t *
> +compare_streams(svn_boolean_t *same,
> + svn_stream_t *stream1,
> + svn_stream_t *stream2,
> + apr_pool_t *pool)
> +{
> + char buf1[BUFSIZ], buf2[BUFSIZ];
If I remember correctly, BUFSIZ is very low on Windows (512 or
something). Would it make sense to use SVN_STREAM__CHUNK_SIZE here
instead?
> + apr_size_t bytes_read1 = BUFSIZ, bytes_read2 = BUFSIZ;
> +
> + *same = TRUE; /* assume TRUE, until disproved below */
> + while (bytes_read1 == BUFSIZ && bytes_read1 == BUFSIZ)
> + {
Indentation.
> @@ -224,26 +252,65 @@
> apr_pool_t *pool)
> {
> svn_boolean_t same;
> - const char *tmp_vfile;
> + svn_subst_eol_style_t eol_style;
> + const char *eol_str;
> + apr_hash_t *keywords;
> + svn_boolean_t special;
>
> - if (compare_textbases)
> - SVN_ERR(svn_wc_translated_file2
> - (&tmp_vfile, versioned_file,
> - versioned_file, adm_access,
> - SVN_WC_TRANSLATE_TO_NF,
> - pool));
> - else
> - SVN_ERR(svn_wc_translated_file2
> - (&tmp_vfile, base_file,
> - versioned_file, adm_access,
> - SVN_WC_TRANSLATE_FROM_NF,
> - pool));
> + SVN_ERR(svn_wc__get_eol_style(&eol_style, &eol_str, versioned_file,
> + adm_access, pool));
> + SVN_ERR(svn_wc__get_keywords(&keywords, versioned_file,
> + adm_access, NULL, pool));
Hmmm... Reas and parses properties file twice, but that's another
improvement:-)
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Feb 26 23:02:01 2006