On 2/27/06, Peter Lundblad <peter@famlundblad.se> wrote:
> 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.
Thanks, Peter catching so many nits :)
>
> > [[
> > 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/
Ok.
>
> (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...)
I prefer describe some key things in log message.
>
> > 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/
Done.
>
> (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.
>
Ok.
> > + * 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.
Ok.
>
> > + * @since New in 1.4
> > + *
> I put a period at the end, but we're not consistent.
Ok.
>
> > + */
> > +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?
I've renamed repair to always_repair to be consistent with
svn_subst_translate_to_normal_form. Docstring added.
What do you think: might be better rename function
svn_subst_detranslated_stream() to svn_subst_stream_detranslated() to
be consistent with svn_subst_stream_translated() ?
>
> > + 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/
Ok.
>
> > +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.
Ok.
>
> >
> > -/* 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?
>
My fault. Fixed.
> > 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?
Ok. I have renamed compare_streams to svn_stream_contents_same and move to
streams.c. Buffers are made of size SVN_STREAM__CHUNK_SIZE and
allocated from pool.
>
> > + apr_size_t bytes_read1 = BUFSIZ, bytes_read2 = BUFSIZ;
> > +
> > + *same = TRUE; /* assume TRUE, until disproved below */
> > + while (bytes_read1 == BUFSIZ && bytes_read1 == BUFSIZ)
> > + {
>
> Indentation.
Ok.
But not only identation! I've duplicated bytes_read1 two times instead
of bytes_read1 and bytes_read2. Bad copy-pasting :). Fixed.
> > @@ -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:-)
Yes, it is another topic.
Because I had so many mistakes, I post revised patch and log message.
I'll commit it today evening if nobody else comment it. Thanks again!
[[
Use stream translation API to check for file modifications.
* subversion/include/svn_io.h
* subversion/libsvn_subr/streams.c
(svn_stream_contents_same): New function for compare streams content.
* subversion/include/svn_subst.h
(svn_subst_detranslated_stream): New function declaration.
* 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 of file from working copy form to normal form. Handles
special files correctly.
* subversion/libsvn_wc/questions.c
(svn_wc__versioned_file_modcheck): If translation is required then
compare versioned and base files using streamed translation API,
instead of creating temporary files.
]]
--
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Feb 27 11:12:56 2006