Erik Huelsmann wrote:
> lundblad suggested I change blame not to use the extra intermediate
> file I made it use for certain EOL style settings. In order to do so,
> I need a diff implementation which runs on streams (I'm working on
> it!), and the patch below, which translates a stream on-the-fly.
>
> I'm hoping for comments.
+1 in principle. I haven't got time to review it this week. I just had a
quick browse through the diff and made a few comments.
> [[[
> Prepare to eliminate the - recently introduced - intermediate file for blame.
>
> To prevent the creation of an extra intermediate file, on the fly recoding
> of EOLs is required. This stream class provides that.
That mostly talks about your plans for "blame". Please could the log message
first and foremost say what the patch does, and just mention the intended use
for it briefly afterwards as a justification?
> Index: subversion/include/svn_subst.h
> ===================================================================
[...]
> +/** Return a stream which performs eol translation and keyword
> + * expansion much like svn_subst_translate_stream3() except that
> + * it's done on-the-fly when reading or writing to @a stream.
Could you remove "much" to make it more definite?
It might be worth saying explicitly that it does the same translation on
reading as on writing, not the opposite translation. It may be impossible to
do otherwise, with the given parameters, but I found that a little confusing at
first, wondering how it could know which way to translate.
> + *
> + * @since New in 1.4
> + */
> +svn_stream_t *
> +svn_subst_stream_translated (svn_stream_t *stream,
> + const char *eol_str,
> + svn_boolean_t repair,
> + apr_hash_t *keywords,
> + svn_boolean_t expand);
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
[...]
> +svn_subst_stream_translated (svn_stream_t *stream,
> + const char *eol_str,
> + svn_boolean_t repair,
> + apr_hash_t *keywords,
> + svn_boolean_t expand,
> + apr_pool_t *pool)
> +{
> + struct translated_stream_baton *baton = apr_palloc (pool, sizeof (*baton));
> + svn_stream_t *s = svn_stream_create (baton, pool);
> +
> + /* Make sure EOL_STR and KEYWORDS are allocated in POOL, as
> + required by create_translation_baton() */
> + if (eol_str)
> + eol_str = apr_pstrdup (pool, eol_str);
> + if (keywords)
> + keywords = apr_hash_copy (pool, keywords);
That's only a shallow copy of the hash - is that any good?
> @@ -1256,9 +1469,19 @@
> src_stream = svn_stream_from_aprfile (s, pool);
> dst_stream = svn_stream_from_aprfile (d, pool);
>
> + /* No reason to choose either source or destination,
> + other than to exercise all code paths */
> + if (expand)
> + src_stream = svn_subst_stream_translated (src_stream,
> + eol_str, repair,
> + keywords, expand, pool);
> + else
> + dst_stream = svn_subst_stream_translated (dst_stream,
> + eol_str, repair,
> + keywords, expand, pool);
It's not this function's job to exercise your new code paths! By all means try
this as a form of regression test but, for committing to the repository, if you
can make this function simpler by cutting out the "if" and one of the paths,
please do, although it seems to me that it would be even simpler, and thus
better, if you just left it how it was. Presumably you will soon add some code
that really needs the translated stream, and that will exercise it.
> +
> /* Translate src stream into dst stream. */
> - err = svn_subst_translate_stream3 (src_stream, dst_stream, eol_str,
> - repair, keywords, expand, pool);
> + err = svn_stream_copy (src_stream, dst_stream, pool);
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Nov 29 18:51:35 2005