Thanks Daniel, these are really good pointers for me. Shall I continue
with this and submit a patch
with the changes.
Regards
Prabhu
On 10/21/2012 10:42 PM, Daniel Shahaf wrote:
> 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-26 11:45:00 CEST