On Fri, Apr 5, 2013 at 7:47 PM, Daniel Shahaf <danielsh_at_elego.de> wrote:
> THanks for the review, commnts inline:
>
Sorry for late reply. See my reply inline.
> Ivan Zhakov wrote on Fri, Apr 05, 2013 at 19:40:16 +0400:
>> On Fri, Apr 5, 2013 at 6:47 PM, <danielsh_at_apache.org> wrote:
>> > + /** Filesystem backend (#fs_type) -specific information.
>> > + * @see SVN_FS_FSFS_INFO_* */
>> > + apr_hash_t *fsap_info;
>> > +
>> What is the values of this hash table? Always strings? What about
>> integers or booleans?
>>
>
> Each SVN_FS_FSFS_INFO_* macro will define its value's
> type. I suppose
> we can encourage the values to be, say, C strings in most cases.
>
It would be great to guarantee them to be C strings in this case
svnadmin info or any other program could print them as opaque.
>> > + /** List of user-serviceable config files.
>> > + * Elements are dirents (as const char *). */
>> > + apr_array_header_t *config_files;
>> I don't see real use why config_files information can be useful in
>> svnadmin info command. So I suggest to remove it until we need it.
>>
>
> ACK. I assumed this would be useful. What do others think?
> (this would contain ["DB_CONFIG"] or ["fsfs.conf"], currently.)
>
>> > +/** Value: abspath to rep-cache.db, or absent if that doesn't exist.
>> > + @note Do not modify the db schema or tables!
>> > + */
>> > +#define SVN_FS_FSFS_INFO_REP_CACHE_PATH "rep-cache-path"
>> I suggest to replace it with boolean REP_CACHE_ENABLED. In most cases
>> user is interested wheither rep caching is enabled or not.
>> rep-cache.db stored in stable location.
>
> My rationale was that giving the path is more useful than just giving
> a "1" bit. I assume most users will just check
> svn_hash_gets(fsap_info, SVN_FS_FSFS_INFO_REP_CACHE_PATH) != NULL
> , but if someone wants to open the sqlite db and get some statistics
> they have the path right there.
>
I think we should leave it just yes/no value. Otherwise we implicitly
disclose (and promise for future) how rep cache database is stored.
Another argument that rep-cache.db can exist while rep-cache disabled
by fsfs.conf.
Could you please change it to SVN_FS_FSFS_INFO_REP_CACHE_ENABLED? At
least for 1.8.
>> > + /** Set of basenames of hook scripts which have been installed.
>> > + * Keys are C strings such as "post-commit", values are undefined. */
>> > + apr_hash_t *hooks_enabled;
>> > +
>> > + /** Set of basenames of hook scripts which are respected by this version of
>> > + * Subversion. Keys are C strings such as "post-commit", values are
>> > + * undefined.
>> > + *
>> > + * @note Hooks are sometimes extended (e.g., by passing additional arguments
>> > + * to them). In the future we might extend the semantics of this hash to
>> > + * describe that case, for example by adding keys or defining a meaning for
>> > + * the values.
>> > + */
>> > + apr_hash_t *hooks_known;
>> > +
>> Do we really need information about hooks in svnadmin info command?
>> What is the behavior is hook file is present but empty? I suggest to
>> remove this information from svn_repos_info_t and implemented as
>> different API if needed.
>
> It wasn't my idea to add this info in the first place. If people think
> this needs to go it can go.
>
I see that you dropped svn_repos_info_t structure completely. I don't
have strong opinion on this, but API with svn_repos_info_t with basic
information (format, supported version and FS info) was much cleaner
and easier to understand for my taste.
--
Ivan Zhakov
Received on 2013-04-09 23:27:39 CEST