THanks for the review, commnts inline: Ivan Zhakov wrote on Fri, Apr 05, 2013 at 19:40:16 +0400: > On Fri, Apr 5, 2013 at 6:47 PM, <danie...@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. > > + /** 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. > > + /** 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. Cheers, Daniel
