[PATCH] Fix 'svnadmin recover' regression crash with old repositories

Hi,

I've noticed that merge of the FSFS format 7 branch in r1546842 [1] introduced
a regression: 'svnadmin recover' now crashes on the --pre-1.4-compatible FSFS
repositories. If you create a repository using the 'svnadmin create
--compatible-version=1.3 / 1.2 / 1.1' or take any existing repository with FSFS
format <= 3, attempt to recover it will coredump:
[[[
  Repository lock acquired.
  Please wait; recovering the repository may take some time...
  Floating point exception (core dumped)
]]]

More interesting, we have a test (namely "recover --pre-1.4-compatible"), which
should have failed upon this regression, however, the test is seriously broken
and always reports false positive (PASS). I propose a series of 3 patches that
sequentially fix the test (in order to get the expected FAIL) and then the
crash itself.

So, "recover --pre-1.4-compatible" test is a regression test for the issue
4213 [2]. It looks pretty simple, ...
[[[
  svntest.main.safe_rmtree(sbox.repo_dir, 1)
  svntest.main.create_repos(sbox.repo_dir, minor_version=0)
  svntest.main.run_svnadmin("recover", sbox.repo_dir)
]]]

...but has several problems:

1) First of all, creating an FSFS repository with MINOR_VERSION set to '0' (or
   equivalently, running 'svnadmin create --compatible-version=1.0') is
   prohibited since r1494223 [3] and should error out like this:
   [[[
   svnadmin: E205000: Repositories compatible with 1.0.x must use --fs-type=bdb
   ]]]

   So, the corresponding test should fail during the repository creation phase,
   but it doesn't. This happens due to a mistake in the 'create_repos' method,
   which incorrectly uses the "if not" statement to handle the absence of
   MINOR_VERSION. This check effectively is the "if MINOR_VERSION is None or
   MINOR_VERSION == 0" and we end up with creating a 1.9-compatible repository
   in a "--pre-1.4-compatible" regression test. I've changed that statement to
   a "if MINOR_VERSION is None" (following PEP8 [4]), see the attached patch #1

2) Now, applying patch #1 leaves us with two other problems (minor and major):

    - Minor: we cannot create a 1.0-compatible FSFS repository, but still want
      to have a regression test for issue 4213 [2]. So, I've bumped the
      MINOR_REVISION to 3, thus making the test initialization equivalent to
      the 'svnadmin create --pre-1.4-compatible' call (as noted in the test
      docstring).

    - Major: currently the test *checks nothing*, because simply calling the
      'run_svnadmin' method does not check EXIT_CODE, STDOUT or STDERR.
      So, even with patched svnadmin that errors out on any 'recover' command,
      the test still passes (and it still passes with crashing svnadmin!). In
      order to fix this, I've replaced the 'run_svnadmin' call with
      'run_and_verify_svnadmin'.

    (These changes are combined in my patch #2)

With patches #1 and #2 applied, we finally have the expected FAIL in the test
("svnadmin recover terminated by signal 8") and can proceed to fixing it. The
crash happens due to the way 'svn_fs_fs__find_max_ids' function uses the new
svn_fs_fs__revision_file_t structure since r1532029 [5]. It calls the
'svn_fs_fs__item_offset' function with uninitialized REV_FILE and this
leads to a coredump for old repositories. REV_FILE is initialized (via
'svn_fs_fs__open_pack_or_rev_file') right after the erroneous
'svn_fs_fs__item_offset' call, so swapping the initialization and offset
evaluation should do the trick. I've attached the valgrind output and the
patch #3, which fixes this issue.

All tests (including the fixed one) pass.

NOTE: In order to prove that after all these manipulations the "recover
--pre-1.4-compatible" is a correct regression test for issue 4213 [2], I've
checked that undoing the r1367683 changeset [6] fails the test with the
expected error (as described in the issue):
[[[
  svnadmin: E000002: Can't open file
  'svn-test-work/repositories/svnadmin_tests-30/db/revs/1'
]]]

[1] http://svn.apache.org/viewvc?view=revision&revision=r1546842
[2] http://subversion.tigris.org/issues/show_bug.cgi?id=4213
[3] http://svn.apache.org/viewvc?view=revision&revision=r1494223
[4] http://www.python.org/dev/peps/pep-0008/#programming-recommendations
[5] http://svn.apache.org/viewvc?view=revision&revision=r1532029
[6] http://svn.apache.org/viewvc?view=revision&revision=r1367683

Thanks and regards,
Evgeny Kotkov