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
};
