Issue #4476 - Mergeinfo containing r0 makes svnsync and svnadmin dump fail

http://subversion.tigris.org/issues/show_bug.cgi?id=4476

There's another part to this issue: 'svnadmin load'.

When mergeinfo refers to r0, not only does 'svnadmin dump' fail (fixed in r1574868) and 'svnsync' fail, but 'svnadmin load' also fails, as I found when I started writing a test for the 'svnsync' problem.

I think 'svnadmin load' should always be able to load a dump file in order to restore a backup. There is a 'validate properties' option, so I would say the loading should fail if that is enabled, and succeed if not enabled. When loading 'succeeds' it should load exactly what was in the dump file, and not try to 'correct' it, for any data that could in principle have been dumped out of a repository that was not utterly broken.

(If we want to consider automatically 'correcting' properties during loading, that should be a separate development and should be optional, in other words there should always be a way to load exactly what is in the dump file. That feature should probably be implemented at a higher level than libsvn_repos, too.)

But 'svnadmin load' can do more than just restore revisions exactly how they were: it can also renumber revisions and move all loaded paths into a subdirectory, and when doing either or both of these it attempts to adjust mergeinfo accordingly. In that case if mergeinfo is invalid then we have three options:

  - libsvn_repos throws an error
  - libsvn_repos warns and does not adjust that mergeinfo property
  - svnadmin 'corrects' the mergeinfo, and then libsvn_repos adjusts it

I think warning and not adjusting is better than throwing an error, but I am not sure if 'svnadmin load' should be in the business of correcting bad mergeinfo or if that would be better left to an external tool. I would say this mode of operation (adjusting revision numbers and/or paths) is a lower priority.

PROPOSAL

In the presence of a mergeinfo property that refers to r0, at the libsvn_repos API:

'dump'
  shall dump the property verbatim (with nowarning). (Done.)

'load', with 'validate properties' OFF and not adjusting revision numbers or paths,
  shall load theproperty verbatim, and warn.

'load', with 'validate properties' OFF and adjusting revision numbers or paths,
  shall load the property verbatim (unadjusted), and warn.

'load', with 'validate properties' ON,
  shall fail.

And I would say the same rules should apply to any property that fails the validation rules.

At the application layer:

'svnadmin dump' and 'svnadmin load'
  should behave the same as the repos layer.

Does that sound good as a fix that I can do now and back-port to 1.8.x and 1.7.x?

I'll come to 'svnsync' later, but basically my current thought is it should 'correct' the mergeinfo by removing the r0 reference.

- Julian
Received on 2014-12-02 17:43:27 CET