Daniel Rall wrote:
> On Fri, 14 Jul 2006, C. Michael Pilato wrote:
> ...
>
>>> trunk/subversion/include/svn_config.h
>>> trunk/subversion/libsvn_subr/config_file.c
>>>
>>>Log:
>>>Fix broken API contract and numerous error leaks in
>>>svn_config_ensure(). The broken API contract resulted an error like
>>>the following when $HOME isn't accessible:
>>>
>>> svn: Can't check path '/home/thiru/.subversion': Permission denied
>>
>>How is that a broken API contract? The contact mentions exactly two cases
>>in which errors should *not* be returned:
>>
>> - something exists but is the wrong kind
>> - you fail while trying to create something
>>
>>The "permission denied" case you site fits neither of those. So, where's
>>the problem? (I mean, besides those error leaks, which really *are* no-nos.)
>
>
> Mike, thanks for the review. The API states:
>
> "Also don't error if trying to create something and failing -- it's
> okay for the config area or its contents not to be created."
>
> While we're not performing a specific write operation where this error
> occurs (svn_io_check_path() on CONFIG_DIR), we are returning an error
> due to insufficient permissions to the file system where we plan to
> subsequently create our config area and templates.
>
> This violates the spirit of this API -- we should not return an error
> due to insufficient permissions when we can bow out gracefully. Can
> you suggest a better phrasing for the doc string which would make this
> more explicit?
I see as different "try to create something new but can't" and "try to read
something existing but can't". If Subversion can't create from scratch a
new configuration area, it's okay to silently move along, because its
behavior will fallback to the exact same values that would have been written
to that new configuration area. However, if Subversion can't read from an
existing configuration area, it's *not* okay to silently move along, because
the default values it chooses could differ from the possibly hand-tweaked
configuration in the area it tried to read but couldn't. I think users will
want to know about such a situation, so they can either fix the permissions
on the configuration directory, or use --config-dir to tell the Subversion
binaries to use a different one.
--
C. Michael Pilato <cmpilato@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on Fri Jul 14 19:28:34 2006