Daniel Shahaf wrote on Sat, 27 Dec 2008 at 23:42 +0200:
> Hyrum K. Wright wrote on Sat, 27 Dec 2008 at 14:00 -0600:
> > Daniel Shahaf wrote:
> > > P.S. I've also noticed that 'svnadmin recover svn-r9-packed' (where
> > > svn-r9-packed is the repository created by the script I gave in the
> > > original mail) fails (both with and without the patch) with the error:
> > >
> > > ..\..\..\subversion\libsvn_fs_fs\fs_fs.c:6155: (apr_err=160004)
> > > svnadmin: Expected current rev to be <= 0 but found 9
> >
> > Does this happen with the inconsistent repository you created, or with the sync
> > running to some sort of completion? What about a non-packed repository which
> > was aborted?
> >
>
> It happens on a 3-revisions packed Greek repository, and doesn't happen
> on the same repository after I dump|load it to get a non-packed version.
> The results with the packed/non-packed syncs of the svn repository are
> consistent (error only with the packed sync). In short, it seems to
> happen iff the repos is packed.
>
And a patch:
[[[
Fix 'svnadmin recover' of packed FSFS repositories.
* subversion/libsvn_fs_fs/fs_fs.c
(recover_get_largest_revision):
Use open_pack_or_rev_file() instead of svn_fs_fs__path_rev() to look
for the rev files.
]]]
[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 34960)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -5878,13 +5881,19 @@ recover_get_largest_revision(svn_fs_t *fs, svn_rev
/* Keep doubling right, until we find a revision that doesn't exist. */
while (1)
{
- svn_node_kind_t kind;
- SVN_ERR(svn_io_check_path(svn_fs_fs__path_rev(fs, right, iterpool),
- &kind, iterpool));
+ svn_error_t *err;
+ apr_file_t *file;
+
+ err = open_pack_or_rev_file(&file, fs, right, iterpool);
svn_pool_clear(iterpool);
- if (kind == svn_node_none)
- break;
+ if (err && err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
+ {
+ svn_error_clear(err);
+ break;
+ }
+ else
+ SVN_ERR(err);
right <<= 1;
}
@@ -5896,16 +5905,22 @@ recover_get_largest_revision(svn_fs_t *fs, svn_rev
while (left + 1 < right)
{
svn_revnum_t probe = left + ((right - left) / 2);
- svn_node_kind_t kind;
+ svn_error_t *err;
+ apr_file_t *file;
- SVN_ERR(svn_io_check_path(svn_fs_fs__path_rev(fs, probe, iterpool),
- &kind, iterpool));
+ err = open_pack_or_rev_file(&file, fs, probe, iterpool);
svn_pool_clear(iterpool);
- if (kind == svn_node_none)
- right = probe;
+ if (err && err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
+ {
+ svn_error_clear(err);
+ right = probe;
+ }
else
- left = probe;
+ {
+ SVN_ERR(err);
+ left = probe;
+ }
}
svn_pool_destroy(iterpool);
]]]
The root of the problem was that svn_fs_fs__path_rev() only works
correctly for non-packed revs. I suggest also the following patch:
[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 34960)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -196,6 +196,9 @@ svn_fs_fs__path_rev(svn_fs_t *fs, svn_revnum_t rev
if (ffd->max_files_per_dir)
{
+ /* We assume here that REV is not yet in a pack file. */
+ assert(rev >= ffd->min_unpacked_rev);
+
return svn_path_join(path_rev_shard(fs, rev, pool),
apr_psprintf(pool, "%ld", rev),
pool);
Index: subversion/libsvn_fs_fs/fs_fs.h
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.h (revision 34960)
+++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
@@ -402,6 +402,7 @@ svn_error_t *svn_fs_fs__dup_perms(const char *file
apr_pool_t *pool);
/* Return the path to the file containing revision REV in FS.
+ Only works when REV has not yet been packed.
Allocate the new char * from POOL. */
const char *svn_fs_fs__path_rev(svn_fs_t *fs,
svn_revnum_t rev,
]]]
I have checked all callers of __dup_perms(), and I think they should all
be okay, except for this one:
[[[
Index: subversion/libsvn_fs_fs/lock.c
===================================================================
--- subversion/libsvn_fs_fs/lock.c (revision 34960)
+++ subversion/libsvn_fs_fs/lock.c (working copy)
@@ -206,7 +206,7 @@ write_digest_file(apr_hash_t *children,
SVN_ERR(svn_stream_close(stream));
SVN_ERR(svn_io_file_rename(tmp_path, digest_path, pool));
- return svn_fs_fs__dup_perms
+ return svn_fs_fs__dup_perms /* XXX triggers the assertion here */
(digest_path, svn_fs_fs__path_rev(fs, 0, pool), pool);
}
]]]
which triggers the assertion added by the second patch. (This code path
is run during 'svn lock'.)
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=994384
Received on 2008-12-28 14:48:39 CET