subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From stef...@apache.org
Subject svn commit: r1665894 - in /subversion/trunk/subversion: libsvn_fs_fs/id.c libsvn_fs_fs/tree.c libsvn_fs_x/fs_id.c libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Date Wed, 11 Mar 2015 15:03:18 GMT
Author: stefan2
Date: Wed Mar 11 15:03:17 2015
New Revision: 1665894

URL: http://svn.apache.org/r1665894
Log:
Fix the noderev relatedness check for FSFS and FSX when both noderevs / IDs
belong to different transactions.  Provide a test case for it.

Due to a misread of the 1.8.x logic, the new code in 1.9 reported noderevs
from different txns as "unrelated".  The actual problem that exists in FSFS
is that node-IDs that just got created (as added w/o history) in a txn have
only a txn-local ID.  Hence, they may clash between txns.  OTOH, uncommitted
new nodes from different txns cannot be related.  So, the relation check
can be implemented for all possible cases in FSFS but requires extra logic.

BDB did the right thing from the start.  FSX had code added to mimic FSFS'
restriction and that code can simply be removed.

Found by: julianfoad

* subversion/libsvn_fs_fs/id.c
  (svn_fs_fs__id_check_related): Use a more obvious check for the
                                 "same tmp node-ID but different txn" case.

* subversion/libsvn_fs_fs/tree.c
  (fs_node_relation): Re-implement the logic for nodes from different txns.

* subversion/libsvn_fs_x/fs_id.c
  (id_compare): Remove the "different txns implies unrelated nodes" block.

* subversion/libsvn_fs_x/tree.c
  (x_node_relation): Same.

* subversion/tests/libsvn_fs/fs-test.c
  (check_txn_related): New test, inspired by check_txn_related.
  (test_funcs): Register the new test.

Modified:
    subversion/trunk/subversion/libsvn_fs_fs/id.c
    subversion/trunk/subversion/libsvn_fs_fs/tree.c
    subversion/trunk/subversion/libsvn_fs_x/fs_id.c
    subversion/trunk/subversion/libsvn_fs_x/tree.c
    subversion/trunk/subversion/tests/libsvn_fs/fs-test.c

Modified: subversion/trunk/subversion/libsvn_fs_fs/id.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/id.c?rev=1665894&r1=1665893&r2=1665894&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/id.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/id.c Wed Mar 11 15:03:17 2015
@@ -365,14 +365,21 @@ svn_fs_fs__id_check_related(const svn_fs
   if (a == b)
     return TRUE;
 
-  /* If both node_ids start with _ and they have differing transaction
-     IDs, then it is impossible for them to be related. */
-  if (id_a->private_id.node_id.revision == SVN_INVALID_REVNUM)
+  /* If both node_ids have been created within _different_ transactions
+     (and are still uncommitted), then it is impossible for them to be
+     related.
+
+     Due to our txn-local temporary IDs, however, they might have been
+     given the same temporary node ID.  We need to detect that case.
+   */
+  if (   id_a->private_id.node_id.revision == SVN_INVALID_REVNUM
+      && id_b->private_id.node_id.revision == SVN_INVALID_REVNUM)
     {
-      if (   !svn_fs_fs__id_part_eq(&id_a->private_id.txn_id,
-                                    &id_b->private_id.txn_id)
-          || !svn_fs_fs__id_txn_used(&id_a->private_id.txn_id))
+      if (!svn_fs_fs__id_part_eq(&id_a->private_id.txn_id,
+                                 &id_b->private_id.txn_id))
         return FALSE;
+
+      /* At this point, matching node_ids implies relatedness. */
     }
 
   return svn_fs_fs__id_part_eq(&id_a->private_id.node_id,

Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1665894&r1=1665893&r2=1665894&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Wed Mar 11 15:03:17 2015
@@ -1328,8 +1328,8 @@ fs_node_relation(svn_fs_node_relation_t
                  apr_pool_t *pool)
 {
   dag_node_t *node;
-  const svn_fs_id_t *id;
-  svn_fs_fs__id_part_t rev_item_a, rev_item_b, node_id_a, node_id_b;
+  const svn_fs_id_t *id_a, *id_b;
+  svn_fs_fs__id_part_t node_id_a, node_id_b;
 
   /* Root paths are a common special case. */
   svn_boolean_t a_is_root_dir
@@ -1337,6 +1337,11 @@ fs_node_relation(svn_fs_node_relation_t
   svn_boolean_t b_is_root_dir
     = (path_b[0] == '\0') || ((path_b[0] == '/') && (path_b[1] == '\0'));
 
+  /* Another useful thing to know: Both are txns but not the same txn. */
+  svn_boolean_t different_txn
+    = root_a->is_txn_root && root_b->is_txn_root
+        && strcmp(root_a->txn, root_b->txn);
+
   /* Path from different repository are always unrelated. */
   if (root_a->fs != root_b->fs)
     {
@@ -1344,19 +1349,11 @@ fs_node_relation(svn_fs_node_relation_t
       return SVN_NO_ERROR;
     }
 
-  /* Nodes from different transactions are never related. */
-  if (root_a->is_txn_root && root_b->is_txn_root
-      && strcmp(root_a->txn, root_b->txn))
-    {
-      *relation = svn_fs_node_unrelated;
-      return SVN_NO_ERROR;
-    }
-
   /* Are both (!) root paths? Then, they are related and we only test how
    * direct the relation is. */
   if (a_is_root_dir && b_is_root_dir)
     {
-      *relation = root_a->rev == root_b->rev
+      *relation = ((root_a->rev == root_b->rev) && !different_txn)
                 ? svn_fs_node_same
                 : svn_fs_node_common_ancestor;
       return SVN_NO_ERROR;
@@ -1365,21 +1362,35 @@ fs_node_relation(svn_fs_node_relation_t
   /* We checked for all separations between ID spaces (repos, txn).
    * Now, we can simply test for the ID values themselves. */
   SVN_ERR(get_dag(&node, root_a, path_a, pool));
-  id = svn_fs_fs__dag_get_id(node);
-  rev_item_a = *svn_fs_fs__id_rev_item(id);
-  node_id_a = *svn_fs_fs__id_node_id(id);
+  id_a = svn_fs_fs__dag_get_id(node);
+  node_id_a = *svn_fs_fs__id_node_id(id_a);
 
   SVN_ERR(get_dag(&node, root_b, path_b, pool));
-  id = svn_fs_fs__dag_get_id(node);
-  rev_item_b = *svn_fs_fs__id_rev_item(id);
-  node_id_b = *svn_fs_fs__id_node_id(id);
+  id_b = svn_fs_fs__dag_get_id(node);
+  node_id_b = *svn_fs_fs__id_node_id(id_b);
+
+  /* Noderevs from different nodes are unrelated. */
+  if (!svn_fs_fs__id_part_eq(&node_id_a, &node_id_b))
+    {
+      *relation = svn_fs_node_unrelated;
+      return SVN_NO_ERROR;
+    }
 
-  if (svn_fs_fs__id_part_eq(&rev_item_a, &rev_item_b))
+  /* Noderevs have the same node-ID now. So, they *seem* to be related.
+   *
+   * Special case: Different txns may create the same (txn-local) node ID.
+   * Only when they are committed can they actually be related to others. */
+  if (different_txn && node_id_a.revision == SVN_INVALID_REVNUM)
+    {
+      *relation = svn_fs_node_unrelated;
+      return SVN_NO_ERROR;
+    }
+
+  /* The noderevs are actually related.  Are they the same? */
+  if (svn_fs_fs__id_eq(id_a, id_b))
     *relation = svn_fs_node_same;
-  else if (svn_fs_fs__id_part_eq(&node_id_a, &node_id_b))
-    *relation = svn_fs_node_common_ancestor;
   else
-    *relation = svn_fs_node_unrelated;
+    *relation = svn_fs_node_common_ancestor;
 
   return SVN_NO_ERROR;
 }

Modified: subversion/trunk/subversion/libsvn_fs_x/fs_id.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/fs_id.c?rev=1665894&r1=1665893&r2=1665894&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/fs_id.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/fs_id.c Wed Mar 11 15:03:17 2015
@@ -219,12 +219,6 @@ id_compare(const svn_fs_id_t *a,
   if (svn_fs_x__id_eq(&id_a->noderev_id, &id_b->noderev_id))
     return svn_fs_node_same;
 
-  /* Items from different txns are unrelated. */
-  if (   svn_fs_x__is_txn(id_a->noderev_id.change_set)
-      && svn_fs_x__is_txn(id_b->noderev_id.change_set)
-      && id_a->noderev_id.change_set != id_b->noderev_id.change_set)
-    return svn_fs_node_unrelated;
-
   /* Fetch the nodesrevs, compare the IDs of the nodes they belong to and
      clean up any temporaries.  If we can't find one of the noderevs, don't
      get access to the FS etc., report the IDs as "unrelated" as only

Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/tree.c?rev=1665894&r1=1665893&r2=1665894&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/tree.c Wed Mar 11 15:03:17 2015
@@ -1342,19 +1342,15 @@ x_node_relation(svn_fs_node_relation_t *
       return SVN_NO_ERROR;
     }
 
-  /* Nodes from different transactions are never related. */
-  if (root_a->is_txn_root && root_b->is_txn_root
-      && strcmp(root_a->txn, root_b->txn))
-    {
-      *relation = svn_fs_node_unrelated;
-      return SVN_NO_ERROR;
-    }
-
   /* Are both (!) root paths? Then, they are related and we only test how
    * direct the relation is. */
   if (a_is_root_dir && b_is_root_dir)
     {
-      *relation = root_a->rev == root_b->rev
+      svn_boolean_t different_txn
+        = root_a->is_txn_root && root_b->is_txn_root
+            && strcmp(root_a->txn, root_b->txn);
+
+      *relation = ((root_a->rev == root_b->rev) && !different_txn)
                 ? svn_fs_node_same
                 : svn_fs_node_common_ancestor;
       return SVN_NO_ERROR;
@@ -1370,6 +1366,8 @@ x_node_relation(svn_fs_node_relation_t *
   noderev_id_b = *svn_fs_x__dag_get_id(node);
   SVN_ERR(svn_fs_x__dag_get_node_id(&node_id_b, node));
 
+  /* In FSX, even in-txn IDs are globally unique.
+   * So, we can simply compare them. */
   if (svn_fs_x__id_eq(&noderev_id_a, &noderev_id_b))
     *relation = svn_fs_node_same;
   else if (svn_fs_x__id_eq(&node_id_a, &node_id_b))

Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1665894&r1=1665893&r2=1665894&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Wed Mar 11 15:03:17 2015
@@ -4339,6 +4339,195 @@ check_related(const svn_test_opts_t *opt
 
 
 static svn_error_t *
+check_txn_related(const svn_test_opts_t *opts,
+                  apr_pool_t *pool)
+{
+  apr_pool_t *subpool = svn_pool_create(pool);
+  svn_fs_t *fs;
+  svn_fs_txn_t *txn[3];
+  svn_fs_root_t *root[3];
+  svn_revnum_t youngest_rev = 0;
+
+  /* Create a filesystem and repository. */
+  SVN_ERR(svn_test__create_fs(&fs, "test-repo-check-related",
+                              opts, pool));
+
+  /*** Step I: Build up some state in our repository through a series
+       of commits */
+
+  /* This is the node graph we are testing.  It contains one revision (r1)
+     and two transactions, T1 and T2 - yet uncommitted.
+
+     A is a file that exists in r1 (A-0) and gets modified in both txns.
+     C is a copy of A1 made in both txns.
+     B is a new node created in both txns
+     D is a file that exists in r1 (D-0) and never gets modified.
+
+                 +--A-0--+                  D-0
+                 |       |
+           +-----+       +-----+
+           |     |       |     |
+     B-1   C-T   A-1     A-2   C-1   B-2
+  */
+  /* Revision 1 */
+  SVN_ERR(svn_fs_begin_txn(&txn[0], fs, youngest_rev, subpool));
+  SVN_ERR(svn_fs_txn_root(&root[0], txn[0], subpool));
+  SVN_ERR(svn_fs_make_file(root[0], "A", subpool));
+  SVN_ERR(svn_test__set_file_contents(root[0], "A", "1", subpool));
+  SVN_ERR(svn_fs_make_file(root[0], "D", subpool));
+  SVN_ERR(svn_fs_commit_txn(NULL, &youngest_rev, txn[0], subpool));
+  SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(youngest_rev));
+  svn_pool_clear(subpool);
+  SVN_ERR(svn_fs_revision_root(&root[0], fs, youngest_rev, pool));
+
+  /* Transaction 1 */
+  SVN_ERR(svn_fs_begin_txn(&txn[1], fs, youngest_rev, pool));
+  SVN_ERR(svn_fs_txn_root(&root[1], txn[1], pool));
+  SVN_ERR(svn_test__set_file_contents(root[1], "A", "2", pool));
+  SVN_ERR(svn_fs_copy(root[0], "A", root[1], "C", pool));
+  SVN_ERR(svn_fs_make_file(root[1], "B", pool));
+
+  /* Transaction 2 */
+  SVN_ERR(svn_fs_begin_txn(&txn[2], fs, youngest_rev, pool));
+  SVN_ERR(svn_fs_txn_root(&root[2], txn[2], pool));
+  SVN_ERR(svn_test__set_file_contents(root[2], "A", "2", pool));
+  SVN_ERR(svn_fs_copy(root[0], "A", root[2], "C", pool));
+  SVN_ERR(svn_fs_make_file(root[2], "B", pool));
+
+  /*** Step II: Exhaustively verify relationship between all nodes in
+       existence. */
+  {
+    int i, j;
+
+    struct path_rev_t
+    {
+      const char *path;
+      int root;
+    };
+
+    /* Our 16 existing files/revisions. */
+    struct path_rev_t path_revs[8] = {
+      { "A", 0 }, { "A", 1 }, { "A", 2 },
+      { "B", 1 }, { "B", 2 },
+      { "C", 1 }, { "C", 2 },
+      { "D", 0 }
+    };
+
+    int related_matrix[8][8] = {
+      /* A-0 ... D-0 across the top here*/
+      { 1, 1, 1, 0, 0, 1, 1, 0 }, /* A-0 */
+      { 1, 1, 1, 0, 0, 1, 1, 0 }, /* A-1 */
+      { 1, 1, 1, 0, 0, 1, 1, 0 }, /* A-2 */
+      { 0, 0, 0, 1, 0, 0, 0, 0 }, /* C-1 */
+      { 0, 0, 0, 0, 1, 0, 0, 0 }, /* C-2 */
+      { 1, 1, 1, 0, 0, 1, 1, 0 }, /* B-1 */
+      { 1, 1, 1, 0, 0, 1, 1, 0 }, /* B-2 */
+      { 0, 0, 0, 0, 0, 0, 0, 1 }  /* D-0 */
+    };
+
+    /* Here's the fun part.  Running the tests. */
+    for (i = 0; i < 8; i++)
+      {
+        for (j = 0; j < 8; j++)
+          {
+            struct path_rev_t pr1 = path_revs[i];
+            struct path_rev_t pr2 = path_revs[j];
+            const svn_fs_id_t *id1, *id2;
+            int related = 0;
+            svn_fs_node_relation_t relation;
+
+            svn_pool_clear(subpool);
+
+            /* Get the ID for the first path/revision combination. */
+            SVN_ERR(svn_fs_node_id(&id1, root[pr1.root], pr1.path, subpool));
+
+            /* Get the ID for the second path/revision combination. */
+            SVN_ERR(svn_fs_node_id(&id2, root[pr2.root], pr2.path, subpool));
+
+            /* <exciting> Now, run the relationship check! </exciting> */
+            related = svn_fs_check_related(id1, id2) ? 1 : 0;
+            if (related == related_matrix[i][j])
+              {
+                /* xlnt! */
+              }
+            else if ((! related) && related_matrix[i][j])
+              {
+                return svn_error_createf
+                  (SVN_ERR_TEST_FAILED, NULL,
+                   "expected '%s-%d' to be related to '%s-%d'; it was not",
+                   pr1.path, pr1.root, pr2.path, pr2.root);
+              }
+            else if (related && (! related_matrix[i][j]))
+              {
+                return svn_error_createf
+                  (SVN_ERR_TEST_FAILED, NULL,
+                   "expected '%s-%d' to not be related to '%s-%d'; it was",
+                   pr1.path, pr1.root, pr2.path, pr2.root);
+              }
+
+            /* Asking directly, i.e. without involving the noderev IDs as
+             * an intermediate, should yield the same results. */
+            SVN_ERR(svn_fs_node_relation(&relation, root[pr1.root], pr1.path,
+                                         root[pr2.root], pr2.path, subpool));
+            if (i == j)
+              {
+                /* Identical note. */
+                if (!related || relation != svn_fs_node_same)
+                  {
+                    return svn_error_createf
+                      (SVN_ERR_TEST_FAILED, NULL,
+                      "expected '%s-%d' to be the same as '%s-%d';"
+                      " it was not",
+                      pr1.path, pr1.root, pr2.path, pr2.root);
+                  }
+              }
+            else if (related && relation != svn_fs_node_common_ancestor)
+              {
+                return svn_error_createf
+                  (SVN_ERR_TEST_FAILED, NULL,
+                   "expected '%s-%d' to have a common ancestor with '%s-%d';"
+                   " it had not",
+                   pr1.path, pr1.root, pr2.path, pr2.root);
+              }
+            else if (!related && relation != svn_fs_node_unrelated)
+              {
+                return svn_error_createf
+                  (SVN_ERR_TEST_FAILED, NULL,
+                   "expected '%s-%d' to not be related to '%s-%d'; it was",
+                   pr1.path, pr1.root, pr2.path, pr2.root);
+              }
+          } /* for ... */
+      } /* for ... */
+
+    /* Verify that the noderevs stay the same after their last change.
+       There is only D that is not changed. */
+    for (i = 1; i <= 2; ++i)
+      {
+        svn_fs_node_relation_t relation;
+        svn_pool_clear(subpool);
+
+        /* Query their noderev relationship to the latest change. */
+        SVN_ERR(svn_fs_node_relation(&relation, root[i], "D",
+                                     root[0], "D", subpool));
+
+        /* They shall use the same noderevs */
+        if (relation != svn_fs_node_same)
+          {
+            return svn_error_createf
+              (SVN_ERR_TEST_FAILED, NULL,
+              "expected 'D-%d' to be the same as 'D-0'; it was not", i);
+          }
+      } /* for ... */
+  }
+
+  /* Destroy the subpool. */
+  svn_pool_destroy(subpool);
+
+  return SVN_NO_ERROR;
+}
+
+
+static svn_error_t *
 branch_test(const svn_test_opts_t *opts,
             apr_pool_t *pool)
 {
@@ -6915,6 +7104,8 @@ static struct svn_test_descriptor_t test
                        "test property and text rep-sharing collision"),
     SVN_TEST_OPTS_PASS(test_internal_txn_props,
                        "test setting and getting internal txn props"),
+    SVN_TEST_OPTS_PASS(check_txn_related,
+                       "test svn_fs_check_related for transactions"),
     SVN_TEST_NULL
   };
 



Mime
View raw message