I don't think we should fix this with a 'revision+1' to explicitly allow many bad ranges, but I do think we should specifically fix the r0 case for empty repositories.
Bert Sent from Mail for Windows 10 From: Julian Foad Sent: zaterdag 5 september 2015 12:02 To: dev;Bert Huijben Subject: svnsync crash on empty repo Hi, Bert. On trunk (released versions are unaffected), "svnsync sync" with an empty source repo fails an assertion in svn_ra_replay_range(): subversion/libsvn_ra/ra_loader.c' line 1198: assertion failed (SVN_IS_VALID_REVNUM(start_revision) && SVN_IS_VALID_REVNUM(end_revision) && start_revision <= end_revision && SVN_IS_VALID_REVNUM(low_water_mark)) because start_revision is 1 and end_revision is 0. The assertion was added in r1665480 <http://svn.apache.org/viewvc?revision=1665480&view=revision>, with the log msg mentioning "Add assertions here that already apply to some ra layers." One possible fix is to avoid calling it with an empty revision range, like this: Index: subversion/svnsync/svnsync.c =================================================================== --- subversion/svnsync/svnsync.c (revision 1701278) +++ subversion/svnsync/svnsync.c (working copy) @@ -1551,3 +1551,3 @@ do_synchronize(svn_ra_session_t *to_sess - if (from_latest < last_merged) + if (from_latest <= last_merged) return SVN_NO_ERROR; That makes the caller return early when there are no revisions to sync. The only other real caller (svnrdump) already uses a similar condition. However, I think we need to continue allowing the historical usage of svn_ra_replay_range() for backward compatibility. Should the assertion should be changed like this? - && start_revision <= end_revision + && start_revision <= (end_revision + 1) I haven't yet seen where the problem or restriction existed in some RA layers, so I don't know if that's a good enough fix on its own. My new svnsync test (empty repo) passes over all RA layers with this change. The attached patch contains both possible fixes, a log msg and a regression test. (I found this by running tests with the "--dump-load-cross-check" option, which I have not done for a while. Doing so also reports several other test failures, mostly just because those tests use deliberately invalid data.) - Julian
