On Fri, Feb 28, 2014 at 4:40 AM, Ben Reser <b...@reser.org> wrote:
> 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.
>
I don't intend to make the functionality "worse".
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:
>
Things are much worse, in fact. The old implementation
would consider all property lists to be equal (FSFS only)
because the representation_t structs are empty except
for the txn_id. Hence, revision, offset and unifier would
always match between any modified prop lists.
r1573071 documents the problem in the API. Should we
write an API errata?
> > +/** 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.
>
Does not work for in-txn prop lists as their reps structs
are almost completely 0.
> 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;
> + }
> +
>
This is incorrect. NULL prop list reps are equal to
empty prop lists.
/* 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.
>
r1573071 fixes the a!=a problem for in-txn items but that's
the best we can do here without a content comparison.
-- Stefan^2.
