First off I understand what you're trying to do and appreciate that! But
you're changing API behavior in ways that don't make sense to me.
On 2/26/14, 4:15 PM, stef...@apache.org wrote:
> + * @note The behavior under @a strict == #FALSE is implementation dependent
> + * in that the false positives reported may differ from release to release
> + * and backend to backend. It is perfectly legal to report all combinations
> + * as "changed" even for @a path1 == @a path2 and @a root1 == @a root2.
> + * There is also no guarantee that there will be false positives at all.
Surely we can always know that the same transaction root and same path are the
same. However, with FSFS the swig-rb tests are failing right now because of
just this behavior. Specifically this line:
assert(!txn1.root.props_changed?(path_in_repos, txn1.root, path_in_repos))
Of course you can make changes like this on new APIs. But you can't do this
when the the rules you've defined for FALSE differ from the rules in previous
versions of Subversion so greatly:
> +/** Similar to svn_fs_props_changed2 with @a strict set to #FALSE.
> + *
> + * @deprecated Provided for backward compatibility with the 1.8 API.
> */
> +SVN_DEPRECATED
> svn_error_t *
> svn_fs_props_changed(svn_boolean_t *changed_p,
> svn_fs_root_t *root1,
This would at least compare the uniquifiers in 1.8.x. Now in the FALSE case
(looking at the code you committed in r1572336) we only compare if the pointers
for the representations are the same for properties in transactions (see
svn_fs_fs__prop_rep_equal). Since the svn_fs_props_changed() API is going to
call get_dag() for both nodes I believe that this API will always return true
for properties in transactions.
This isn't a problem in the file case because you've upgraded it to at least
compare MD5s. But properties don't have MD5's to compare.
This patch fixes the ruby tests for me:
[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1572761)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -1131,9 +1131,15 @@ svn_fs_fs__prop_rep_equal(svn_boolean_t *equal,
return SVN_NO_ERROR;
}
+ /* Can't be the same if only one of them have a property representation */
+ if (rep_a == NULL || rep_b == NULL)
+ {
+ *equal = FALSE;
+ return SVN_NO_ERROR;
+ }
+
/* Committed property lists can be compared quickly */
- if ( rep_a && rep_b
- && !svn_fs_fs__id_txn_used(&rep_a->txn_id)
+ if ( !svn_fs_fs__id_txn_used(&rep_a->txn_id)
&& !svn_fs_fs__id_txn_used(&rep_b->txn_id))
{
/* MD5 must be given. Having the same checksum is good enough for
@@ -1144,10 +1150,11 @@ svn_fs_fs__prop_rep_equal(svn_boolean_t *equal,
}
/* Skip the expensive bits unless we are in strict mode.
- Simply assume that there is a different. */
+ At least check if the uniquifier is the same. */
if (!strict)
{
- *equal = FALSE;
+ *equal = memcmp(&rep_a->uniquifier, &rep_b->uniquifier,
+ sizeof(rep_a->uniquifier)) == 0;
return SVN_NO_ERROR;
}
]]]
First of all I return FALSE if either of the nodes is missing a property
representation while the other one has one, I don't see a point in doing a
direct comparison in the case of strict if this is true, it's obvious they are
different.
Further, if we're not in strict mode I compare the uniquifier which avoids us
always returning FALSE from svn_fs_fs__prop_rep_equal for transactions. But
I'm not sure if this is really correct, since the old code compared item_index
and revision first.
