Re: [PATCH] Implement svnadmin verify --force

Prabhu, some more review comments on your last patch.

Instead of checking for errors in several places within the main loop, put the verification code in a wrapper function and check for errors exactly once where you call that function.  That way you won't miss any.  In your last patch, there is at least one call that could return a verification error that isn't checked -- the call to svn_fs_revision_root().

Run the test suite before posting a patch, and mention the results.  Three of the svnadmin tests fail when I run them with your patch:

  FAIL:  svnadmin_tests.py 11: verify a repository containing paths like 'c:hi'
  FAIL:  svnadmin_tests.py 19: svnadmin verify detects invalid revprops file
  FAIL:  svnadmin_tests.py 23: svnadmin verify with non-UTF-8 paths

Look for compiler warnings, and address any that indicate problems.  I see two:

  subversion/libsvn_repos/dump.c:1363:1: no previous prototype for 'notify_verification_error'
  subversion/libsvn_repos/dump.c:1442:20: declaration of 'err' shadows a previous local

Include the log message each time you send the patch, even if the log message is the same as it was the previous time, so we don't have to hunt for it.

Thanks.
- Julian
Received on 2012-11-01 19:50:48 CET