[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Implement svnadmin verify --force

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Sun, 21 Oct 2012 19:12:14 +0200

BTW, I'd like to point out a few further problems with the patch; even
if it doesn't get applied, you might find them useful for your next
patch.

Prabhu Gnana Sundar wrote on Sun, Oct 21, 2012 at 00:57:47 +0530:
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 1398254)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -1397,6 +1398,7 @@
> end_rev, youngest);
>
> /* Verify global/auxiliary data and backend-specific data first. */
> + if (!keep_going)
> SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> start_rev, end_rev, pool));

This hunk is wrong, it causes work to _not be done_, not only to not be
a fatal error.

>
> @@ -1425,7 +1428,21 @@
> notify_func, notify_baton,
> start_rev,
> FALSE, TRUE, /* use_deltas, verify */
> - iterpool));
> + iterpool);
> + if (err && keep_going)
> + {

Wrong indentation.

> + svn_repos_notify_t *notify2 = svn_repos_notify_create(svn_repos_notify_failure, iterpool);
> +
> + notify2->warning_str = _(err->message);
> + notify2->apr_err = _(err->apr_err);

Type mismatch (because _ takes a 'const char *' parameter, and apr_err is
an integral type), probably segfault under --enable-nls.

> + notify2->child_apr_err = _(err->child->apr_err);

Segfault whenever err->child happens to be NULL. (this will almost
never happen in maintainer builds)

OTOH, maintainer builds probably want to skip tracing links here (see
for example svn_error_purge_tracing()).

Why not stow the entire error chain in the notification? Why just the
topmost error and the first child?

> + notify2->child_warning_str = _(err->child->message);
> + notify_func(notify_baton, notify2, iterpool);
> + continue;

Error leak (ERR needs to be cleared) --- this should have caused svn to
assert at exit if you ran this block in maintainer mode.

> + }
> + else
> + SVN_ERR(err);
> +
> SVN_ERR(svn_delta_get_cancellation_editor(cancel_func, cancel_baton,
> dump_editor, dump_edit_baton,
> &cancel_editor,
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 1398254)
> +++ subversion/include/svn_repos.h (working copy)
> + * @deprecated Provided for backward compatibility with the 1.7 API.
> */
> +

Missing SVN_DEPRECATED decorator.

> svn_error_t *
> svn_repos_verify_fs2(svn_repos_t *repos,
> svn_revnum_t start_rev,

The rest looks good.

Cheers,

Daniel
Received on 2012-10-21 19:12:54 CEST

This is an archived mail posted to the Subversion Dev mailing list.