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
Received on 2015-03-11 18:11:54 CET