=?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <brane@xbc.nu> writes:
> Here's my review. It might be a bit incoherent in places, because I
> dind't write it top-down, and I only thought of using a temporary pool
> for expansion towards the end. So, read with a grain of salt. :-)
Waiter, can I get some sea salt?
...
> On second thought, add a tmp_value stringbuf to the svn_config_t, so
> that tmp_key is always used for names and tmp_value for values. We don't
> want them to bash on each other by mistake. And remembet to always use
> svn_config_t::pool when manipulating svn_config_t::tmp_key and
> svn_config_t::tmp_value, never x_pool.
I didn't implement this, as I wanted to avoid multiple recursive
invocations of expand_option_value() stomping on each other. Instead,
I used a temporary sub-pool.
> >--- subversion/libsvn_subr/config.c (revision 5277)
> >+++ subversion/libsvn_subr/config.c (working copy)
...
> >@@ -361,26 +363,38 @@
> > if (sec_ptr != NULL && option != NULL)
> > {
> > cfg_section_t *sec = sec_ptr;
> >+ cfg_option_t *opt;
> >
> > /* Canonicalize the option key */
> > svn_stringbuf_set (cfg->tmp_key, option);
> > make_hash_key (cfg->tmp_key->data);
> >
> >- return apr_hash_get (sec->options, cfg->tmp_key->data,
> >- cfg->tmp_key->len);
> >+ opt = apr_hash_get (sec->options, cfg->tmp_key->data,
> >+ cfg->tmp_key->len);
> >+ /* NOTE: ConfigParser's sections are case sensitive. */
> >
> Yes, I think we document elsewhere that case-insensitivity in the
> section and option names is (should be) the only difference between our
> svn_config_t and ConfigParser.
I left the comment, as I didn't take the time to look around for where
we note this. Perhaps config_impl.h is the Right place?
...
> >+ name_end = strstr (name_start, FMT_END);
> >+
> >+ if (name_end != NULL)
> >+ {
> >+ cfg_option_t *x_opt;
> >+ int name_len = name_end - name_start;
> >+ char name[name_len + 1];
> >
> Excuse me? This actually compiles on your box? Variable-length arrays
> and late declarations are a C99 feature. We can't use C99 in our code,
> because there aren't enough C99 compilers in the world. You can't do that.
Heh, yeah, I was a bit surprised myself that it compiled.
> You need yet another temporary stringbuf in svn_config_t, I think.
>
> >+ apr_cpystrn (name, name_start, name_len + 1);
> >
> So:
>
> svn_stringbuf_ensore(cfg->tmp_opt, name_len + 1);
> apr_cpystrn(cfg->tmp_opt->data, name_start, name_len + 1);
Looks like I could get away with that in this case. Instead, I used
apr_pstrmemdup() with the temporary sub-pool.
> >+ x_opt = find_option (cfg, section->name, name, NULL);
> >+
> >+ if (x_opt != NULL)
> >+ {
> >+ const char *cstring;
> >+ make_string_from_option (&cstring, cfg, section, x_opt);
> >
> I think there's a serious problem here. You're calling
> make_string_from_option recursively. Instead, you should just insert the
> expanded value in-place, and continue from the beginning of the
> expansion. This means changing the way you're handling origiinal value.
>
> Hm. this means that using cfg->tmp_val and cfg->tmp_opt won't work.
> Yikes. Tell you what, just use a temporary pool in
> make_string_from_option, do all your temporary allocations from there,
> then destroy the pool after expansion. Your make_string_from_option then
> becomes:
>
> make_string_from_option (const char &&valuep, svn_config_t *cfg,
> cfg_section_t *sec, cfg_option_t *opt,
> apr_pool_t *x_pool)
> {
> apr_pool_t *temp_pool = (x_pool ? x_pool : svn_pool_create(cfg->x_pool));
>
> ...
>
> if (!x_pool)
> svn_pool_destroy(temp_pool);
> }
>
> Then you pass temp_pool as a new parameter to expand_option_value, and
> pass /that/ pool back to make_string_from_option. That way you'll use
> the same pool for any number of recursive expansions, and clear it at
> the end (by that time, you shouldn've copied the result into x_pool, if
> necessary).
As far as I've been able to tell, that worked out really well.
...
> >+ }
> >+ else
> >+ /* Though ConfigParser treats unterminated format specifiers
> >+ as an exception condition, we leave the expanded value set
> >+ to NULL. */
> >+ remaining_value = NULL;
>
> Same here.
Here we do actually want to terminate the loop (with some additional
logic after termination), as since there are no more format specifier
endings, no more parsing is required (it's all shmoo).
> >+ }
> >+
> >+ if (buf != NULL)
> >+ {
> >+ /* ### HELP: What to do with buf? We want its c-string data,
> >+ ### not the struct itself. */
> >+ *opt_x_valuep = buf->data;
> >
>
> *opt_x_valuep = apr_pstrmemdup(cfg->x_pool, buf->data, buf->len);
>
> I think we already document that what you get from svn_config_get is
> only valid until the next call to any svn_config function.
>
> Hm, it's not so simple, really -- you should only copy to the
> cfg->x_pool at the end of a complete expansion. Remember, you have a
> pair recursion between make_string_from_option and expand_option_value.
> Only the top call to expand_option_value should copy the data to
> cfg->x_pool. Alernatively, you could have the top
> make_string_from_option do that *if* the value got expanded (there, you
> at least know you're at the top of the recursoin, because my proposed
> x_pool parameter will be NULL).
The copy in make_string_from_option() made sense to me. Here it is:
* subversion/libsvn_subr/config_impl.h
(SVN_CONFIG__DEFAULT_SECTION): The name of the default configuration
section.
* subversion/libsvn_subr/config.c
(find_option): If the option in the specified section isn't found,
look in the default section for an option of the same name (if
we aren't already looking there).
(make_string_from_option): Call expand_option_value(), and set
opt->expanded. Expanded signature to take a cfg_section_t and
apr_pool_t pointer for use by expand_option_value()/find_option().
(FMT_START, FMT_END, FMT_START_LEN, FMT_END_LEN): Constants used in
expand_option_value().
(expand_option_value): New function which expands option values. It
takes pointers to a value and x_value string -- rather than
directly to a cfg_option_t -- so that it can be called on plain
text (as on the default value parameter passed to
svn_config_get()). It must be forward decl'd ahead of
make_string_from_option(), as the functions have a bi-directional
dependency upon each other. Config text which would be exceptions
to Python's ConfigParser (such as the failure to resolve an
expansion, or an unterminated format string) is treated as plain
text.
(svn_config_get): Pass a cfg_section_t ** to find_option() so the
section is available when make_string_from_option() is called.
Call expand_option_value() on the default_value parameter.
(svn_config_enumerate): Pass a cfg_section_t * and NULL apr_pool_t *
to make_string_from_option() to handle the function's API change.
* subversion/tests/libsvn_subr/config-test.c
Test option expansion behavior of the svn_config_read() and
svn_config_get() functions.
(config_keys, config_values): More name/value pairs from the
configuration file.
(test1): Added a conditionally compiled debugging trace. Fail the
test if only one of the values is null.
* subversion/tests/libsvn_subr/config-test.cfg
Introduced more features from Python's ConfigParser file format into
test file.
Index: subversion/libsvn_subr/config_impl.h
===================================================================
--- subversion/libsvn_subr/config_impl.h (revision 5417)
+++ subversion/libsvn_subr/config_impl.h (working copy)
@@ -61,6 +61,7 @@
const char *file,
svn_boolean_t must_exist);
+#define SVN_CONFIG__DEFAULT_SECTION "DEFAULT"
#ifdef SVN_WIN32
/* Get the common or user-specific AppData folder */
Index: subversion/libsvn_subr/config.c
===================================================================
--- subversion/libsvn_subr/config.c (revision 5417)
+++ subversion/libsvn_subr/config.c (working copy)
@@ -361,38 +361,155 @@
if (sec_ptr != NULL && option != NULL)
{
cfg_section_t *sec = sec_ptr;
+ cfg_option_t *opt;
/* Canonicalize the option key */
svn_stringbuf_set (cfg->tmp_key, option);
make_hash_key (cfg->tmp_key->data);
- return apr_hash_get (sec->options, cfg->tmp_key->data,
- cfg->tmp_key->len);
+ opt = apr_hash_get (sec->options, cfg->tmp_key->data,
+ cfg->tmp_key->len);
+ /* NOTE: ConfigParser's sections are case sensitive. */
+ if (opt == NULL
+ && apr_strnatcasecmp(section, SVN_CONFIG__DEFAULT_SECTION) != 0)
+ /* Options which aren't found in the requested section are
+ also sought after in the default section. */
+ opt = find_option (cfg, SVN_CONFIG__DEFAULT_SECTION, option, &sec);
+ return opt;
}
return NULL;
}
-/* Set *VALUEP according to the OPT's value. */
+/* Has a bi-directional dependency with make_string_from_option(). */
static void
-make_string_from_option (const char **valuep,
- svn_config_t *cfg, cfg_option_t *opt)
+expand_option_value (svn_config_t *cfg, cfg_section_t *section,
+ const char *opt_value, const char **opt_x_valuep,
+ apr_pool_t *x_pool);
+
+
+/* Set *VALUEP according to the OPT's value. A value for X_POOL must
+ only ever be passed into this function by expand_option_value(). */
+static void
+make_string_from_option (const char **valuep, svn_config_t *cfg,
+ cfg_section_t *section, cfg_option_t *opt,
+ apr_pool_t* x_pool)
{
- /* ### TODO: Expand the option's value */
- (void)(cfg);
+ apr_pool_t *tmp_pool = (x_pool ? x_pool : svn_pool_create (cfg->x_pool));
+ expand_option_value (cfg, section, opt->value, &opt->x_value, tmp_pool);
+ opt->expanded = TRUE;
/* For legacy reasons, the cfg is still using counted-length strings
internally. But the public interfaces just use null-terminated
C strings now, so below we ignore length and use only data. */
if (opt->x_value)
- *valuep = opt->x_value;
+ {
+ if (!x_pool)
+ /* Grab the fully expanded value from tmp_pool before its
+ disappearing act. */
+ opt->x_value = apr_pstrmemdup (cfg->x_pool, opt->x_value,
+ strlen (opt->x_value));
+ *valuep = opt->x_value;
+ }
else
*valuep = opt->value;
+
+ if (!x_pool)
+ svn_pool_destroy (tmp_pool);
}
+#define FMT_START "%("
+#define FMT_END ")s"
+#define FMT_START_LEN (sizeof (FMT_START) - 1)
+#define FMT_END_LEN (sizeof (FMT_END) - 1)
+
+
+static void
+expand_option_value (svn_config_t *cfg, cfg_section_t *section,
+ const char *opt_value, const char **opt_x_valuep,
+ apr_pool_t *x_pool)
+{
+ svn_stringbuf_t *buf = NULL;
+ const char *parse_from = opt_value;
+ const char *copy_from = parse_from;
+ const char *name_start, *name_end;
+
+ while (parse_from != NULL
+ && *parse_from != '\0'
+ && (name_start = strstr (parse_from, FMT_START)) != NULL)
+ {
+ name_start += FMT_START_LEN;
+ if (*name_start == '\0')
+ /* FMT_START at end of opt_value. */
+ break;
+
+ /* ### Rewriting this function using full-blown tokenization
+ ### would prevent attempted expansions of text like
+ ### "%(foo %(bar)s" under the name of "foo %(bar". */
+
+ name_end = strstr (name_start, FMT_END);
+ if (name_end != NULL)
+ {
+ cfg_option_t *x_opt;
+ apr_size_t len = name_end - name_start;
+ char *name = apr_pstrmemdup (x_pool, name_start, len);
+
+ x_opt = find_option (cfg, section->name, name, NULL);
+
+ if (x_opt != NULL)
+ {
+ const char *cstring;
+
+ /* Pass back the sub-pool originally provided by
+ make_string_from_option() as an indication of when it
+ should terminate. */
+ make_string_from_option (&cstring, cfg, section, x_opt, x_pool);
+
+ /* Append the plain text preceding the expansion. */
+ len = name_start - FMT_START_LEN - copy_from;
+ if (buf == NULL)
+ {
+ buf = svn_stringbuf_ncreate (copy_from, len, x_pool);
+ cfg->x_values = TRUE;
+ }
+ else
+ svn_stringbuf_appendbytes(buf, copy_from, len);
+
+ /* Append the expansion and adjust parse pointers. */
+ svn_stringbuf_appendcstr (buf, cstring);
+ parse_from = name_end + FMT_END_LEN;
+ copy_from = parse_from;
+ }
+ else
+ /* Though ConfigParser considers the failure to resolve
+ the requested expansion an exception condition, we
+ consider it to be plain text, and look for the start of
+ the next one. */
+ parse_from = name_end + FMT_END_LEN;
+ }
+ else
+ /* Though ConfigParser treats unterminated format specifiers
+ as an exception condition, we consider them to be plain
+ text. The fact that there are no more format specifier
+ endings means we're done parsing. */
+ parse_from = NULL;
+ }
+
+ if (buf != NULL)
+ {
+ /* Copy the remainder of the plain text. */
+ svn_stringbuf_appendcstr (buf, copy_from);
+
+ *opt_x_valuep = apr_pstrmemdup (x_pool, buf->data, buf->len);
+ }
+ else
+ *opt_x_valuep = NULL;
+}
+
+
void
svn_config_get (svn_config_t *cfg, const char **valuep,
@@ -401,11 +518,12 @@
{
if (cfg)
{
- cfg_option_t *opt = find_option (cfg, section, option, NULL);
+ cfg_section_t *sec;
+ cfg_option_t *opt = find_option (cfg, section, option, &sec);
if (opt != NULL)
- make_string_from_option (valuep, cfg, opt);
+ make_string_from_option (valuep, cfg, sec, opt, NULL);
else
- *valuep = default_value; /* ### TODO: Expand default_value */
+ expand_option_value (cfg, sec, default_value, valuep, cfg->x_pool);
}
else
*valuep = default_value;
@@ -481,7 +599,7 @@
opt = opt_ptr;
++count;
- make_string_from_option (&temp_value, cfg, opt);
+ make_string_from_option (&temp_value, cfg, sec, opt, NULL);
if (!callback (opt->name, temp_value, baton))
break;
}
Index: subversion/tests/libsvn_subr/config-test.c
===================================================================
--- subversion/tests/libsvn_subr/config-test.c (revision 5417)
+++ subversion/tests/libsvn_subr/config-test.c (working copy)
@@ -84,10 +84,14 @@
}
-static const char *config_keys[] = { "a", "b", NULL };
-static const char *config_values[] = { "Aa", "100", NULL };
+static const char *config_keys[] = { "foo", "a", "b", "c", "d", "e", "f", "g",
+ "h", "i", NULL };
+static const char *config_values[] = { "bar", "Aa", "100", "bar",
+ "a %(bogus)s oyster bar",
+ "%(bogus)s shmoo %(",
+ "%Aa", "lyrical bard", "%(unterminated",
+ "Aa 100", NULL };
-
static svn_error_t *
test1 (const char **msg,
svn_boolean_t msg_only,
@@ -117,7 +121,14 @@
py_val = (char *) config_values[i];
svn_config_get(cfg, (const char **) &c_val, "section1", key,
"default value");
- if (c_val != NULL && py_val != NULL && strcmp(c_val, py_val) != 0)
+#if 0
+ printf("Testing expected value '%s' against '%s' for "
+ "option '%s'\n", py_val, c_val, key);
+#endif
+ /* Fail iff one value is null, or the strings don't match. */
+ if ((c_val == NULL ^ py_val == NULL)
+ || (c_val != NULL && py_val != NULL
+ && apr_strnatcmp(c_val, py_val) != 0))
return fail(pool, "Expected value '%s' not equal to '%s' for "
"option '%s'", py_val, c_val, key);
}
Index: subversion/tests/libsvn_subr/config-test.cfg
===================================================================
--- subversion/tests/libsvn_subr/config-test.cfg (revision 5417)
+++ subversion/tests/libsvn_subr/config-test.cfg (working copy)
@@ -1,3 +1,25 @@
+# default values across all sections
+[DEFAULT]
+foo=bar
+# Not implementing __name__ expansions
+#baz=%(__name__)s
+
[section1]
+# Trailing whitespace
a=Aa
-b= 100
\ No newline at end of file
+# leading whitespace / numeric
+b= 100
+# Variable expansion
+c=%(foo)s
+# Expansion for non-existent option (ConfigParser throws an
+# InterpolationError with the message "Bad value substitution")
+d=a %(bogus)s oyster %(foo)s
+e=%(bogus)s shmoo %(
+# Expansion format escaping doesn't seem possible
+f=%%(a)s
+# Two-level variable expansion with surrounding text
+g=lyrical %(c)sd
+# Unterminated format string
+h= %(unterminated
+# Multiple expansions
+i=%(a)s %(b)s
\ No newline at end of file
--
Daniel Rall <dlr@finemaltcoding.com>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 21 22:20:51 2003