On Fri, Mar 19, 2010 at 10:26 AM, C. Michael Pilato <cmpil...@collab.net> wrote:
> 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.)
Ah, yes, that's what I meant; that patch looks great, assuming it
works :) I would like to get a working Subversion development
environment back one of these days, once I re-derive how to work
around the Debian libtool issue again...
thanks,
dave
> --
> C. Michael Pilato <cmpil...@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)
> {
>
>
--
glas...@davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
