The attached patch enables the following behaviour: % svn --config-option=config:auth:password-stories= status iota svn: warning: apr_err=SVN_ERR_CL_ARG_PARSING_ERROR svn: warning: W205000: Ignoring unknown value 'password-stories'; did you mean 'password-stores'? M iota % svn --config-option=config:foobar:password-stores= status iota svn: warning: apr_err=SVN_ERR_CL_ARG_PARSING_ERROR svn: warning: W205000: Ignoring unknown value 'foobar' M iota
The combination itself is not validated: a correctly-spelled option in the wrong section (such as config:helpers:http-library=...) won't trigger a warning. Unknown options will cause warnings even if they are used by the substitution syntax, e.g., --config-option config:auth:foo=bar will trigger a warning about 'foo', even if --config-option 'config:auth:password-stores=%(foo)s' is specified on the command-line or in the --config-dir. (If this is a concern, we could move the spell-checking to a later point in the argument parsing sequence, after the configuration from --config-dir has been loaded.) Log message skeleton: [[[ * gen-make.py: Generate a list of SVN_CONFIG_* file/section/option names for the svn_cmdline_*() C code to use. * subversion/libsvn_subr/cmdline.c (svn_cmdline__parse_config_option): When parsing the argument to --config-option, spell-check the FILE:SECTION:OPTION coordinates, and warn if any component is unknown. ]]] Thoughts? Daniel (I'm going to be mostly offline for a few days; will reply when I'm back)
Index: build/generator/gen_base.py =================================================================== --- build/generator/gen_base.py (revision 1670590) +++ build/generator/gen_base.py (working copy) @@ -22,6 +22,7 @@ # gen_base.py -- infrastructure for generating makefiles, dependencies, etc. # +import collections import os import sys import glob @@ -319,6 +320,91 @@ class GeneratorBase: def errno_filter(self, codes): return codes + class FileSectionOptionEnum(object): + # These are accessed via getattr() later on + file = object() + section = object() + option = object() + + def _client_configuration_defines(self): + """Return an iterator over SVN_CONFIG_* #define's in the "Client + configuration files strings" section of svn_config.h.""" + + pattern = re.compile( + r'^\s*#\s*define\s+' + r'(?P<macro>SVN_CONFIG_(?P<kind>CATEGORY|SECTION|OPTION)_[A-Z0-9a-z_]+)' + ) + kind = { + 'CATEGORY': self.FileSectionOptionEnum.file, + 'SECTION': self.FileSectionOptionEnum.section, + 'OPTION': self.FileSectionOptionEnum.option, + } + + fname = os.path.join(os.path.abspath(os.path.dirname(sys.argv[0])), + 'subversion', 'include', 'svn_config.h') + lines = iter(open(fname)) + for line in lines: + if "@name Client configuration files strings" in line: + break + else: + raise Exception("Unable to parse svn_config.h") + + for line in lines: + if "@{" in line: + break + else: + raise Exception("Unable to parse svn_config.h") + + for line in lines: + if "@}" in line: + break + match = pattern.match(line) + if match: + yield ( + match.group('macro'), + kind[match.group('kind')], + ) + else: + raise Exception("Unable to parse svn_config.h") + + def write_config_keys(self): + groupby = collections.defaultdict(list) + empty_sections = [] + previous = (None, None) + for macro, kind in self._client_configuration_defines(): + if (previous[1] is self.FileSectionOptionEnum.section + or previous[1] is self.FileSectionOptionEnum.option) \ + and kind is self.FileSectionOptionEnum.section: + empty_sections.append(macro) + groupby[kind].append(macro) + previous = (macro, kind) + + lines = [] + lines.append('/* Automatically generated by %s:write_config_keys() */' + % (__file__,)) + lines.append('') + + for kind in ('file', 'section', 'option'): + macros = groupby[getattr(self.FileSectionOptionEnum, kind)] + lines.append('const char *svn__valid_config_%ss[] = {' % (kind,)) + for macro in macros: + lines.append(' %s,' % (macro,)) + # Remove ',' for c89 compatibility + lines[-1] = lines[-1][0:-1] + lines.append('};') + lines.append('') + + lines.append('const char *svn__empty_config_sections[] = {'); + for section in empty_sections: + lines.append(' %s,' % (section,)) + # Remove ',' for c89 compatibility + lines[-1] = lines[-1][0:-1] + lines.append('};') + lines.append('') + + self.write_file_if_changed('subversion/libsvn_subr/config_keys.inc', + '\n'.join(lines)) + class DependencyGraph: """Record dependencies between build items. @@ -1212,6 +1298,9 @@ class IncludeDependencyInfo: if os.sep.join(['libsvn_subr', 'error.c']) in fname \ and 'errorcode.inc' == include_param: continue # generated by GeneratorBase.write_errno_table + if os.sep.join(['libsvn_subr', 'cmdline.c']) in fname \ + and 'config_keys.inc' == include_param: + continue # generated by GeneratorBase.write_config_keys elif direct_possibility_fname in domain_fnames: self._upd_dep_hash(hdrs, direct_possibility_fname, type_code) elif (len(domain_fnames) == 1 Index: gen-make.py =================================================================== --- gen-make.py (revision 1670590) +++ gen-make.py (working copy) @@ -67,6 +67,7 @@ def main(fname, gentype, verfname=None, generator.write() generator.write_sqlite_headers() generator.write_errno_table() + generator.write_config_keys() if ('--debug', '') in other_options: for dep_type, target_dict in generator.graph.deps.items(): Index: subversion/include/private/svn_cmdline_private.h =================================================================== --- subversion/include/private/svn_cmdline_private.h (revision 1670590) +++ subversion/include/private/svn_cmdline_private.h (working copy) @@ -89,6 +89,9 @@ typedef struct svn_cmdline__config_argument_t * data in @a pool * * @since New in 1.7. + * + * @since 1.9: If the file/section/option combination is unknown, warn to @c + * stderr. */ svn_error_t * svn_cmdline__parse_config_option(apr_array_header_t *config_options, Index: subversion/libsvn_subr/cmdline.c =================================================================== --- subversion/libsvn_subr/cmdline.c (revision 1670590) +++ subversion/libsvn_subr/cmdline.c (working copy) @@ -810,6 +810,116 @@ svn_cmdline__print_xml_prop(svn_stringbuf_t **outs return; } +/* Return the most similar string to NEEDLE in HAYSTACK, which contains + * HAYSTACK_LEN elements. Return NULL if no string is sufficiently similar. + */ +static const char * +most_similar(const char *needle_cstr, + const char **haystack, + apr_size_t haystack_len, + apr_pool_t *scratch_pool) +{ + /* TODO: use svn_cl__similarity_check()? */ + const char *max_similar; + apr_size_t max_score = 0; + apr_size_t i; + svn_membuf_t membuf; + svn_string_t *needle_str = svn_string_create(needle_cstr, scratch_pool); + + svn_membuf__create(&membuf, 64, scratch_pool); + + for (i = 0; i < haystack_len; i++) + { + apr_size_t score; + svn_string_t *hay = svn_string_create(haystack[i], scratch_pool); + + score = svn_string__similarity(needle_str, hay, &membuf, NULL); + + if (score >= (2 * SVN_STRING__SIM_RANGE_MAX + 1) / 3 + && score > max_score) + { + max_score = score; + max_similar = haystack[i]; + } + } + + if (max_score) + return max_similar; + else + return NULL; +} + +/* Verify that NEEDLE is in HAYSTACK, which contains HAYSTACK_LEN elements. */ +static svn_error_t * +string_in_array(const char *needle, + const char **haystack, + apr_size_t haystack_len, + apr_pool_t *scratch_pool) +{ + const char *next_of_kin; + apr_size_t i; + for (i = 0; i < haystack_len; i++) + { + if (!strcmp(needle, haystack[i])) + return SVN_NO_ERROR; + } + + /* Error. */ + next_of_kin = most_similar(needle, haystack, haystack_len, scratch_pool); + if (next_of_kin) + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, + _("Ignoring unknown value '%s'; " + "did you mean '%s'?"), + needle, next_of_kin); + else + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, + _("Ignoring unknown value '%s'"), + needle); +} + +#include "config_keys.inc" + +/* Validate the FILE, SECTION, and OPTION components of CONFIG_OPTION are + * known. Warn to stderr if not. */ +static svn_error_t * +validate_config_option(svn_cmdline__config_argument_t *config_option, + apr_pool_t *scratch_pool) +{ + svn_boolean_t arbitrary_keys = FALSE; + + /* TODO: some day, we could also verify that OPTION is valid for SECTION; + i.e., forbid config:working-copy:diff-extensions and similar invalid + combinations. */ + +#define ARRAYLEN(x) ( sizeof((x)) / sizeof((x)[0]) ) + + SVN_ERR(string_in_array(config_option->file, svn__valid_config_files, + ARRAYLEN(svn__valid_config_files), + scratch_pool)); + SVN_ERR(string_in_array(config_option->section, svn__valid_config_sections, + ARRAYLEN(svn__valid_config_sections), + scratch_pool)); + + /* Don't validate option names for sections such as servers[group], + * config[tunnels], and config[auto-props] that permit arbitrary options. */ + { + int i; + + for (i = 0; i < ARRAYLEN(svn__empty_config_sections); i++) + if (!strcmp(config_option->section, svn__empty_config_sections[i])) + arbitrary_keys = TRUE; + } + + if (! arbitrary_keys) + SVN_ERR(string_in_array(config_option->option, svn__valid_config_options, + ARRAYLEN(svn__valid_config_options), + scratch_pool)); + +#undef ARRAYLEN + + return SVN_NO_ERROR; +} + svn_error_t * svn_cmdline__parse_config_option(apr_array_header_t *config_options, const char *opt_arg, @@ -826,6 +936,8 @@ svn_cmdline__parse_config_option(apr_array_header_ if ((equals_sign = strchr(second_colon + 1, '=')) && (equals_sign != second_colon + 1)) { + svn_error_t *warning; + config_option = apr_pcalloc(pool, sizeof(*config_option)); config_option->file = apr_pstrndup(pool, opt_arg, first_colon - opt_arg); @@ -834,6 +946,14 @@ svn_cmdline__parse_config_option(apr_array_header_ config_option->option = apr_pstrndup(pool, second_colon + 1, equals_sign - second_colon - 1); + warning = validate_config_option(config_option, pool); + if (warning) + { + /* TODO: before committing, add 'prefix' argument */ + svn_handle_warning2(stderr, warning, "svn: "); + svn_error_clear(warning); + } + if (! (strchr(config_option->option, ':'))) { config_option->value = apr_pstrndup(pool, equals_sign + 1, Index: subversion/tests/cmdline/getopt_tests.py =================================================================== --- subversion/tests/cmdline/getopt_tests.py (revision 1670590) +++ subversion/tests/cmdline/getopt_tests.py (working copy) @@ -223,6 +223,18 @@ def getopt_help_bogus_cmd(sbox): "run svn help bogus-cmd" run_one_test(sbox, 'svn_help_bogus-cmd', 'help', 'bogus-cmd') +def getopt_config_option(sbox): + "--config-option's spell checking" + sbox.build(create_wc=False, read_only=True) + expected_stderr = '.*W205000.*did you mean.*' + expected_stdout = svntest.verify.AnyOutput + svntest.actions.run_and_verify_svn(expected_stdout, expected_stderr, + 'info', + '--config-option', + 'config:miscellanous:diff-extensions=' + + '-u -p', + '--', sbox.repo_url) + ######################################################################## # Run the tests @@ -237,6 +249,7 @@ test_list = [ None, getopt_help, getopt_help_bogus_cmd, getopt_help_log_switch, + getopt_config_option, ] if __name__ == '__main__':