On Wed, Mar 11, 2015 at 6:04 PM, Julian Foad <julianf...@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.
