Ivan Zhakov wrote on Thu, Apr 25, 2013 at 02:58:05 +0400:
> On Thu, Apr 25, 2013 at 2:01 AM, <danie...@apache.org> wrote:
> > +svn_error_t *
> > +subcommand_info(apr_getopt_t *os, void *baton, apr_pool_t *pool)
> > +{
> > + SVN_ERR(svn_cmdline_printf(pool, _("Path: %s\n"),
> > + svn_repos_path(repos, pool)));
> Missing svn_dirent_local_style().
>
Fixed. (I actually wanted to do that before commit, but was distracted
and forgot about it.)
> > + /* Delete the trailing comma. */
> > + as_string[strlen(as_string)-1] = '\0';
> This will crash in case of empty string. Simple solution would be:
> [[[
> if (apr_hash_count(capabilities_set) > 0)
> ]]]
>
Fixed differently.
> > + {
> > + int repos_format, fs_format, minor;
> One variable per line makes code much cleaner.
>
*nod*. Is that your preference or the project-agreed style?
> > + SVN_ERR_ASSERT(repos_version->major == SVN_VER_MAJOR);
> > + SVN_ERR_ASSERT(fs_version->major == SVN_VER_MAJOR);
> > +
> > + minor = (repos_version->minor > fs_version->minor)
> > + ? repos_version->minor : fs_version->minor;
> It would great to implement svn_ver_compare() and use it here. This
There is svn_ver__at_least() but I didn't want to use non-public
functions.
> avoid hard-coding SVN_VER_MAJOR and make code easier to understand.
>
I hard-coded SVN_VER_MAJOR because of the assert a few lines above which
guarantees the repos/fs major versions are equal to it.
> > + SVN_ERR(svn_cmdline_printf(pool, _("Compatible with version:
> > %d.%d.0\n"),
> > + SVN_VER_MAJOR, minor));
> > + }
> > +
Thanks for the review.
On IRC you also made a point about the "Config files" output (prints the
path to DB_CONFIG or to fsfs.conf). Do you think it should be removed
from 'svnadmin info'? From svn_fs.h (svn_fs_info_config_files)? What
do others think?
FWIW, example output:
[[[
% $svnadmin create r && $svnadmin info r
Path: r
Repository Format: 5
Filesystem Format: 6
Compatible With Version: 1.8.0
Repository Capabilities: mergeinfo
Filesystem Type: fsfs
FSFS Sharded: yes
FSFS Shard Size: 4
FSFS Packed: yes
Config File: r/db/fsfs.conf
% svnadmin create --fs-type=bdb --pre-1.4-compatible r2 && $svnadmin info r2
svnadmin: warning: The "bdb" repository back-end is deprecated, consider using
"fsfs" instead.
Path: r2
Repository Format: 3
Filesystem Format: 1
Compatible With Version: 1.0.0
Filesystem Type: bdb
Config File: r2/db/DB_CONFIG
]]]
