Philip Martin wrote: > Julian Foad writes: >>> URL: http://svn.apache.org/r1590751 >> >> 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 didn't mean we should change svn_hash_gets(), I meant to avoid calling it if we have no hash. > 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. My main point is that the way this hash (pointer) gets from here to the Subversion libraries is through the call to svn_client_create_context2(). That is already documented to accept NULL. So that seems like the best place to handle any necessary default-construction. That also sets the precedent for whether the config hash pointers passed around the system generally may or may not be null. - Julian >> 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. >
