You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2015/03/11 16:03:18 UTC
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
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
};
Re: 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
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Mar 17, 2015 at 10:44 AM, Julian Foad <ju...@btopenworld.com>
wrote:
> Stefan Fuhrmann wrote:
> > Thanks for the review, Julian! Uncovered a bug and
> > fixed all the things you found:
>
> Thanks.
>
> I've proposed r1665894 and r1667101 for backport to 1.9.x.
>
> I'm not quite sure of the severity of the bug, or what real-world
> problem it may have caused, if any, so I haven't written a
> 'justification' in the proposal.
>
Np. Added a justification in r1667508.
-- Stefan^2.
Re: 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
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> Thanks for the review, Julian! Uncovered a bug and
> fixed all the things you found:
Thanks.
I've proposed r1665894 and r1667101 for backport to 1.9.x.
I'm not quite sure of the severity of the bug, or what real-world
problem it may have caused, if any, so I haven't written a
'justification' in the proposal.
> Julian Foad wrote:
>> That says: the root node-rev of rX is never the same as that of rY when X
>> != Y.
>>
>> Can I just double-check: is that guaranteed to be true for every FSFS
>> repository? It's possible to commit an empty revision. The
>> "svn_fs_fs__create_txn" function ensures the new txn always has a new
>> root node-rev, but is it in any way possible that a repository could
>> exist where an (empty) commit has bypassed that code path and ended up
>> with the root noderev of r11 the same as of r10?
>
> That would not be a valid FSFS repository. Format 6 revisions and
> earlier end with a revision trailer that points to the root noderev and
> change list in that revision. Format 7 implies them as well albeit
> less explicitly. We check them explicitly in validate_root_noderev()
> and svn_fs_fs__verify_root().
Thanks for confirming that.
>> It's probably fine, but it was a bit surprising to me to find this
>> requirement made explicit. Is that consistent with the rest of FSFS?
>
> There are certainly places that assume that the root node is always
> available but I don't remember anything specific outside the consistency
> checks.
>
> If we were to allow for truly empty revisions in the future, then that
> would probably be a storage-only feature and the missing noderev
> structs would be created on-the-fly in the reader / parser function.
[...]
>> Would it be clearer to write:
>>
>> * Special case: Different txns may create the same (txn-local) node ID.
>> * These are not related to each other, nor to any other node ID so far.
>
> Committed as r1667090.
Thanks.
> r1667101 fixes another problem with that code.
Looks good.
>> Can you add test cases for the root node-rev, because the code for
>> that is special-cased?
>
> Done in r1667102.
Thanks.
- Julian
Re: 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
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Mar 11, 2015 at 6:04 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> Hi Stefan.
>
> A few comments below...
>
Thanks for the review, Julian! Uncovered a bug and
fixed all the things you found:
> ----- Original Message -----
> > URL: http://svn.apache.org/r1665894
>
[...]
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Wed Mar 11 15:03:17
> 2015
>
[...]
> > @@ -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;
>
> That says: the root node-rev of rX is never the same as that of rY when X
> != Y.
>
> Can I just double-check: is that guaranteed to be true for every FSFS
> repository? It's possible to commit an empty revision. The
> "svn_fs_fs__create_txn" function ensures the new txn always has a new
> root node-rev, but is it in any way possible that a repository could
> exist where an (empty) commit has bypassed that code path and ended up
> with the root noderev of r11 the same as of r10?
>
That would not be a valid FSFS repository. Format 6 revisions and
earlier end with a revision trailer that points to the root noderev and
change list in that revision. Format 7 implies them as well albeit
less explicitly. We check them explicitly in validate_root_noderev()
and svn_fs_fs__verify_root().
It's probably fine, but it was a bit surprising to me to find this
> requirement made explicit. Is that consistent with the rest of FSFS?
>
There are certainly places that assume that the root node is always
available but I don't remember anything specific outside the consistency
checks.
If we were to allow for truly empty revisions in the future, then that
would probably be a storage-only feature and the missing noderev
structs would be created on-the-fly in the reader / parser function.
> > 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;
> > + }
>
> I found this condition a bit difficult to understand. At first sight
> it looks asymmetric, but that's because you already know the two
> node-ids are equal so you only need to examine one to know that both
> are created in their respective transactions. OK.
>
> The comment "Only when they are committed can they actually be related
> to others" confused me. If a txn creates a new node-id, when *this*
> txn is committed the node-id will definitely not be related to any
> others. Only *later* commits may create new node-revs that are related
> by having that same node-id.
>
> Would it be clearer to write:
>
> * Special case: Different txns may create the same (txn-local) node ID.
> * These are not related to each other, nor to any other node ID so far.
> */
>
Committed as r1667090. r1667101 fixes another problem with that code.
[...]
> > Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
> >
> ==============================================================================
> > static svn_error_t *
> > +check_txn_related(const svn_test_opts_t *opts,
> > + apr_pool_t *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 */
> [...]
> > + /* Transaction 1 */
> [...]
> > + /* Transaction 2 */
> [...]
> > +
> > + /*** Step II: Exhaustively verify relationship between all nodes in
> > + existence. */
> [...]
> > +}
>
> Can you add test cases for the root node-rev, because the code for
> that is special-cased?
>
> (It's valid to copy the root, so you can create 'E' as a copy of the
> root and test for relatedness between E and '/' in various revs/txnx.)
>
Done in r1667102.
-- Stefan^2.
Re: 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
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Stefan.
A few comments below...
----- Original Message -----
> 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/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
> ==============================================================================
> --- 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;
That says: the root node-rev of rX is never the same as that of rY when X != Y.
Can I just double-check: is that guaranteed to be true for every FSFS
repository? It's possible to commit an empty revision. The
"svn_fs_fs__create_txn" function ensures the new txn always has a new
root node-rev, but is it in any way possible that a repository could
exist where an (empty) commit has bypassed that code path and ended up
with the root noderev of r11 the same as of r10?
It's probably fine, but it was a bit surprising to me to find this
requirement made explicit. Is that consistent with the rest of FSFS?
> 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;
> + }
I found this condition a bit difficult to understand. At first sight
it looks asymmetric, but that's because you already know the two
node-ids are equal so you only need to examine one to know that both
are created in their respective transactions. OK.
The comment "Only when they are committed can they actually be related
to others" confused me. If a txn creates a new node-id, when *this*
txn is committed the node-id will definitely not be related to any
others. Only *later* commits may create new node-revs that are related
by having that same node-id.
Would it be clearer to write:
* Special case: Different txns may create the same (txn-local) node ID.
* These are not related to each other, nor to any other node ID so far. */
> +
> + /* 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/tests/libsvn_fs/fs-test.c
> ==============================================================================
> static svn_error_t *
> +check_txn_related(const svn_test_opts_t *opts,
> + apr_pool_t *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 */
[...]
> + /* Transaction 1 */
[...]
> + /* Transaction 2 */
[...]
> +
> + /*** Step II: Exhaustively verify relationship between all nodes in
> + existence. */
[...]
> +}
Can you add test cases for the root node-rev, because the code for
that is special-cased?
(It's valid to copy the root, so you can create 'E' as a copy of the
root and test for relatedness between E and '/' in various revs/txnx.)
- Julian