[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1590751 - /subversion/trunk/subversion/svn/svn.c

From: Philip Martin <philip.martin_at_wandisco.com>
Date: Tue, 29 Apr 2014 12:06:57 +0100

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

> 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

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.