Julian Foad <julianf...@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.
