On Thu, Apr 25, 2013 at 2:01 AM, <danielsh_at_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
Received on 2013-04-25 00:58:57 CEST