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

Reply via email to