Lieven Govaerts wrote:
> Julian,
>
> attached you'll find a new patch for issue 1780, that I think adresses your
> remarks.
Thanks.
> I added line 76 in translate-test.c to test for this problem. Solved it in
> the attached patch.
Good.
>>2) It takes order(N-squared) time to process a line
>>containing N dollar signs.
>
> I tried to improve performance, by now only using the matching algorithm
> when a valid keyword has been found. So it will only skip dollar signs when
> a keyword was matched first. Whenever something like '$testblahblah$' is
> found, it'll just skip that part of text.
Good.
> I had to separate the keyword matching functionality in translate_keyword in
> its own function match_keyword, to support this performance improvement.
> Since these are internal functions and not used outside subst.c this is no
> problem. Correct assumption?
Correct.
> That makes the patch a bit larger, and maybe a bit less clear.
It's fine. Well, actually, the structural part:
if (!matches)
{X}
if (A || !matches || C)
{Y}
else
{Z}
was quite difficult to understand, but it's OK.
> [[[
> Fix issue #1780: correct keyword expansion when URL or Author value contains
> '$'.
>
> * subversion/libsvn_subr/subst.c
> (translate_chunk): add algorithm that keeps searching for next '$' in the
> value of a valid expanded keyword until a correct value is found or search
> limits are reached.
> (match_keyword): new local function that matches a valid keyword in a char
> buffer.
> (translate_keyword): added keyword_name parameter, translate a keyword
> keyword_name with its value. Keyword matching was put in match_keyword.
>
> * subversion/tests/libsvn_wc/translate-test.c
> (lines[]): added lines 74-76 to test on '$' in Author and URL keywords.
> ]]]
> if (b->keyword_off > SVN_KEYWORD_MAX_LEN ||
Should that be ">="? By the time it's greater, we'll already have written at
least one character off the end of the buffer.
Check what's going to happen at the boundary condition: will it do the right
thing for a valid keyword-pattern of exactly MAX_LEN? MAX_LEN-1? MAX_LEN+1?
What about a non-keyword pattern of those lengths?
I think "b->keyword_off >= SVN_KEYWORD_MAX_LEN" will need to go after
"translate_keyword()", but see what you think.
> keyword_matches == FALSE ||
> translate_keyword (b->keyword_buf, &b->keyword_off,
> keyword_name, b->expand, b->keywords))
> {
Since I've done some tidying while studying it, I attach my version of the
patch and also a diff from yours to mine. The changes are mostly issues of
style and comments. The significant changes are: use "apr_size_t" for
"next_sign_off", because that is the type used for other values to which it
relates (b->keyword_off); don't initialise the new flag at its definition
because it is initialised at its first use; add a missing comma in the array of
test lines.
If you think the two things above (changing the limit test to ">=" and moving
it to the end of the condition) are the only further changes needed, I can do
that and commit it for you, otherwise I look forward to seeing the next version.
- Julian
Fix issue #1780: correct keyword expansion when URL or Author value contains
'$'.
Patch by: Lieven Govaerts <lgo@mobsol.be>
* subversion/libsvn_subr/subst.c
(translate_chunk): Keep searching for next '$' until valid keyword
is found or search limits are reached.
(translate_chunk): add algorithm that keeps searching for next '$' in the
value of a valid expanded keyword until a correct value is found or search
limits are reached.
(match_keyword): new local function that matches a valid keyword in a char
buffer.
(translate_keyword): added keyword_name parameter, translate a keyword
keyword_name with its value. Keyword matching was put in match_keyword.
* subversion/tests/libsvn_wc/translate-test.c
(lines): Add lines to test on '$' in Author and URL keywords.
(substitute_and_verify):
Index: subversion/libsvn_subr/subst.c
===================================================================
--- subversion/libsvn_subr/subst.c (revision 18446)
+++ subversion/libsvn_subr/subst.c (working copy)
@@ -607,10 +607,33 @@
}
/* Parse BUF (whose length is *LEN) for Subversion keywords. If a
- keyword is found, optionally perform the substitution on it in
- place, update *LEN with the new length of the translated keyword
- string, and return TRUE. If this buffer doesn't contain a known
- keyword pattern, leave BUF and *LEN untouched and return FALSE.
+ keyword is found, update *KEYWORD_NAME with the keyword name and
+ return TRUE. */
+static svn_boolean_t
+match_keyword (char *buf,
+ apr_size_t *len,
+ char *keyword_name,
+ apr_hash_t *keywords)
+{
+ apr_size_t i;
+
+ /* Early return for ignored keywords */
+ if (! keywords)
+ return FALSE;
+
+ /* Extract the name of the keyword */
+ for (i = 0; i < *len - 2 && buf[i + 1] != ':'; i++)
+ keyword_name[i] = buf[i + 1];
+ keyword_name[i] = '\0';
+
+ return apr_hash_get (keywords, keyword_name, APR_HASH_KEY_STRING) != NULL;
+}
+
+/* Try to translate keyword *KEYWORD_NAME in BUF:
+ optionally perform the substitution in place, update *LEN with
+ the new length of the translated keyword string, and return TRUE.
+ If this buffer doesn't contain a known keyword pattern, leave BUF
+ and *LEN untouched and return FALSE.
See the docstring for svn_subst_copy_and_translate for how the
EXPAND and KEYWORDS parameters work.
@@ -625,12 +648,11 @@
static svn_boolean_t
translate_keyword (char *buf,
apr_size_t *len,
+ const char *keyword_name,
svn_boolean_t expand,
apr_hash_t *keywords)
{
const svn_string_t *value;
- char keyword_name[SVN_KEYWORD_MAX_LEN + 1];
- apr_size_t i;
/* Make sure we gotz good stuffs. */
assert (*len <= SVN_KEYWORD_MAX_LEN);
@@ -640,11 +662,6 @@
if (! keywords)
return FALSE;
- /* Extract the name of the keyword */
- for (i = 0; i < *len - 2 && buf[i + 1] != ':'; i++)
- keyword_name[i] = buf[i + 1];
- keyword_name[i] = '\0';
-
value = apr_hash_get (keywords, keyword_name, APR_HASH_KEY_STRING);
if (value)
@@ -919,6 +936,7 @@
/* precalculate some oft-used values */
const char *end = buf + buflen;
const char *interesting = b->interesting;
+ apr_size_t next_sign_off = 0;
/* At the beginning of this loop, assume that we might be in an
* interesting state, i.e. with data in the newline or keyword
@@ -943,21 +961,50 @@
}
else if (b->keyword_off && *p == '$')
{
- /* If translation fails, treat this '$' as a starting '$'. */
- b->keyword_buf[b->keyword_off++] = '$';
- if (translate_keyword (b->keyword_buf, &b->keyword_off,
- b->expand, b->keywords))
- p++;
- else
- b->keyword_off--;
+ svn_boolean_t keyword_matches;
+ char keyword_name[SVN_KEYWORD_MAX_LEN + 1];
- SVN_ERR (translate_write (dst, b->keyword_buf, b->keyword_off));
+ /* If keyword is matched, but not correctly translated, try to
+ * look for the next ending '$'. */
+ b->keyword_buf[b->keyword_off++] = *p++;
+ keyword_matches = match_keyword (b->keyword_buf, &b->keyword_off,
+ keyword_name, b->keywords);
+ if (keyword_matches == FALSE)
+ {
+ /* reuse the ending '$' */
+ *p--;
+ b->keyword_off--;
+ }
- b->keyword_off = 0;
+ if (b->keyword_off > SVN_KEYWORD_MAX_LEN ||
+ keyword_matches == FALSE ||
+ translate_keyword (b->keyword_buf, &b->keyword_off,
+ keyword_name, b->expand, b->keywords))
+ {
+ /* write out non-matching text or translated keyword */
+ SVN_ERR (translate_write (dst, b->keyword_buf, b->keyword_off));
+
+ next_sign_off = 0;
+ b->keyword_off = 0;
+ }
+ else
+ {
+ if (next_sign_off == 0)
+ next_sign_off = b->keyword_off - 1;
+
+ continue;
+ }
}
else if (b->keyword_off == SVN_KEYWORD_MAX_LEN - 1
|| (b->keyword_off && (*p == '\r' || *p == '\n')))
{
+ if (next_sign_off > 0)
+ {
+ /* rolling back, continue with next '$' in keyword_buf */
+ p -= (b->keyword_off - next_sign_off);
+ b->keyword_off = next_sign_off;
+ next_sign_off = 0;
+ }
/* No closing '$' found; flush the keyword buffer. */
SVN_ERR (translate_write (dst, b->keyword_buf, b->keyword_off));
Index: subversion/tests/libsvn_wc/translate-test.c
===================================================================
--- subversion/tests/libsvn_wc/translate-test.c (revision 18446)
+++ subversion/tests/libsvn_wc/translate-test.c (working copy)
@@ -126,7 +126,16 @@
".$veR$Author$",
"$",
"$$",
- "Line 74: end of subst test data."
+ /* Line 74-75 test for keywords containing '$', issue #1780 */
+ "Line 74: Valid $Author: jran$dom $, started expanded.",
+ "Line 75: Valid $URL: http://tomato/mau$ve $, started expanded.",
+ /* Line 76 tests for a string with a potential keyword of exactly 253
+ bytes long */
+ "$ "
+ " "
+ " "
+ " $$",
+ "Line 77: end of subst test data."
};
@@ -468,6 +477,11 @@
NULL);
expect[71 - 1] =
apr_pstrcat (pool, ".$veR$Author: ", author, " $", NULL);
+
+ expect[74 - 1] =
+ apr_pstrcat (pool, "Line 74: ",
+ "Valid $Author: ", author, " $, started expanded.",
+ NULL);
}
else /* unexpand */
{
@@ -476,6 +490,8 @@
"Line 37: Valid $LastChangedBy$, started expanded.";
expect[38 - 1] =
"Line 38: Valid $Author$, started expanded.";
+ expect[74 - 1] =
+ "Line 74: Valid $Author$, started expanded.";
}
}
@@ -499,6 +515,10 @@
apr_pstrcat (pool, "Line 42: ",
"Valid $URL: ", url, " $, started expanded.",
NULL);
+ expect[75 - 1] =
+ apr_pstrcat (pool, "Line 75: ",
+ "Valid $URL: ", url, " $, started expanded.",
+ NULL);
}
else /* unexpand */
{
@@ -507,6 +527,8 @@
"Line 41: Valid $HeadURL$, started expanded.";
expect[42 - 1] =
"Line 42: Valid $URL$, started expanded.";
+ expect[75 - 1] =
+ "Line 75: Valid $URL$, started expanded.";
}
}
--- subversion/libsvn_subr/subst.c 2006-02-14 04:00:26.000000000 +0000
+++ subversion/libsvn_subr/subst.c 2006-02-14 01:44:23.000000000 +0000
@@ -607,7 +607,7 @@
}
/* Parse BUF (whose length is *LEN) for Subversion keywords. If a
- keyword is found, update *keyword_name with the keyword name and
+ keyword is found, update *KEYWORD_NAME with the keyword name and
return TRUE. */
static svn_boolean_t
match_keyword (char *buf,
@@ -629,7 +629,7 @@
return apr_hash_get (keywords, keyword_name, APR_HASH_KEY_STRING) != NULL;
}
-/* Try to translate keyword *keyword_name in BUF with its value,
+/* Try to translate keyword *KEYWORD_NAME in BUF:
optionally perform the substitution in place, update *LEN with
the new length of the translated keyword string, and return TRUE.
If this buffer doesn't contain a known keyword pattern, leave BUF
@@ -936,9 +936,7 @@
/* precalculate some oft-used values */
const char *end = buf + buflen;
const char *interesting = b->interesting;
- unsigned int next_sign_off = 0;
- svn_boolean_t keyword_matches = FALSE;
- char keyword_name[SVN_KEYWORD_MAX_LEN + 1];
+ apr_size_t next_sign_off = 0;
/* At the beginning of this loop, assume that we might be in an
* interesting state, i.e. with data in the newline or keyword
@@ -963,6 +961,9 @@
}
else if (b->keyword_off && *p == '$')
{
+ svn_boolean_t keyword_matches;
+ char keyword_name[SVN_KEYWORD_MAX_LEN + 1];
+
/* If keyword is matched, but not correctly translated, try to
* look for the next ending '$'. */
b->keyword_buf[b->keyword_off++] = *p++;
@@ -971,7 +972,8 @@
if (keyword_matches == FALSE)
{
/* reuse the ending '$' */
- *p--; b->keyword_off--;
+ *p--;
+ b->keyword_off--;
}
if (b->keyword_off > SVN_KEYWORD_MAX_LEN ||
@@ -979,7 +981,7 @@
translate_keyword (b->keyword_buf, &b->keyword_off,
keyword_name, b->expand, b->keywords))
{
- // skip part of text
+ /* write out non-matching text or translated keyword */
SVN_ERR (translate_write (dst, b->keyword_buf, b->keyword_off));
next_sign_off = 0;
--- subversion/tests/libsvn_wc/translate-test.c 2006-02-14 04:00:26.000000000 +0000
+++ subversion/tests/libsvn_wc/translate-test.c 2006-02-14 00:37:07.000000000 +0000
@@ -134,7 +134,7 @@
"$ "
" "
" "
- " $$"
+ " $$",
"Line 77: end of subst test data."
};
@@ -515,7 +515,6 @@
apr_pstrcat (pool, "Line 42: ",
"Valid $URL: ", url, " $, started expanded.",
NULL);
-
expect[75 - 1] =
apr_pstrcat (pool, "Line 75: ",
"Valid $URL: ", url, " $, started expanded.",
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 14 05:23:30 2006