Julian Foad <julianfoad_at_btopenworld.com> writes:
>> Author: philip
>
>> URL: http://svn.apache.org/r1590751
>> Log:
>> If reading the users config fails, say because $HOME is unreadable,
>> provide an empty config rather than a NULL config. This fixes a
>> SEGV and allows command line options that override the default
>> config to work.
>
> Hi Philip.
>
> I'm wondering why you create the two SVN_CONFIG_CATEGORY_... entries
> in the hash? If there's a need for a standard empty config to be
> created that's more than just an empty hash, we should have a
> constructor function for it.
The config API is a bit confusing. At the top level there is no config
type and practically no API, it's just a hash and apr_hash_get/set. The
thing that looks like an API, svn_config_t and svn_config_xxx(), is
really an API to the level below the hash. svn_config_get_config() is a
bit of an exception.
I suppose we could add a constructor to create "a hash with the right
content". What should we call such a function? We can't use
svn_config_create() as that already means something different.
Perhaps it's svn_config_get_config() that should change. Perhaps it
should map EACCESS and ENOTDIR to some explict config error and not
return a NULL hash?
> The main thing 'svn' does with the config hash is pass it to
> svn_client_create_context2(), and that already claims to accept
> NULL. The only other things we do with it are to call svn_hash_gets(),
I was a bit dubious about changing svn_hash_gets as it is currently a
thin wrapper around apr_hash_get. I feel a bit uneasy about allowing
NULL to be passed everywhere that svn_hash_gets is used. Are there
places where we need the hash to exist? I don't know if there are any
such places, and if there are then they should already have an explict
check.
> which could be trivially conditionalized, and call
> svn_cmdline__apply_config_options(). That last call is the only place
> where we need to actually create a hash. Any reason not to do it
> there?
svn_cmdline__apply_config_options might be a better place if the
svn_hash_gets change is acceptable.
> What about the other command-line programs, don't they want the same?
> svnmucc had the same code in it, and svnrdump and svnsync both call
> svn_config_get_config() without any error handling at all, and
> presumably all of these should handle these conditions like 'svn'
> does. svnadmin has a config_dir option but doesn't use it. (We should
> maybe fix its help to say it's unused, or remove the option, or
> something.) The other programs don't use a config dir.
>
> I note that you proposed this for back-port to 1.8.
I suppose I was being lazy, adding a constructor makes the backport more
work.
Yes, some other commands are affected. svnadmin is interesting, in
http://subversion.tigris.org/issues/show_bug.cgi?id=1385 I added a
comment "svnadmin will error rather than create a missing config" so I
guess ---config-dir had an effect some time in the past.
Received on 2014-04-29 13:07:34 CEST