Re: [patch] fsfs revprop packing paranoia

[Daniel Shahaf]

Philip Martin wrote on Fri, Aug 13, 2010 at 11:13:10 +0100:
> Daniel Shahaf <d...@daniel.shahaf.name> writes:
> 
> > I figure this patch can't do any harm (except cost another file-read when a
> > write-lock or txn-current-lock is being acquired), and it might as well
> > help, so I committed it in r984990.
> 
> >> @@ -594,7 +598,11 @@ static svn_error_t *
> >>    err = get_lock_on_filesystem(lock_filename, subpool);
> >>  
> >>    if (!err)
> >> -    err = body(baton, subpool);
> >> +    {
> >> +      SVN_ERR(update_min_unpacked_rev(fs, pool));
> >> +      SVN_ERR(update_min_unpacked_revprop(fs, pool));
> >> +      err = body(baton, subpool);
> >> +    }
> 
> This makes 1.6 repositories incompatible with the 1.7 client:
> 

Easier recipe:

% ./subversion/svnadmin/svnadmin create r --pre-1.7-compatible
subversion/libsvn_subr/io.c:2858: (apr_err=235000)
svn: In file 'subversion/libsvn_fs_fs/fs_fs.c' line 1199: assertion failed 
(ffd->format >= SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT)
zsh: abort      ./subversion/svnadmin/svnadmin create r --pre-1.7-compatible

> $ rm -rf repo && svnadmin create repo                   # using 1.6
> $ subversion/svn/svn mkdir -mm file://`pwd`/repo/A      # using 1.7
> ../src/subversion/libsvn_fs_fs/fs_fs.c:565: (apr_err=235000)
> svn: In file '../src/subversion/libsvn_fs_fs/fs_fs.c' line 1199: assertion 
> failed (ffd->format >= SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT)
> Aborted

The following causes the assertion to disappear.  (Despite the bad results
the last time I said so,) I'll commit it if I don't hear any objections.

[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c     (revision 984990)
+++ subversion/libsvn_fs_fs/fs_fs.c     (working copy)
@@ -599,8 +599,11 @@ with_some_lock()
 
   if (!err)
     {
-      SVN_ERR(update_min_unpacked_rev(fs, pool));
-      SVN_ERR(update_min_unpacked_revprop(fs, pool));
+      fs_fs_data_t *ffd = fs->fsap_data;
+      if (ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT)
+        SVN_ERR(update_min_unpacked_rev(fs, pool));
+      if (ffd->format >= SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT)
+        SVN_ERR(update_min_unpacked_revprop(fs, pool));
       err = body(baton, subpool);
     }
 
@@ -2789,7 +2792,6 @@ set_revision_proplist()
 
   SVN_ERR(ensure_revision_exists(fs, rev, pool));
 
-  SVN_ERR(update_min_unpacked_revprop(fs, pool));
   if (ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT ||
       rev >= ffd->min_unpacked_revprop)
     {
]]]

Thanks for the catch,

Daniel