You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2014/02/28 13:10:02 UTC
Re: svn commit: r1554800 - in /subversion/trunk/subversion: include/svn_fs.h libsvn_fs/fs-loader.c libsvn_fs/fs-loader.h libsvn_fs_base/tree.c libsvn_fs_fs/tree.c libsvn_fs_x/tree.c
> 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
Re: svn commit: r1554800 - in /subversion/trunk/subversion:
include/svn_fs.h libsvn_fs/fs-loader.c libsvn_fs/fs-loader.h
libsvn_fs_base/tree.c libsvn_fs_fs/tree.c libsvn_fs_x/tree.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Mar 7, 2014 at 1:46 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
> On Fri, Feb 28, 2014 at 1:10 PM, Julian Foad <ju...@btopenworld.com>wrote:
>
>> > 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...
>>
>
> > 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.
>>
>
> You are right, this is a bug. I've got confused by
> svn_fs_fs__node_id treating the root as a special
> case. And I've operated on the assumption that
> node_id 0 would always imply "root" - which is
> also false. Going to fix that now.
>
r1575320 handles the "nodeID 0 may be copied
as well" issue - basically simplifying the code.
r1575323 fixes the only place where I assumed
that nodeID==0 was equivalent to being root.
>
> Thanks for the review!
>
-- Stefan^2.
>
Re: svn commit: r1554800 - in /subversion/trunk/subversion:
include/svn_fs.h libsvn_fs/fs-loader.c libsvn_fs/fs-loader.h
libsvn_fs_base/tree.c libsvn_fs_fs/tree.c libsvn_fs_x/tree.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Feb 28, 2014 at 1:10 PM, Julian Foad <ju...@btopenworld.com>wrote:
> > 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...
>
> > 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.
>
Done in r1574038 and r1575245.
> + *
> > + * @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").
>
There goes one of my new pet words :(
> Also s/they may/there may/ ?
>
Both fixed in r1574038.
> > + */
> > + 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"
>
Also fixed in r1574038.
> +
> > +} 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.
>
That's r1574144 and -65 now.
> +/** 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?
>
I agree. r1574038 again.
> + * @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.
>
Now that we declared the current behavior as the fully
intended one, we can simply test for FS equality.
Done in r1574056.
> 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.
>
You are right, this is a bug. I've got confused by
svn_fs_fs__node_id treating the root as a special
case. And I've operated on the assumption that
node_id 0 would always imply "root" - which is
also false. Going to fix that now.
Thanks for the review!
-- Stefan^2.