On Wed, Mar 11, 2015 at 6:04 PM, Julian Foad <julianfoad_at_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.
Received on 2015-03-16 19:33:42 CET