On Thu, Apr 25, 2013 at 2:01 AM, <danie...@apache.org> wrote: > URL: http://svn.apache.org/r1471717 > Log: > Implement 'svnadmin info'. > See my review inline.
> Modified: > subversion/trunk/subversion/svnadmin/svnadmin.c > > Modified: subversion/trunk/subversion/svnadmin/svnadmin.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/svnadmin/svnadmin.c?rev=1471717&r1=1471716&r2=1471717&view=diff > ============================================================================== > --- subversion/trunk/subversion/svnadmin/svnadmin.c (original) > +++ subversion/trunk/subversion/svnadmin/svnadmin.c Wed Apr 24 22:01:17 2013 [...] > +svn_error_t * > +subcommand_info(apr_getopt_t *os, void *baton, apr_pool_t *pool) > +{ > + struct svnadmin_opt_state *opt_state = baton; > + svn_repos_t *repos; > + svn_fs_t *fs; > + > + /* Expect no more arguments. */ > + SVN_ERR(parse_args(NULL, os, 0, 0, pool)); > + > + SVN_ERR(open_repos(&repos, opt_state->repository_path, pool)); > + fs = svn_repos_fs(repos); > + SVN_ERR(svn_cmdline_printf(pool, _("Path: %s\n"), > + svn_repos_path(repos, pool))); Missing svn_dirent_local_style(). > + > + { > + apr_hash_t *capabilities_set; > + apr_array_header_t *capabilities; > + char *as_string; > + > + SVN_ERR(svn_repos_capabilities(&capabilities_set, repos, pool, pool)); > + SVN_ERR(svn_hash_keys(&capabilities, capabilities_set, pool)); > + as_string = svn_cstring_join(capabilities, ",", pool); > + > + /* 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) { apr_array_header_t *capabilities; char *as_string; SVN_ERR(svn_hash_keys(&capabilities, capabilities_set, pool)); as_string = svn_cstring_join(capabilities, ",", pool); as_string[strlen(as_string)-1] = '\0'; SVN_ERR(svn_cmdline_printf(pool, _("Repository capabilities: %s\n"), as_string)); } ]]] > + > + if (capabilities->nelts) > + SVN_ERR(svn_cmdline_printf(pool, _("Repository capabilities: %s\n"), > + as_string)); > + } > + > + { > + int repos_format, fs_format, minor; One variable per line makes code much cleaner. > + svn_version_t *repos_version, *fs_version; > + SVN_ERR(svn_repos_info_format(&repos_format, &repos_version, > + repos, pool, pool)); > + SVN_ERR(svn_cmdline_printf(pool, _("Repository format: %d\n"), > + repos_format)); > + > + SVN_ERR(svn_fs_info_format(&fs_format, &fs_version, > + fs, pool, pool)); > + SVN_ERR(svn_cmdline_printf(pool, _("Filesystem format: %d\n"), > + fs_format)); > + > + SVN_ERR_ASSERT(repos_version->major == SVN_VER_MAJOR); > + SVN_ERR_ASSERT(fs_version->major == SVN_VER_MAJOR); > + SVN_ERR_ASSERT(repos_version->patch == 0); > + SVN_ERR_ASSERT(fs_version->patch == 0); > + > + 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 avoid hard-coding SVN_VER_MAJOR and make code easier to understand. > + SVN_ERR(svn_cmdline_printf(pool, _("Compatible with version: %d.%d.0\n"), > + SVN_VER_MAJOR, minor)); > + } > + > + { > + apr_array_header_t *files; > + int i; > + > + SVN_ERR(svn_fs_info_config_files(&files, fs, pool, pool)); > + for (i = 0; i < files->nelts; i++) Loop without '{' and '}' is very dangerous :) > + SVN_ERR(svn_cmdline_printf(pool, _("Config file: %s\n"), > + APR_ARRAY_IDX(files, i, const char *))); > + } > + > + { > + const svn_fs_info_placeholder_t *info; > + > + SVN_ERR(svn_fs_info(&info, fs, pool, pool)); > + SVN_ERR(svn_cmdline_printf(pool, _("Filesystem type: %s\n"), > + info->fs_type)); > + if (!strcmp(info->fs_type, SVN_FS_TYPE_FSFS)) > + { > + const svn_fs_fsfs_info_t *fsfs_info = (const void *)info; > + svn_revnum_t youngest; > + SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool)); > + > + if (fsfs_info->shard_size) > + SVN_ERR(svn_cmdline_printf(pool, _("FSFS sharded: yes\n"))); > + else > + SVN_ERR(svn_cmdline_printf(pool, _("FSFS sharded: no\n"))); > + > + if (fsfs_info->shard_size) > + SVN_ERR(svn_cmdline_printf(pool, _("FSFS shard size: %d\n"), > + fsfs_info->shard_size)); > + > + if (fsfs_info->min_unpacked_rev + fsfs_info->shard_size > youngest + > 1) > + SVN_ERR(svn_cmdline_printf(pool, _("FSFS packed: yes\n"))); > + else if (fsfs_info->min_unpacked_rev) > + SVN_ERR(svn_cmdline_printf(pool, _("FSFS packed: partly\n"))); > + else > + SVN_ERR(svn_cmdline_printf(pool, _("FSFS packed: no\n"))); > + } > + } > + > + return SVN_NO_ERROR; > +} > + > /* This implements `svn_opt_subcommand_t'. */ > static svn_error_t * > subcommand_lock(apr_getopt_t *os, void *baton, apr_pool_t *pool) > > -- Ivan Zhakov CTO | VisualSVN | http://www.visualsvn.com
