On Fri, Feb 28, 2014 at 1:10 PM, Julian Foad <julianfoad_at_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.
Received on 2014-03-07 13:47:22 CET