You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/04/22 15:49:15 UTC
svn commit: r936844 - in /subversion/trunk/subversion: libsvn_subr/subst.c
tests/libsvn_subr/stream-test.c
Author: stsp
Date: Thu Apr 22 13:49:14 2010
New Revision: 936844
URL: http://svn.apache.org/viewvc?rev=936844&view=rev
Log:
Hopefully fix issue #3616, "Mark/seek in svn_subst_stream_translated() -
needs to restore translation state"
"Hopefully" because an apparent bug in the translation stream prevents
a new test which attempts to verify the fix from passing.
* subversion/libsvn_subr/subst.c
(keywords_hash_deep_copy, dup_translation_baton): New helper functions.
(mark_translated_t): New type.
(translated_stream_mark, translated_stream_seek): Save translation state
upon mark, and restore it upon seek.
(svn_subst_stream_translated): Drive-by comment fix, and call the new
keywords_hash_deep_copy() helper which is based on code which used to
live here.
* subversion/tests/libsvn_subr/stream-test.c
(): Include svn_subst.h.
(test_funcs): Add new test.
(test_stream_seek_translated): New test. Currently XFail because of
aforementioned bug, which was discussed on IRC as follows:
<stsp> svn_subst_stream_translated() does not expand keywords if
they're not read in their entirety by the caller
<stsp> is that a bug?
<stsp> i would expect that, if I have the input containing a keyword
such as One$MyKeyword$Two, which expands to "my keyword expanded",
and I read up to the position of the character K, the result would
be "Onemy"
<stsp> but what is actually returned from svn_stream_read is One$My
<stsp> that makes setting a mark within a keyword rather pointless
<julianf> stsp: ew, that's an ugly bug in stream_translated.
<julianf> That's just waiting to blow up.
<stsp> julianf, yeah I think it should read ahead, expand the keyword,
and return part of the expansion
<julianf> +1
Review by: julianfoad
philip
Modified:
subversion/trunk/subversion/libsvn_subr/subst.c
subversion/trunk/subversion/tests/libsvn_subr/stream-test.c
Modified: subversion/trunk/subversion/libsvn_subr/subst.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/subst.c?rev=936844&r1=936843&r2=936844&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/subst.c (original)
+++ subversion/trunk/subversion/libsvn_subr/subst.c Thu Apr 22 13:49:14 2010
@@ -839,6 +839,43 @@ create_translation_baton(const char *eol
return b;
}
+/* Create a deep copy of the KEYWORDS hash, allocated in RESULT_POOL,
+ * and return a pointer to it. */
+static apr_hash_t *
+keywords_hash_deep_copy(const apr_hash_t *keywords, apr_pool_t *result_pool)
+{
+ apr_hash_t *copy = apr_hash_make(result_pool);
+ apr_hash_index_t *hi;
+
+ for (hi = apr_hash_first(result_pool, keywords);
+ hi; hi = apr_hash_next(hi))
+ {
+ const void *key;
+ void *val;
+
+ apr_hash_this(hi, &key, NULL, &val);
+ apr_hash_set(copy, apr_pstrdup(result_pool, key),
+ APR_HASH_KEY_STRING,
+ svn_string_dup(val, result_pool));
+ }
+
+ return copy;
+}
+
+/* Create a deep copy of TRANSLATION_BATON, allocated in RESULT_POOL,
+ * and return a pointer to it. */
+static struct translation_baton *
+dup_translation_baton(const struct translation_baton *translation_baton,
+ apr_pool_t *result_pool)
+{
+ struct translation_baton *b = apr_palloc(result_pool, sizeof(*b));
+
+ *b = *translation_baton;
+ b->keywords = keywords_hash_deep_copy(translation_baton->keywords,
+ result_pool);
+ return b;
+}
+
/* Translate eols and keywords of a 'chunk' of characters BUF of size BUFLEN
* according to the settings and state stored in baton B.
*
@@ -1164,13 +1201,40 @@ translated_stream_reset(void *baton)
return svn_error_return(err);
}
+/* svn_stream_mark_t for translation streams. */
+typedef struct
+{
+ /* Saved translation state. */
+ struct translated_stream_baton *saved_baton;
+
+ /* Mark set on the underlying stream. */
+ svn_stream_mark_t *mark;
+} mark_translated_t;
+
/* Implements svn_io_mark_fn_t. */
static svn_error_t *
translated_stream_mark(void *baton, svn_stream_mark_t **mark, apr_pool_t *pool)
{
+ mark_translated_t *mark_translated;
struct translated_stream_baton *b = baton;
+ struct translated_stream_baton *sb;
+
+ mark_translated = apr_palloc(pool, sizeof(*mark_translated));
+ SVN_ERR(svn_stream_mark(b->stream, &mark_translated->mark, pool));
+
+ /* Save translation state. */
+ sb = apr_pcalloc(pool, sizeof(*sb));
+ sb->in_baton = dup_translation_baton(b->in_baton, pool);
+ sb->out_baton = dup_translation_baton(b->out_baton, pool);
+ sb->written = b->written;
+ sb->readbuf = svn_stringbuf_dup(b->readbuf, pool);
+ sb->readbuf_off = b->readbuf_off;
+ sb->buf = apr_pmemdup(pool, b->buf, SVN__STREAM_CHUNK_SIZE + 1);
- return svn_error_return(svn_stream_mark(b->stream, mark, pool));
+ mark_translated->saved_baton = sb;
+ *mark = (svn_stream_mark_t *)mark_translated;
+
+ return SVN_NO_ERROR;
}
/* Implements svn_io_seek_fn_t. */
@@ -1178,30 +1242,25 @@ static svn_error_t *
translated_stream_seek(void *baton, svn_stream_mark_t *mark)
{
struct translated_stream_baton *b = baton;
- svn_error_t *err;
+ struct translated_stream_baton *sb;
+ mark_translated_t *mark_translated = (mark_translated_t *)mark;
/* Flush output buffer if necessary. */
if (b->written)
SVN_ERR(translate_chunk(b->stream, b->out_baton, NULL, 0, b->iterpool));
- err = svn_stream_seek(b->stream, mark);
- if (err == NULL)
- {
- /* Force into boring state. */
- b->in_baton->newline_off = 0;
- b->in_baton->keyword_off = 0;
- b->out_baton->newline_off = 0;
- b->out_baton->keyword_off = 0;
-
- /* Output buffer has been flushed. */
- b->written = FALSE;
+ SVN_ERR(svn_stream_seek(b->stream, mark_translated->mark));
- /* Reset read buffer. */
- svn_stringbuf_setempty(b->readbuf);
- b->readbuf_off = 0;
- }
+ /* Restore translation state. */
+ sb = mark_translated->saved_baton;
+ b->in_baton = dup_translation_baton(sb->in_baton, b->pool);
+ b->out_baton = dup_translation_baton(sb->out_baton, b->pool);
+ b->written = sb->written;
+ b->readbuf = svn_stringbuf_dup(sb->readbuf, b->pool);
+ b->readbuf_off = sb->readbuf_off;
+ b->buf = apr_pmemdup(b->pool, sb->buf, SVN__STREAM_CHUNK_SIZE + 1);
- return svn_error_return(err);
+ return SVN_NO_ERROR;
}
svn_error_t *
@@ -1255,7 +1314,7 @@ svn_subst_stream_translated(svn_stream_t
= apr_palloc(baton_pool, sizeof(*baton));
svn_stream_t *s = svn_stream_create(baton, baton_pool);
- /* Make sure EOL_STR and KEYWORDS are allocated in POOL, as
+ /* Make sure EOL_STR and KEYWORDS are allocated in BATON_POOL, as
required by create_translation_baton() */
if (eol_str)
eol_str = apr_pstrdup(baton_pool, eol_str);
@@ -1265,22 +1324,8 @@ svn_subst_stream_translated(svn_stream_t
keywords = NULL;
else
{
- /* deep copy the hash to make sure it's allocated in POOL */
- apr_hash_t *copy = apr_hash_make(baton_pool);
- apr_hash_index_t *hi;
-
- for (hi = apr_hash_first(pool, keywords);
- hi; hi = apr_hash_next(hi))
- {
- const void *key;
- void *val;
- apr_hash_this(hi, &key, NULL, &val);
- apr_hash_set(copy, apr_pstrdup(baton_pool, key),
- APR_HASH_KEY_STRING,
- svn_string_dup(val, baton_pool));
- }
-
- keywords = copy;
+ /* deep copy the hash to make sure it's allocated in BATON_POOL */
+ keywords = keywords_hash_deep_copy(keywords, baton_pool);
}
}
Modified: subversion/trunk/subversion/tests/libsvn_subr/stream-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/stream-test.c?rev=936844&r1=936843&r2=936844&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/stream-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/stream-test.c Thu Apr 22 13:49:14 2010
@@ -24,6 +24,7 @@
#include <stdio.h>
#include "svn_pools.h"
#include "svn_io.h"
+#include "svn_subst.h"
#include <apr_general.h>
#include "../svn_test.h"
@@ -554,6 +555,86 @@ test_stream_seek_stringbuf(apr_pool_t *p
return SVN_NO_ERROR;
}
+static svn_error_t *
+test_stream_seek_translated(apr_pool_t *pool)
+{
+ svn_stream_t *stream, *translated_stream;
+ svn_stringbuf_t *stringbuf;
+ char buf[23];
+ apr_size_t len;
+ svn_stream_mark_t *mark, *mark2;
+ apr_hash_t *keywords;
+ svn_string_t *keyword_val;
+
+ keywords = apr_hash_make(pool);
+ keyword_val = svn_string_create("my key word was expanded", pool);
+ apr_hash_set(keywords, "MyKeyword", APR_HASH_KEY_STRING, keyword_val);
+ stringbuf = svn_stringbuf_create("One$MyKeyword$Two", pool);
+ stream = svn_stream_from_stringbuf(stringbuf, pool);
+ translated_stream = svn_subst_stream_translated(stream, APR_EOL_STR,
+ FALSE, keywords, TRUE, pool);
+ len = 3;
+ SVN_ERR(svn_stream_read(translated_stream, buf, &len));
+ SVN_ERR_ASSERT(len == 3);
+ buf[3] = '\0';
+ SVN_ERR_ASSERT(strcmp(buf, "One") == 0);
+
+ /* Seek from outside of keyword to inside of keyword. */
+ SVN_ERR(svn_stream_mark(translated_stream, &mark, pool));
+ len = 3;
+ SVN_ERR(svn_stream_read(translated_stream, buf, &len));
+ SVN_ERR_ASSERT(len == 3);
+ buf[3] = '\0';
+ /* ### The test currently fails here because the keyword isn't
+ * ### expanded correctly. buf contains "$My\0" */
+ SVN_ERR_ASSERT(strcmp(buf, "my ") == 0);
+ SVN_ERR(svn_stream_seek(stream, mark));
+ len = 3;
+ SVN_ERR(svn_stream_read(stream, buf, &len));
+ SVN_ERR_ASSERT(len == 3);
+ buf[3] = '\0';
+ SVN_ERR_ASSERT(strcmp(buf, "my ") == 0);
+
+ /* Seek from inside of keyword to inside of keyword. */
+ SVN_ERR(svn_stream_mark(translated_stream, &mark, pool));
+ len = 3;
+ SVN_ERR(svn_stream_read(translated_stream, buf, &len));
+ SVN_ERR_ASSERT(len == 3);
+ buf[3] = '\0';
+ SVN_ERR_ASSERT(strcmp(buf, "key") == 0);
+ SVN_ERR(svn_stream_seek(stream, mark));
+ len = 3;
+ SVN_ERR(svn_stream_read(stream, buf, &len));
+ SVN_ERR_ASSERT(len == 3);
+ buf[3] = '\0';
+ SVN_ERR_ASSERT(strcmp(buf, "my ") == 0);
+
+ /* Seek from inside of keyword to outside of keyword. */
+ len = 22;
+ SVN_ERR(svn_stream_read(translated_stream, buf, &len));
+ SVN_ERR_ASSERT(len == 22);
+ buf[22] = '\0';
+ SVN_ERR_ASSERT(strcmp(buf, "keyword was expandedTw") == 0);
+ SVN_ERR(svn_stream_mark(translated_stream, &mark2, pool));
+ SVN_ERR(svn_stream_seek(stream, mark));
+ len = 3;
+ SVN_ERR(svn_stream_read(stream, buf, &len));
+ SVN_ERR_ASSERT(len == 3);
+ buf[3] = '\0';
+ SVN_ERR_ASSERT(strcmp(buf, "my ") == 0);
+ SVN_ERR(svn_stream_seek(stream, mark2));
+ len = 1;
+ SVN_ERR(svn_stream_read(stream, buf, &len));
+ SVN_ERR_ASSERT(len == 1);
+ buf[1] = '\0';
+ SVN_ERR_ASSERT(strcmp(buf, "o") == 0);
+
+ SVN_ERR(svn_stream_close(stream));
+
+ return SVN_NO_ERROR;
+}
+
+
/* The test table. */
@@ -578,5 +659,7 @@ struct svn_test_descriptor_t test_funcs[
"test stream seeking for files"),
SVN_TEST_PASS2(test_stream_seek_stringbuf,
"test stream seeking for stringbufs"),
+ SVN_TEST_XFAIL2(test_stream_seek_translated,
+ "test stream seeking for translated streams"),
SVN_TEST_NULL
};
Re: svn commit: r936844 - in /subversion/trunk/subversion:
libsvn_subr/subst.c tests/libsvn_subr/stream-test.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Apr 22, 2010 at 09:49, <st...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu Apr 22 13:49:14 2010
>...
> @@ -1164,13 +1201,40 @@ translated_stream_reset(void *baton)
> return svn_error_return(err);
> }
>
> +/* svn_stream_mark_t for translation streams. */
> +typedef struct
> +{
> + /* Saved translation state. */
> + struct translated_stream_baton *saved_baton;
You can lose the indirection here, and just embed one struct in
another. That'll save a separate allocation later, and various derefs.
> + /* Mark set on the underlying stream. */
> + svn_stream_mark_t *mark;
> +} mark_translated_t;
> +
> /* Implements svn_io_mark_fn_t. */
> static svn_error_t *
> translated_stream_mark(void *baton, svn_stream_mark_t **mark, apr_pool_t *pool)
> {
> + mark_translated_t *mark_translated;
> struct translated_stream_baton *b = baton;
> + struct translated_stream_baton *sb;
> +
> + mark_translated = apr_palloc(pool, sizeof(*mark_translated));
> + SVN_ERR(svn_stream_mark(b->stream, &mark_translated->mark, pool));
> +
> + /* Save translation state. */
> + sb = apr_pcalloc(pool, sizeof(*sb));
> + sb->in_baton = dup_translation_baton(b->in_baton, pool);
> + sb->out_baton = dup_translation_baton(b->out_baton, pool);
> + sb->written = b->written;
> + sb->readbuf = svn_stringbuf_dup(b->readbuf, pool);
> + sb->readbuf_off = b->readbuf_off;
> + sb->buf = apr_pmemdup(pool, b->buf, SVN__STREAM_CHUNK_SIZE + 1);
>
> - return svn_error_return(svn_stream_mark(b->stream, mark, pool));
> + mark_translated->saved_baton = sb;
The above will all get easier with the embedded struct.
>...
> + /* Restore translation state. */
> + sb = mark_translated->saved_baton;
> + b->in_baton = dup_translation_baton(sb->in_baton, b->pool);
> + b->out_baton = dup_translation_baton(sb->out_baton, b->pool);
> + b->written = sb->written;
> + b->readbuf = svn_stringbuf_dup(sb->readbuf, b->pool);
> + b->readbuf_off = sb->readbuf_off;
> + b->buf = apr_pmemdup(b->pool, sb->buf, SVN__STREAM_CHUNK_SIZE + 1);
s/mark_translated/mt/, and the above becomes something like:
b->in_baton = dup_translation_baton(mt->save.in_baton, b->pool);
And with this said, I think you should stop copying the keywords
around. They are CONSTANT for the duration of the translation stream.
Thus, the individual marks can just refer to the stream's set of
keywords. That stream has a longer life than the marks, so we don't
need the keyword copying. (make sure to note this lifetime in some
comments somewhere!)
The problem with the copying is the following (in pseudocode):
for i in range(1000000):
mark = stream.mark()
stream.reset_to(mark)
Each time you copy the keywords back into the stream, its pool just
gets larger...
This is why it is *TERRIBLY* important for objects not to carry around
pools. The translated_stream_baton should not be doing this!
>...
> +++ subversion/trunk/subversion/tests/libsvn_subr/stream-test.c Thu Apr 22 13:49:14 2010
>...
> + len = 3;
> + SVN_ERR(svn_stream_read(translated_stream, buf, &len));
> + SVN_ERR_ASSERT(len == 3);
> + buf[3] = '\0';
> + SVN_ERR_ASSERT(strcmp(buf, "One") == 0);
You should be using SVN_TEST_ASSERT() and SVN_TEST_STRING_ASSERT() ...
>...
Cheers,
-g