On Fri, Apr 5, 2013 at 7:47 PM, Daniel Shahaf <danie...@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, <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. > 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
