Daniel Shahaf wrote on Wed, Oct 19, 2011 at 15:30:45 +0200:
> Stefan Sperling wrote on Wed, Oct 19, 2011 at 15:14:46 +0200:
> > On Wed, Oct 19, 2011 at 03:07:52PM +0200, Daniel Shahaf wrote:
> > > People who are bitten by this will not be able to commit anything with
> > > this revision applied.
> >
> > Why? I thought this was some transient error, and that we prevent a bad
> > revision from enterting the repository.
> >
> > > Should we add a way to turn off the new validation?
> >
> > If you're saying that this can prevent people from committing new
> > revisions, because a wrong pred count in a transaction is a permanent,
> > rather than transient error condition, I think we should pull this
> > revision from 1.7.x.
> >
> > But why do you think that every transaction will have a wrong pred
> > count once one transaction had a wrong pred count?
>
> The revision works as intended for new repositories. But for
> repositories where the predecessor count on "/" is already wrong, the
> error doesn't fix itself...
>
> If /@50's predecessor is /@20, then /@50's pred-count is 21; but /@51's
> pred-count would be 22, not 51.
>
> I'm working on a different fix that doesn't have this problem: checking
> if 50 - 21 == 51 - 22, and erroring out if that's not the case.
And here's the patch. It passes basic_tests and I verified manually
that if I break the count: and pred: for /@2 then /@3 commits
successfully with consistently-wrong values. If no objections I'll
commit this and nominate for backport.
(and yes, I'll fix the #define, too)
[[[
Index: subversion/libsvn_fs_fs/tree.c
===================================================================
--- subversion/libsvn_fs_fs/tree.c (revision 1185907)
+++ subversion/libsvn_fs_fs/tree.c (working copy)
@@ -870,11 +870,11 @@ add_change(svn_fs_t *fs,
/* Get the id of a node referenced by path PATH in ROOT. Return the
id in *ID_P allocated in POOL. */
-static svn_error_t *
-fs_node_id(const svn_fs_id_t **id_p,
- svn_fs_root_t *root,
- const char *path,
- apr_pool_t *pool)
+svn_error_t *
+svn_fs_fs__node_id(const svn_fs_id_t **id_p,
+ svn_fs_root_t *root,
+ const char *path,
+ apr_pool_t *pool)
{
if ((! root->is_txn_root)
&& (path[0] == '\0' || ((path[0] == '/') && (path[1] == '\0'))))
@@ -895,6 +895,7 @@ add_change(svn_fs_t *fs,
}
return SVN_NO_ERROR;
}
+#define fs_node_id svn_fs_fs__node_id
svn_error_t *
Index: subversion/libsvn_fs_fs/tree.h
===================================================================
--- subversion/libsvn_fs_fs/tree.h (revision 1185907)
+++ subversion/libsvn_fs_fs/tree.h (working copy)
@@ -61,6 +61,13 @@ svn_fs_fs__check_path(svn_node_kind_t *kind_p,
const char *path,
apr_pool_t *pool);
+/* Implement root_vtable_t.node_id(). */
+svn_error_t *
+svn_fs_fs__node_id(const svn_fs_id_t **id_p,
+ svn_fs_root_t *root,
+ const char *path,
+ apr_pool_t *pool);
+
/* Set *REVISION to the revision in which PATH under ROOT was created.
Use POOL for any temporary allocations. If PATH is in an
uncommitted transaction, *REVISION will be set to
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1185907)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -5839,15 +5839,37 @@ write_hash_rep(svn_filesize_t *size,
/* Sanity check ROOT_NODEREV, a candidate for being the root node-revision
of (not yet committed) revision REV. Use OCEAN for temporary allocations.
*/
-static APR_INLINE svn_error_t *
-validate_root_noderev(node_revision_t *root_noderev,
- svn_revnum_t rev)
+static svn_error_t *
+validate_root_noderev(svn_fs_t *fs,
+ node_revision_t *root_noderev,
+ svn_revnum_t rev,
+ apr_pool_t *pool)
{
+ svn_revnum_t head_revnum = rev-1;
+ int head_predecessor_count;
+
+ SVN_ERR_ASSERT(rev > 0);
+
+ {
+ svn_fs_root_t *head_revision;
+ const svn_fs_id_t *head_root_id;
+ node_revision_t *head_root_noderev;
+
+ /* Get /@HEAD's noderev. */
+ SVN_ERR(svn_fs_fs__revision_root(&head_revision, fs, head_revnum, pool));
+ SVN_ERR(svn_fs_fs__node_id(&head_root_id, head_revision, "/", pool));
+ SVN_ERR(svn_fs_fs__get_node_revision(&head_root_noderev, fs, head_root_id,
+ pool));
+
+ head_predecessor_count = head_root_noderev->predecessor_count;
+ }
+
/* Bogosity seen on svn.apache.org; see
http://mid.gmane.org/20111002202833.GA12373@daniel3.local
*/
if (root_noderev->predecessor_count != -1
- && root_noderev->predecessor_count != rev)
+ && (root_noderev->predecessor_count - head_predecessor_count)
+ != (rev - head_revnum))
{
return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
_("predecessor count for "
@@ -6028,7 +6050,7 @@ write_final_rev(const svn_fs_id_t **new_id_p,
/* Write out our new node-revision. */
if (at_root)
- SVN_ERR(validate_root_noderev(noderev, rev));
+ SVN_ERR(validate_root_noderev(fs, noderev, rev, pool));
SVN_ERR(svn_fs_fs__write_noderev(svn_stream_from_aprfile2(file, TRUE, pool),
noderev, ffd->format,
svn_fs_fs__fs_supports_mergeinfo(fs),
]]]
Received on 2011-10-19 15:48:11 CEST