On Wednesday 09 February 2011 07:02 PM, Daniel Shahaf wrote:
Arwin Arni wrote on Wed, Feb 09, 2011 at 17:39:55 +0530:
* subversion/libsvn_repos/authz.c,
subversion/tests/libsvn_subr/cache-test.c,
subversion/tests/libsvn_subr/config-test.c,
subversion/tests/cmdline/atomic-ra-revprop-change.c,
subversion/svnserve/serve.c,
subversion/libsvn_fs_fs/fs_fs.c
(svn_repos_authz_read, test_memcache_basic, test_memcache_longkey,
test_text_retrieval,test_boolean_retrieval, test_has_section,
construct_config, load_configs, read_config) : Fixed callers.
Thanks for doing this, but please make it a separate patch. It
distracts the review and it'll have to be split out prior to commit
anyway.
Sure. I'll send these separately.
Index: subversion/libsvn_subr/config.c
===================================================================
--- subversion/libsvn_subr/config.c (revision 1068828)
+++ subversion/libsvn_subr/config.c (working copy)
@@ -78,7 +78,9 @@
There is a apr_strnatcasecmp() call which your patch doesn't remove.
Don't you have to change it?
This is the comparison being done for SVN_CONFIG__DEFAULT_SECTION.
I've left this out specifically, so it doesn't matter if the user says
[default] or [DEFAULT]. (kind of a special case)
I'll document this clearly in the code.
This doesn't concern the issue that I'm fixing anyway, because there
isn't a default section in an authz file.
Also: do we parse option names (the 'foo' in foo=bar) case-sensitively
or not? (The current code seems to be insensitive.) Should we allow
for case-sensitive option names (not just section names) while we're
here? (IMO, yes.)
I had initially proposed additional parameters that decide case
sensitivity of optins/values:
http://svn.haxx.se/dev/archive-2011-01/0528.shtml
Mike felt that we'd be over-engineering it. So currently only section
names are parsed case sensitively.
The rest looks good. I'm not +1 yet, because I haven't checked whether
there are any other hunks (other than the apr_strnatcasecmp()) that need
to be added.
Thanks and Regards,
Arwin Arni
