Re: useful extra check in FS fold_change function

David Glasser wrote:
> When dealing with some nasty cases in the almost-retired old Google
> Code subversion backend, I found a kind of data corruption that the FS
> code wasn't catching even though it caught relatively similar issues.
> Specifically, find the fold_change function in both of the FS
> implementations. There's a check:
>
> /* Sanity check: an add, replacement, or reset must be the first
> thing to follow a deletion. */
> if ((old_change->change_kind == svn_fs_path_change_delete)
> && (! ((change->kind == svn_fs_path_change_replace)
> || (change->kind == svn_fs_path_change_reset)
> || (change->kind == svn_fs_path_change_add))))
> return svn_error_create
> (SVN_ERR_FS_CORRUPT, NULL,
> _("Invalid change ordering: non-add change on deleted path"));
>
> It might also be nice to check the opposite condition: if change->kind
> is add or replace, and old_change is not delete (or reset, I guess),
> throw an error. (We had accidentally recorded the sequence "add,
> prop-mod, text-mod" out of order as "prop-mod, add, text-mod", which
> was interpreted as an "R" below instead of an "A"; this suggested
> check would have caught it earlier.)

Shouldn't this be just: "if change->kind is add, and old_change is not
delete, throw an error"? It would be legit for someone to record the
sequence "add, prop-mod, replace, text-mod", for example.

Is the attached patch what you had in mind? (Plus similar logic for FSFS,
of course.)

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Index: subversion/libsvn_fs_base/bdb/changes-table.c
===================================================================
--- subversion/libsvn_fs_base/bdb/changes-table.c (revision 924936)
+++ subversion/libsvn_fs_base/bdb/changes-table.c (working copy)
@@ -168,6 +168,15 @@
           (SVN_ERR_FS_CORRUPT, NULL,
            _("Invalid change ordering: non-add change on deleted path"));
 
+ /* Sanity check: an add can't follow anything except
+ a delete or reset. */
+ if ((change->kind == svn_fs_path_change_add)
+ && (old_change->change_kind != svn_fs_path_change_delete)
+ && (old_change->change_kind != svn_fs_path_change_reset))
+ return svn_error_create
+ (SVN_ERR_FS_CORRUPT, NULL,
+ _("Invalid change ordering: add change on preexisting path"));
+
       /* Now, merge that change in. */
       switch (change->kind)
         {