You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Reser <be...@reser.org> on 2014/02/28 04:40:19 UTC
Re: svn commit: r1572363 - in /subversion/trunk/subversion: include/
libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ libsvn_fs_x/ libsvn_repos/
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, stefan2@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.
Re: svn commit: r1572363 - in /subversion/trunk/subversion: include/
libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ libsvn_fs_x/ libsvn_repos/
Posted by Ben Reser <be...@reser.org>.
On 2/28/14, 1:07 PM, Stefan Fuhrmann wrote:
> 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.
Ouch.
> r1573071 documents the problem in the API. Should we
> write an API errata?
Yes I think so. I don't expect anyone to have expected that old behavior.
> This is incorrect. NULL prop list reps are equal to
> empty prop lists.
Ahh that makes sense. Part of the reason I didn't commit it is what I expected
to find in the code so widely divergent from the way the code was that I wasn't
sure about what I'd done.
> r1573071 fixes the a!=a problem for in-txn items but that's
> the best we can do here without a content comparison.
Okay thanks!
Re: svn commit: r1572363 - in /subversion/trunk/subversion: include/
libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ libsvn_fs_x/ libsvn_repos/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Feb 28, 2014 at 4:40 AM, Ben Reser <be...@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, stefan2@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.