> Author: stefan2
> Date: Thu Jan 2 13:16:43 2014
> New Revision: 1554800
>
> URL: http://svn.apache.org/r1554800
Hi Stefan. I just noticed a few things in this commit...
> Log:
> Provide a path-based counterpart to svn_fs_base__id_compare.
> Most code can now compare nodes directly (next commit) using
> fewer LOCs and being faster for non-BDB repositories.
>
> It also introduces improvements over the id-based API: An enum
> replaces the various magic return values and nodes from different
> repositories are reported as "unrelated" instead of yielding an
> undefined result.
>
> * subversion/include/svn_fs.h
> (svn_fs_node_relation_t,
> svn_fs_node_relation): Declare the new API.
>
> * subversion/libsvn_fs/fs-loader.h
> (root_vtable_t): Add the corresponding vtable entry.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_node_relation): Implement as forwarding to the FS vtable.
>
> * subversion/libsvn_fs_base/tree.c
> (base_node_relation): Naive implementation of the new API;
> basically what the users did in the past.
> (root_vtable): Update.
>
> * subversion/libsvn_fs_fs/tree.c
> (fs_node_relation): Optimized code based on fs_node_id() and
> svn_fs_fs__id_compare() using as few object
> copies as possible. Since all logic is in
> one place now, it will be easier to refine
> in the future.
> (root_vtable): Update.
>
> * subversion/libsvn_fs_x/tree.c
> (x_node_relation): Implement similarly to FSFS.
> (root_vtable): Update.
> Modified: subversion/trunk/subversion/include/svn_fs.h
> ==============================================================================
>
> +/** Defines the possible ways two arbitrary nodes may be related.
Would it be good to reference the old Boolean function svn_fs_check_related(id1, id2) here, saying how the three possible results of this function relate to that one? And/or maybe that function should reference this one.
> + *
> + * @since New in 1.9.
> + */
> +typedef enum svn_fs_node_relation_t
> +{
> + /** The nodes are not related.
> + * Nodes from different repositories are always unrelated. */
> + svn_fs_node_unrelated = 0,
> +
> + /** They are the same physical node, i.e. there is no intermittent change.
> + * However, due to lazy copying, they may be intermittent parent copies.
A better word would be "intervening"; "intermittent" usually means "keeps on stopping and starting" or "occurring from time to time" ("mit" referring to "sending" rather than "middle").
Also s/they may/there may/ ?
> + */
> + svn_fs_node_same,
> +
> + /** The nodes have a common ancestor (which may be one of these nodes)
> + * but are not the same.
> + */
> + svn_fs_node_common_anchestor
"ancestor"
> +
> +} svn_fs_node_relation_t;
So,
svn_fs_compare_ids()
id_vtable_t.compare
svn_fs_base__id_compare()
svn_fs_fs__id_compare()
etc.
all return results that are analogous to svn_fs_node_relation_t, but using numeric codes instead. Maybe it would be
good to update those to use your new constants. There seem to be very few uses of each of them.
> +/** Determine how @a path_a under @a root_a and @a path_b under @a root_b
> + * are related and return the result in @a relation. There is no restriction
> + * concerning the roots: They may refer to different repositories, be in
> + * arbitrary revision order and any of them may pertain to a transaction.
> + * @a pool is used for temporary allocations.
> + *
> + * @note The current implementation considers paths from different svn_fs_t
> + * as unrelated even if the underlying physical repository is the same.
We might as well decide that that's the promised and intended behaviour, and delete "current implementation", don't you think?
> + * @since New in 1.9.
> + */
> +svn_error_t *
> +svn_fs_node_relation(svn_fs_node_relation_t *relation,
> + svn_fs_root_t *root_a,
> + const char *path_a,
> + svn_fs_root_t *root_b,
> + const char *path_b,
> + apr_pool_t *pool);
> +
> Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.c
> ==============================================================================
>
> svn_error_t *
> +svn_fs_node_relation(svn_fs_node_relation_t *relation,
> + svn_fs_root_t *root_a, const char *path_a,
> + svn_fs_root_t *root_b, const char *path_b,
> + apr_pool_t *pool)
> +{
> + /* Different repository types? */
> + if (root_a->vtable != root_b->vtable)
That test doesn't look robust. It may well be the case at the moment that the FSAP vtable is always at the same address for roots in a given FS, but wouldn't it be better to avoid relying on that and compare the FS object address directly?
if (root_a->fs != root_b->fs)
{ return "unrelated" }
/* Now the FS's are the same, so the FSAP vtables must be
equivalent even if not allocated at the same address. */
> + {
> + *relation = svn_fs_node_unrelated;
> + return SVN_NO_ERROR;
> + }
Alternatively, but somehow not so good, we could leave the code above as it was and assert here that root_a->fs == root_b->fs.
> + return svn_error_trace(root_a->vtable->node_relation(relation, root_a,
> + path_a, root_b,
> + path_b, pool));
> +}
> Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.h
> ==============================================================================
> @@ -299,6 +299,10 @@ typedef struct root_vtable_t
> apr_pool_t *pool);
> svn_error_t *(*node_id)(const svn_fs_id_t **id_p, svn_fs_root_t *root,
> const char *path, apr_pool_t *pool);
> + svn_error_t *(*node_relation)(svn_fs_node_relation_t *relation,
> + svn_fs_root_t *root_a, const char *path_a,
> + svn_fs_root_t *root_b, const char *path_b,
> + apr_pool_t *pool);
> svn_error_t *(*node_created_rev)(svn_revnum_t *revision,
> svn_fs_root_t *root, const char *path,
> apr_pool_t *pool);
>
> Modified: subversion/trunk/subversion/libsvn_fs_base/tree.c
> ==============================================================================
>
> +static svn_error_t *
> +base_node_relation(svn_fs_node_relation_t *relation,
> + svn_fs_root_t *root_a, const char *path_a,
> + svn_fs_root_t *root_b, const char *path_b,
> + apr_pool_t *pool)
> +{
> + const svn_fs_id_t *id_a, *id_b;
> +
> + /* Paths from different repository are never related. */
> + if (root_a->fs != root_b->fs)
> + {
> + *relation = svn_fs_node_unrelated;
> + return SVN_NO_ERROR;
> + }
> +
> + /* Naive implementation. */
> + SVN_ERR(base_node_id(&id_a, root_a, path_a, pool));
> + SVN_ERR(base_node_id(&id_b, root_b, path_b, pool));
> +
> + switch (svn_fs_base__id_compare(id_a, id_b))
> + {
> + case 0: *relation = svn_fs_node_same;
> + break;
> + case 1: *relation = svn_fs_node_common_anchestor;
> + break;
> + default: *relation = svn_fs_node_unrelated;
> + break;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
>
> struct node_created_rev_args {
> svn_revnum_t revision;
> @@ -5400,6 +5432,7 @@ static root_vtable_t root_vtable = {
> base_check_path,
> base_node_history,
> base_node_id,
> + base_node_relation,
> base_node_created_rev,
> base_node_origin_rev,
> base_node_created_path,
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> ==============================================================================
>
> +static svn_error_t *
> +fs_node_relation(svn_fs_node_relation_t *relation,
> + svn_fs_root_t *root_a, const char *path_a,
> + svn_fs_root_t *root_b, const char *path_b,
> + 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;
> +
> + /* Root paths are a common special case. */
> + svn_boolean_t a_is_root_dir
> + = (path_a[0] == '\0') || ((path_a[0] == '/') &&
> (path_a[1] == '\0'));
> + svn_boolean_t b_is_root_dir
> + = (path_b[0] == '\0') || ((path_b[0] == '/') &&
> (path_b[1] == '\0'));
> +
> + /* Root paths are never related to non-root paths and path from different
> + * repository are always unrelated. */
It's possible to copy the root (svn cp ^/@123 ^/copy-of-old-root). Does a copy not count as "related"? I'm not sure precisely what "related" means.
> + if (a_is_root_dir ^ b_is_root_dir || root_a->fs != root_b->fs)
> + {
> + *relation = svn_fs_node_unrelated;
> + 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)
> + {
> + *relation = root_a->rev == root_b->rev
> + ? svn_fs_node_same
> + : svn_fs_node_common_anchestor;
> + return SVN_NO_ERROR;
> + }
> +
> + /* 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, FALSE, 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);
> +
> + SVN_ERR(get_dag(&node, root_b, path_b, FALSE, 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);
> +
> + if (svn_fs_fs__id_part_eq(&rev_item_a, &rev_item_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_anchestor;
> + else
> + *relation = svn_fs_node_unrelated;
> +
> + return SVN_NO_ERROR;
> +}
>
> svn_error_t *
> svn_fs_fs__node_created_rev(svn_revnum_t *revision,
> @@ -4275,6 +4338,7 @@ static root_vtable_t root_vtable = {
> svn_fs_fs__check_path,
> fs_node_history,
> svn_fs_fs__node_id,
> + fs_node_relation,
> svn_fs_fs__node_created_rev,
> fs_node_origin_rev,
> fs_node_created_path,
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
> ==============================================================================
>
> +static svn_error_t *
> +x_node_relation(svn_fs_node_relation_t *relation,
> + svn_fs_root_t *root_a, const char *path_a,
> + svn_fs_root_t *root_b, const char *path_b,
> + apr_pool_t *pool)
> +{
> + dag_node_t *node;
> + const svn_fs_id_t *id;
> + svn_fs_x__id_part_t rev_item_a, rev_item_b, node_id_a, node_id_b;
> +
> + /* Root paths are a common special case. */
> + svn_boolean_t a_is_root_dir
> + = (path_a[0] == '\0') || ((path_a[0] == '/') &&
> (path_a[1] == '\0'));
> + svn_boolean_t b_is_root_dir
> + = (path_b[0] == '\0') || ((path_b[0] == '/') &&
> (path_b[1] == '\0'));
> +
> + /* Root paths are never related to non-root paths and path from different
> + * repository are always unrelated. */
> + if (a_is_root_dir ^ b_is_root_dir || root_a->fs != root_b->fs)
> + {
> + *relation = svn_fs_node_unrelated;
> + 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)
> + {
> + *relation = root_a->rev == root_b->rev
> + ? svn_fs_node_same
> + : svn_fs_node_common_anchestor;
> + return SVN_NO_ERROR;
> + }
> +
> + /* 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, FALSE, pool));
> + id = svn_fs_x__dag_get_id(node);
> + rev_item_a = *svn_fs_x__id_rev_item(id);
> + node_id_a = *svn_fs_x__id_node_id(id);
> +
> + SVN_ERR(get_dag(&node, root_b, path_b, FALSE, pool));
> + id = svn_fs_x__dag_get_id(node);
> + rev_item_b = *svn_fs_x__id_rev_item(id);
> + node_id_b = *svn_fs_x__id_node_id(id);
> +
> + if (svn_fs_x__id_part_eq(&rev_item_a, &rev_item_b))
> + *relation = svn_fs_node_same;
> + else if (svn_fs_x__id_part_eq(&node_id_a, &node_id_b))
> + *relation = svn_fs_node_common_anchestor;
> + else
> + *relation = svn_fs_node_unrelated;
> +
> + return SVN_NO_ERROR;
> +}
>
> svn_error_t *
> svn_fs_x__node_created_rev(svn_revnum_t *revision,
> @@ -4199,6 +4262,7 @@ static root_vtable_t root_vtable = {
> svn_fs_x__check_path,
> x_node_history,
> svn_fs_x__node_id,
> + x_node_relation,
> svn_fs_x__node_created_rev,
> x_node_origin_rev,
> x_node_created_path,
>
- Julian
Received on 2014-02-28 13:10:40 CET