On Fri, Feb 28, 2014 at 1:10 PM, Julian Foad <julianf...@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.
