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.
>
Received on 2014-04-29 13:21:59 CEST