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, <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.
> > + /** 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
Received on 2013-04-05 17:48:02 CEST