Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@subversion.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1572336 - in /subversion/trunk/subversion:
 include/private/
 libsvn_fs_base/ libsvn_fs_fs/ libsvn_fs_util/ libsvn_fs_x/
Date: Wed, 26 Feb 2014 23:15:25 -0000
To: commits@subversion.apache.org
From: stefan2@apache.org
Message-Id: <20140226231526.3F95D23888E2@eris.apache.org>

Author: stefan2
Date: Wed Feb 26 23:15:25 2014
New Revision: 1572336

URL: http://svn.apache.org/r1572336
Log:
Improve the accuracy of the "Have props/contents changed between two nodes?"
check for all three backends.

First, we consider props / text contents as equal whenever the same physical
representation is being used - even if the uniquifiers differ.  There is no
such test for directories and it is not needed by any API, ATM.  Secondly,
file contents is considered equal when the SHA1 checksums match (same as in
the working copy).  We also compare the MD5 checksums as they are always
available and offer a quick mismatch test.

Lastly, if the SHA1 is not available, we actually compare the reconstructed
fulltexts - but only if the caller requires a strict equality test with no
false negatives.  The FS API currently calls the internal functionality in
non-strict, i.e. "quick check" mode.

Property lists use a similar checking scheme.  However, we limit the test to
MD5 (FSFS, FSX only) and use a fulltext comparison in strict mode only if
MD5s are not available.  As a side-effect of the extra accuracy, the commit
conflict detection / merge code in non-BDB now allows for equal directory
property lists to be committed to the same node in concurrent transactions
without creating a conflict.

The BDB implementation is the most straightforward of the three backends
because the respective function are directly at the vtable level.  We simply
add checksum and fulltext comparison as needed.

In FSFS, the actual comparison lies deeper in the caller hierarchy and is
being used by other functions as well.  We need to replace the generic "is
the rep equal" test by two tests, one for each kind of representation.  They
will then perform similar tests as in BDB but with more callers to update.
The prop comparison will also use the MD5 checksums when available.

The FSX code has the same structure as FSFS but it does need to cater for
legacy data.  Therefore, the file contents check is much simpler and can
always use the MD5 + SHA1 checksums.

* subversion/include/private/svn_fs_util.h
  (svn_fs__prop_lists_equal): Declare new private contents comparison API.
* subversion/libsvn_fs_util/fs-util.c
  (svn_fs__prop_lists_equal): Implement it.

* subversion/libsvn_fs_base/dag.c
  (svn_fs_base__things_different): Ignore uniquifier since we are only
                                   interested in actual content differences.
* subversion/libsvn_fs_base/tree.c
  (things_changed_args): Add the STRICT option to our parameter set.
  (txn_body_props_changed,
   txn_body_contents_changed): Add more comparisons to eliminate false
                               positives as described above. 
  (base_props_changed,
   base_contents_changed): FS API implementation is currently non-strict.

* subversion/libsvn_fs_fs/fs_fs.h
  (svn_fs_fs__noderev_same_rep_key): Drop old test function.
  (svn_fs_fs__file_text_rep_equal,
   svn_fs_fs__prop_rep_equal): Introduce separate APIs for different types
                               of representations; provide more context
                               such that we may query missing information.
* subversion/libsvn_fs_fs/fs_fs.c
  (svn_fs_fs__noderev_same_rep_key): Drop old test function.
  (svn_fs_fs__file_text_rep_equal,
   svn_fs_fs__prop_rep_equal): Implement.

* subversion/libsvn_fs_fs/dag.h
  (svn_fs_fs__dag_things_different): Add STRICT option and provide a pool
                                     for FS queries as needed.
* subversion/libsvn_fs_fs/dag.c
  (svn_fs_fs__dag_things_different): Update implementation to call the new
                                     comparison functions.

* subversion/libsvn_fs_fs/tree.c
  (fs_props_changed,
   fs_contents_changed): Update callers and use non-strict mode for these
                         FS API functions.
  (merge): Adapt to API change and force strict prop comparison to prevent
           unnecessary conflicts.

* subversion/libsvn_fs_x/fs_x.h
  (svn_fs_x__noderev_same_rep_key): Drop old test function.
  (svn_fs_x__file_text_rep_equal): Similar to FSFS but with a simpler
                                   signature because there is no need to
                                   query for addition data. 
   svn_fs_x__prop_rep_equal): Similar to FSFS.
* subversion/libsvn_fs_x/fs_x.c
  (svn_fs_x__noderev_same_rep_key): Drop old test function.
  (svn_fs_x__file_text_rep_equal, 
   svn_fs_x__prop_rep_equal): Implement.

* subversion/libsvn_fs_x/dag.h
  (svn_fs_x__dag_things_different): Change similarly as FSFS.
* subversion/libsvn_fs_x/dag.c
  (svn_fs_x__dag_things_different): Ditto.

* subversion/libsvn_fs_x/tree.c
  (x_props_changed,
   merge, 
   x_contents_changed): Ditto.

Modified:
    subversion/trunk/subversion/include/private/svn_fs_util.h
    subversion/trunk/subversion/libsvn_fs_base/dag.c
    subversion/trunk/subversion/libsvn_fs_base/tree.c
    subversion/trunk/subversion/libsvn_fs_fs/dag.c
    subversion/trunk/subversion/libsvn_fs_fs/dag.h
    subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
    subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
    subversion/trunk/subversion/libsvn_fs_fs/tree.c
    subversion/trunk/subversion/libsvn_fs_util/fs-util.c
    subversion/trunk/subversion/libsvn_fs_x/dag.c
    subversion/trunk/subversion/libsvn_fs_x/dag.h
    subversion/trunk/subversion/libsvn_fs_x/fs_x.c
    subversion/trunk/subversion/libsvn_fs_x/fs_x.h
    subversion/trunk/subversion/libsvn_fs_x/tree.c

Modified: subversion/trunk/subversion/include/private/svn_fs_util.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_fs_util.h?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_fs_util.h (original)
+++ subversion/trunk/subversion/include/private/svn_fs_util.h Wed Feb 26 23:15:25 2014
@@ -221,6 +221,15 @@ svn_fs__compatible_version(svn_version_t
                            apr_hash_t *config,
                            apr_pool_t *pool);
 
+/* Compare the property lists A and B using POOL for temporary allocations.
+   Return true iff both lists contain the same properties with the same
+   values.  A and B may be NULL in which case they will be equal to and
+   empty list. */
+svn_boolean_t
+svn_fs__prop_lists_equal(apr_hash_t *a,
+                         apr_hash_t *b,
+                         apr_pool_t *pool);
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */

Modified: subversion/trunk/subversion/libsvn_fs_base/dag.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/dag.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/dag.c (original)
+++ subversion/trunk/subversion/libsvn_fs_base/dag.c Wed Feb 26 23:15:25 2014
@@ -1654,14 +1654,8 @@ svn_fs_base__things_different(svn_boolea
 
   /* Compare contents keys and their (optional) uniquifiers. */
   if (contents_changed != NULL)
-    *contents_changed =
-      (! (svn_fs_base__same_keys(noderev1->data_key,
-                                 noderev2->data_key)
-          /* Technically, these uniquifiers aren't used and "keys",
-             but keys are base-36 stringified numbers, so we'll take
-             this liberty. */
-          && (svn_fs_base__same_keys(noderev1->data_key_uniquifier,
-                                     noderev2->data_key_uniquifier))));
+    *contents_changed = (! svn_fs_base__same_keys(noderev1->data_key,
+                                                  noderev2->data_key));
 
   return SVN_NO_ERROR;
 }

Modified: subversion/trunk/subversion/libsvn_fs_base/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/tree.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_base/tree.c Wed Feb 26 23:15:25 2014
@@ -1402,6 +1402,7 @@ struct things_changed_args
   svn_fs_root_t *root2;
   const char *path1;
   const char *path2;
+  svn_boolean_t strict;
   apr_pool_t *pool;
 };
 
@@ -1411,11 +1412,26 @@ txn_body_props_changed(void *baton, trai
 {
   struct things_changed_args *args = baton;
   dag_node_t *node1, *node2;
+  apr_hash_t *proplist1, *proplist2;
 
   SVN_ERR(get_dag(&node1, args->root1, args->path1, trail, trail->pool));
   SVN_ERR(get_dag(&node2, args->root2, args->path2, trail, trail->pool));
-  return svn_fs_base__things_different(args->changed_p, NULL,
-                                       node1, node2, trail, trail->pool);
+  SVN_ERR(svn_fs_base__things_different(args->changed_p, NULL,
+                                        node1, node2, trail, trail->pool));
+
+  /* Is there a potential false positive and do we want to correct it? */
+  if (!args->strict || !*args->changed_p)
+    return SVN_NO_ERROR;
+
+  /* Different representations. They might still have equal contents. */
+  SVN_ERR(svn_fs_base__dag_get_proplist(&proplist1, node1,
+                                        trail, trail->pool));
+  SVN_ERR(svn_fs_base__dag_get_proplist(&proplist2, node2,
+                                        trail, trail->pool));
+
+  *args->changed_p = !svn_fs__prop_lists_equal(proplist1, proplist2,
+                                               trail->pool);
+  return SVN_NO_ERROR;
 }
 
 
@@ -1441,6 +1457,7 @@ base_props_changed(svn_boolean_t *change
   args.path2      = path2;
   args.changed_p  = changed_p;
   args.pool       = pool;
+  args.strict     = FALSE;
 
   return svn_fs_base__retry_txn(root1->fs, txn_body_props_changed, &args,
                                 TRUE, pool);
@@ -4004,11 +4021,53 @@ txn_body_contents_changed(void *baton, t
 {
   struct things_changed_args *args = baton;
   dag_node_t *node1, *node2;
+  svn_checksum_t *checksum1, *checksum2;
+  svn_stream_t *stream1, *stream2;
+  svn_boolean_t same;
 
   SVN_ERR(get_dag(&node1, args->root1, args->path1, trail, trail->pool));
   SVN_ERR(get_dag(&node2, args->root2, args->path2, trail, trail->pool));
-  return svn_fs_base__things_different(NULL, args->changed_p,
-                                       node1, node2, trail, trail->pool);
+  SVN_ERR(svn_fs_base__things_different(NULL, args->changed_p,
+                                        node1, node2, trail, trail->pool));
+
+  /* Is there a potential false positive and do we want to correct it? */
+  if (!args->strict || !*args->changed_p)
+    return SVN_NO_ERROR;
+
+  /* Different representations. They might still have equal contents. */
+
+  /* Compare MD5 checksums.  These should be readily accessible. */
+  SVN_ERR(svn_fs_base__dag_file_checksum(&checksum1, svn_checksum_md5,
+                                         node1, trail, trail->pool));
+  SVN_ERR(svn_fs_base__dag_file_checksum(&checksum2, svn_checksum_md5,
+                                         node2, trail, trail->pool));
+
+  /* Different MD5 checksums -> different contents */
+  if (!svn_checksum_match(checksum1, checksum2))
+    return SVN_NO_ERROR;
+
+  /* Paranoia. Compare SHA1 checksums because that's the level of
+     confidence we require for e.g. the working copy. */
+  SVN_ERR(svn_fs_base__dag_file_checksum(&checksum1, svn_checksum_sha1,
+                                         node1, trail, trail->pool));
+  SVN_ERR(svn_fs_base__dag_file_checksum(&checksum2, svn_checksum_sha1,
+                                         node2, trail, trail->pool));
+
+  /* Different SHA1 checksums -> different contents */
+  if (checksum1 && checksum2)
+    {
+      *args->changed_p = !svn_checksum_match(checksum1, checksum2);
+      return SVN_NO_ERROR;
+    }
+
+  /* SHA1 checksums are not available for very old reps / repositories. */
+  SVN_ERR(svn_fs_base__dag_get_contents(&stream1, node1, trail, trail->pool));
+  SVN_ERR(svn_fs_base__dag_get_contents(&stream2, node2, trail, trail->pool));
+  SVN_ERR(svn_stream_contents_same2(&same, stream1, stream2, trail->pool));
+
+  /* Now, it's definitive. */
+  *args->changed_p = !same;
+  return SVN_NO_ERROR;
 }
 
 
@@ -4051,6 +4110,7 @@ base_contents_changed(svn_boolean_t *cha
   args.path2      = path2;
   args.changed_p  = changed_p;
   args.pool       = pool;
+  args.strict     = FALSE;
 
   return svn_fs_base__retry_txn(root1->fs, txn_body_contents_changed, &args,
                                 TRUE, pool);

Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/dag.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/dag.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/dag.c Wed Feb 26 23:15:25 2014
@@ -1252,29 +1252,40 @@ svn_error_t *
 svn_fs_fs__dag_things_different(svn_boolean_t *props_changed,
                                 svn_boolean_t *contents_changed,
                                 dag_node_t *node1,
-                                dag_node_t *node2)
+                                dag_node_t *node2,
+                                svn_boolean_t strict,
+                                apr_pool_t *pool)
 {
   node_revision_t *noderev1, *noderev2;
+  svn_fs_t *fs;
+  svn_boolean_t same;
 
   /* If we have no place to store our results, don't bother doing
      anything. */
   if (! props_changed && ! contents_changed)
     return SVN_NO_ERROR;
 
+  fs = svn_fs_fs__dag_get_fs(node1);
+
   /* The node revision skels for these two nodes. */
   SVN_ERR(get_node_revision(&noderev1, node1));
   SVN_ERR(get_node_revision(&noderev2, node2));
 
   /* Compare property keys. */
   if (props_changed != NULL)
-    *props_changed = (! svn_fs_fs__noderev_same_rep_key(noderev1->prop_rep,
-                                                        noderev2->prop_rep));
+    {
+      SVN_ERR(svn_fs_fs__prop_rep_equal(&same, fs, noderev1, noderev2,
+                                        strict, pool));
+      *props_changed = !same;
+    }
 
   /* Compare contents keys. */
   if (contents_changed != NULL)
-    *contents_changed =
-      (! svn_fs_fs__noderev_same_rep_key(noderev1->data_rep,
-                                         noderev2->data_rep));
+    {
+      SVN_ERR(svn_fs_fs__file_text_rep_equal(&same, fs, noderev1, noderev2,
+                                             strict, pool));
+      *contents_changed = !same;
+    }
 
   return SVN_NO_ERROR;
 }

Modified: subversion/trunk/subversion/libsvn_fs_fs/dag.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/dag.h?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/dag.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/dag.h Wed Feb 26 23:15:25 2014
@@ -531,27 +531,25 @@ svn_error_t *svn_fs_fs__dag_copy(dag_nod
 

 /* Comparison */
 
-/* Find out what is the same between two nodes.
+/* Find out what is the same between two nodes.  If STRICT is FALSE,
+   this function may report false positives, i.e. report changes even
+   if the resulting contents / props are equal.
 
    If PROPS_CHANGED is non-null, set *PROPS_CHANGED to 1 if the two
    nodes have different property lists, or to 0 if same.
 
    If CONTENTS_CHANGED is non-null, set *CONTENTS_CHANGED to 1 if the
-   two nodes have different contents, or to 0 if same.  For files,
-   file contents are compared; for directories, the entries lists are
-   compared.  If one is a file and the other is a directory, the one's
-   contents will be compared to the other's entries list.  (Not
-   terribly useful, I suppose, but that's the caller's business.)
-
-   ### todo: This function only compares rep keys at the moment.  This
-   may leave us with a slight chance of a false positive, though I
-   don't really see how that would happen in practice.  Nevertheless,
-   it should probably be fixed.
+   two nodes have different contents, or to 0 if same.  NODE1 and NODE2
+   must refer to files from the same filesystem.
+
+   Use POOL for temporary allocations.
  */
 svn_error_t *svn_fs_fs__dag_things_different(svn_boolean_t *props_changed,
                                              svn_boolean_t *contents_changed,
                                              dag_node_t *node1,
-                                             dag_node_t *node2);
+                                             dag_node_t *node2,
+                                             svn_boolean_t strict,
+                                             apr_pool_t *pool);
 
 
 /* Set *REV and *PATH to the copyroot revision and path of node NODE, or

Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Feb 26 23:15:25 2014
@@ -1050,25 +1050,117 @@ svn_fs_fs__file_length(svn_filesize_t *l
   return SVN_NO_ERROR;
 }
 
-svn_boolean_t
-svn_fs_fs__noderev_same_rep_key(representation_t *a,
-                                representation_t *b)
-{
-  if (a == b)
-    return TRUE;
+svn_error_t *
+svn_fs_fs__file_text_rep_equal(svn_boolean_t *equal,
+                               svn_fs_t *fs,
+                               node_revision_t *a,
+                               node_revision_t *b,
+                               svn_boolean_t strict,
+                               apr_pool_t *scratch_pool)
+{
+  svn_stream_t *contents_a, *contents_b;
+  representation_t *rep_a = a->data_rep;
+  representation_t *rep_b = b->data_rep;
+  svn_boolean_t a_empty = !rep_a || rep_a->expanded_size == 0;
+  svn_boolean_t b_empty = !rep_b || rep_b->expanded_size == 0;
+
+  /* This makes sure that neither rep will be NULL later on */
+  if (a_empty && b_empty)
+    {
+      *equal = TRUE;
+      return SVN_NO_ERROR;
+    }
+
+  if (a_empty != b_empty)
+    {
+      *equal = FALSE;
+      return SVN_NO_ERROR;
+    }
+
+  /* File text representations always know their checksums - even in a txn. */
+  if (memcmp(rep_a->md5_digest, rep_b->md5_digest, sizeof(rep_a->md5_digest)))
+    {
+      *equal = FALSE;
+      return SVN_NO_ERROR;
+    }
+
+  /* Paranoia. Compare SHA1 checksums because that's the level of
+     confidence we require for e.g. the working copy. */
+  if (rep_a->has_sha1 && rep_b->has_sha1)
+    {
+      *equal = memcmp(rep_a->sha1_digest, rep_b->sha1_digest,
+                      sizeof(rep_a->sha1_digest)) == 0;
+      return SVN_NO_ERROR;
+    }
+
+  /* Old repositories may not have the SHA1 checksum handy.
+     This check becomes expensive.  Skip it unless explicitly required. */
+  if (!strict)
+    {
+      *equal = TRUE;
+      return SVN_NO_ERROR;
+    }
+
+  SVN_ERR(svn_fs_fs__get_contents(&contents_a, fs, rep_a, TRUE,
+                                  scratch_pool));
+  SVN_ERR(svn_fs_fs__get_contents(&contents_b, fs, rep_b, TRUE,
+                                  scratch_pool));
+  SVN_ERR(svn_stream_contents_same2(equal, contents_a, contents_b,
+                                   scratch_pool));
+
+  return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_fs_fs__prop_rep_equal(svn_boolean_t *equal,
+                          svn_fs_t *fs,
+                          node_revision_t *a,
+                          node_revision_t *b,
+                          svn_boolean_t strict,
+                          apr_pool_t *scratch_pool)
+{
+  representation_t *rep_a = a->prop_rep;
+  representation_t *rep_b = b->prop_rep;
+  apr_hash_t *proplist_a;
+  apr_hash_t *proplist_b;
+
+  /* Mainly for a==b==NULL */
+  if (rep_a == rep_b)
+    {
+      *equal = TRUE;
+      return SVN_NO_ERROR;
+    }
 
-  if (a == NULL || b == NULL)
-    return FALSE;
+  /* Committed property lists can be compared quickly */
+  if (   rep_a && rep_b
+      && !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
+         accepting the prop lists as equal. */
+      *equal = memcmp(rep_a->md5_digest, rep_b->md5_digest,
+                      sizeof(rep_a->md5_digest)) == 0;
+      return SVN_NO_ERROR;
+    }
 
-  if (a->item_index != b->item_index)
-    return FALSE;
+  /* Skip the expensive bits unless we are in strict mode.
+     Simply assume that there is a different. */
+  if (!strict)
+    {
+      *equal = FALSE;
+      return SVN_NO_ERROR;
+    }
 
-  if (a->revision != b->revision)
-    return FALSE;
+  /* At least one of the reps has been modified in a txn.
+     Fetch and compare them. */
+  SVN_ERR(svn_fs_fs__get_proplist(&proplist_a, fs, a, scratch_pool));
+  SVN_ERR(svn_fs_fs__get_proplist(&proplist_b, fs, b, scratch_pool));
 
-  return memcmp(&a->uniquifier, &b->uniquifier, sizeof(a->uniquifier)) == 0;
+  *equal = svn_fs__prop_lists_equal(proplist_a, proplist_b, scratch_pool);
+  return SVN_NO_ERROR;
 }
 
+
 svn_error_t *
 svn_fs_fs__file_checksum(svn_checksum_t **checksum,
                          node_revision_t *noderev,

Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h Wed Feb 26 23:15:25 2014
@@ -65,10 +65,29 @@ svn_error_t *svn_fs_fs__file_length(svn_
                                     node_revision_t *noderev,
                                     apr_pool_t *pool);
 
-/* Return TRUE if the representation keys in A and B both point to the
-   same representation, else return FALSE. */
-svn_boolean_t svn_fs_fs__noderev_same_rep_key(representation_t *a,
-                                              representation_t *b);
+/* Set *EQUAL to TRUE if the text representations in A and B within FS
+   have equal contents, else set it to FALSE.  If STRICT is not set, allow
+   for false negatives.
+   Use SCRATCH_POOL for temporary allocations. */
+svn_error_t *
+svn_fs_fs__file_text_rep_equal(svn_boolean_t *equal,
+                               svn_fs_t *fs,
+                               node_revision_t *a,
+                               node_revision_t *b,
+                               svn_boolean_t strict,
+                               apr_pool_t *scratch_pool);
+
+/* Set *EQUAL to TRUE if the property representations in A and B within FS
+   have equal contents, else set it to FALSE.  If STRICT is not set, allow
+   for false negatives.
+   Use SCRATCH_POOL for temporary allocations. */
+svn_error_t *
+svn_fs_fs__prop_rep_equal(svn_boolean_t *equal,
+                          svn_fs_t *fs,
+                          node_revision_t *a,
+                          node_revision_t *b,
+                          svn_boolean_t strict,
+                          apr_pool_t *scratch_pool);
 
 
 /* Return a copy of the representation REP allocated from POOL. */

Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Wed Feb 26 23:15:25 2014
@@ -1631,7 +1631,7 @@ fs_props_changed(svn_boolean_t *changed_
   SVN_ERR(get_dag(&node1, root1, path1, TRUE, pool));
   SVN_ERR(get_dag(&node2, root2, path2, TRUE, pool));
   return svn_fs_fs__dag_things_different(changed_p, NULL,
-                                         node1, node2);
+                                         node1, node2, FALSE, pool);
 }
 
 
@@ -1868,6 +1868,7 @@ merge(svn_stringbuf_t *conflict_p,
   */
   {
     node_revision_t *tgt_nr, *anc_nr, *src_nr;
+    svn_boolean_t same;
 
     /* Get node revisions for our id's. */
     SVN_ERR(svn_fs_fs__get_node_revision(&tgt_nr, fs, target_id, pool));
@@ -1876,16 +1877,15 @@ merge(svn_stringbuf_t *conflict_p,
 
     /* Now compare the prop-keys of the skels.  Note that just because
        the keys are different -doesn't- mean the proplists have
-       different contents.  But merge() isn't concerned with contents;
-       it doesn't do a brute-force comparison on textual contents, so
-       it won't do that here either.  Checking to see if the propkey
-       atoms are `equal' is enough. */
-    if (! svn_fs_fs__noderev_same_rep_key(src_nr->prop_rep, anc_nr->prop_rep))
+       different contents. */
+    SVN_ERR(svn_fs_fs__prop_rep_equal(&same, fs, src_nr, anc_nr, TRUE, pool));
+    if (! same)
       return conflict_err(conflict_p, target_path);
 
     /* The directory entries got changed in the repository but the directory
        properties did not. */
-    if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
+    SVN_ERR(svn_fs_fs__prop_rep_equal(&same, fs, tgt_nr, anc_nr, TRUE, pool));
+    if (! same)
       {
         /* There is an incoming prop change for this directory.
            We will accept it only if the directory changes were mere updates
@@ -3371,7 +3371,7 @@ fs_contents_changed(svn_boolean_t *chang
   SVN_ERR(get_dag(&node1, root1, path1, TRUE, pool));
   SVN_ERR(get_dag(&node2, root2, path2, TRUE, pool));
   return svn_fs_fs__dag_things_different(NULL, changed_p,
-                                         node1, node2);
+                                         node1, node2, FALSE, pool);
 }
 
 

Modified: subversion/trunk/subversion/libsvn_fs_util/fs-util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_util/fs-util.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_util/fs-util.c (original)
+++ subversion/trunk/subversion/libsvn_fs_util/fs-util.c Wed Feb 26 23:15:25 2014
@@ -286,3 +286,40 @@ svn_fs__compatible_version(svn_version_t
   *compatible_version = version;
   return SVN_NO_ERROR;
 }
+
+svn_boolean_t
+svn_fs__prop_lists_equal(apr_hash_t *a,
+                         apr_hash_t *b,
+                         apr_pool_t *pool)
+{
+  apr_hash_index_t *hi;
+
+  /* Quick checks and special cases. */
+  if (a == b)
+    return TRUE;
+
+  if (a == NULL)
+    return apr_hash_count(b) == 0;
+  if (b == NULL)
+    return apr_hash_count(a) == 0;
+
+  if (apr_hash_count(a) != apr_hash_count(b))
+    return FALSE;
+
+  /* Compare prop by prop. */
+  for (hi = apr_hash_first(pool, a); hi; hi = apr_hash_next(hi))
+    {
+      const char *key;
+      apr_ssize_t klen;
+      svn_string_t *val_a, *val_b;
+
+      apr_hash_this(hi, (const void **)&key, &klen, (void **)&val_a);
+      val_b = apr_hash_get(b, key, klen);
+
+      if (!val_b || !svn_string_compare(val_a, val_b))
+        return FALSE;
+    }
+
+  /* No difference found. */
+  return TRUE;
+}

Modified: subversion/trunk/subversion/libsvn_fs_x/dag.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/dag.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/dag.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/dag.c Wed Feb 26 23:15:25 2014
@@ -1258,29 +1258,37 @@ svn_error_t *
 svn_fs_x__dag_things_different(svn_boolean_t *props_changed,
                                svn_boolean_t *contents_changed,
                                dag_node_t *node1,
-                               dag_node_t *node2)
+                               dag_node_t *node2,
+                               svn_boolean_t strict,
+                               apr_pool_t *pool)
 {
   node_revision_t *noderev1, *noderev2;
+  svn_fs_t *fs;
+  svn_boolean_t same;
 
   /* If we have no place to store our results, don't bother doing
      anything. */
   if (! props_changed && ! contents_changed)
     return SVN_NO_ERROR;
 
+  fs = svn_fs_x__dag_get_fs(node1);
+
   /* The node revision skels for these two nodes. */
   SVN_ERR(get_node_revision(&noderev1, node1));
   SVN_ERR(get_node_revision(&noderev2, node2));
 
   /* Compare property keys. */
   if (props_changed != NULL)
-    *props_changed = (! svn_fs_x__noderev_same_rep_key(noderev1->prop_rep,
-                                                       noderev2->prop_rep));
+    {
+      SVN_ERR(svn_fs_x__prop_rep_equal(&same, fs, noderev1, noderev2,
+                                       strict, pool));
+      *props_changed = !same;
+    }
 
   /* Compare contents keys. */
   if (contents_changed != NULL)
-    *contents_changed =
-      (! svn_fs_x__noderev_same_rep_key(noderev1->data_rep,
-                                        noderev2->data_rep));
+    *contents_changed = !svn_fs_x__file_text_rep_equal(noderev1->data_rep,
+                                                       noderev2->data_rep);
 
   return SVN_NO_ERROR;
 }

Modified: subversion/trunk/subversion/libsvn_fs_x/dag.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/dag.h?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/dag.h (original)
+++ subversion/trunk/subversion/libsvn_fs_x/dag.h Wed Feb 26 23:15:25 2014
@@ -532,27 +532,25 @@ svn_error_t *svn_fs_x__dag_copy(dag_node
 

 /* Comparison */
 
-/* Find out what is the same between two nodes.
+/* Find out what is the same between two nodes.  If STRICT is FALSE,
+   this function may report false positives, i.e. report changes even
+   if the resulting contents / props are equal.
 
    If PROPS_CHANGED is non-null, set *PROPS_CHANGED to 1 if the two
    nodes have different property lists, or to 0 if same.
 
    If CONTENTS_CHANGED is non-null, set *CONTENTS_CHANGED to 1 if the
-   two nodes have different contents, or to 0 if same.  For files,
-   file contents are compared; for directories, the entries lists are
-   compared.  If one is a file and the other is a directory, the one's
-   contents will be compared to the other's entries list.  (Not
-   terribly useful, I suppose, but that's the caller's business.)
-
-   ### todo: This function only compares rep keys at the moment.  This
-   may leave us with a slight chance of a false positive, though I
-   don't really see how that would happen in practice.  Nevertheless,
-   it should probably be fixed.
+   two nodes have different contents, or to 0 if same.  NODE1 and NODE2
+   must refer to files from the same filesystem.
+
+   Use POOL for temporary allocations.
  */
 svn_error_t *svn_fs_x__dag_things_different(svn_boolean_t *props_changed,
                                             svn_boolean_t *contents_changed,
                                             dag_node_t *node1,
-                                            dag_node_t *node2);
+                                            dag_node_t *node2,
+                                            svn_boolean_t strict,
+                                            apr_pool_t *pool);
 
 
 /* Set *REV and *PATH to the copyroot revision and path of node NODE, or

Modified: subversion/trunk/subversion/libsvn_fs_x/fs_x.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/fs_x.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/fs_x.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/fs_x.c Wed Feb 26 23:15:25 2014
@@ -638,19 +638,81 @@ svn_fs_x__file_length(svn_filesize_t *le
 }
 
 svn_boolean_t
-svn_fs_x__noderev_same_rep_key(representation_t *a,
-                               representation_t *b)
+svn_fs_x__file_text_rep_equal(representation_t *a,
+                              representation_t *b)
 {
-  if (a == b)
+  svn_boolean_t a_empty = a == NULL || a->expanded_size == 0;
+  svn_boolean_t b_empty = b == NULL || b->expanded_size == 0;
+
+  /* This makes sure that neither rep will be NULL later on */
+  if (a_empty && b_empty)
     return TRUE;
 
-  if (a == NULL || b == NULL)
+  if (a_empty != b_empty)
     return FALSE;
 
-  return svn_fs_x__id_part_eq(&a->id, &b->id);
+  /* Same physical representation?  Note that these IDs are always up-to-date
+     instead of e.g. being set lazily. */
+  if (svn_fs_x__id_part_eq(&a->id, &b->id))
+    return TRUE;
+
+  /* Contents are equal if the checksums match.  These are also always known.
+   */
+  return memcmp(a->md5_digest, b->md5_digest, sizeof(a->md5_digest)) == 0
+      && memcmp(a->sha1_digest, b->sha1_digest, sizeof(a->sha1_digest)) == 0;
 }
 
 svn_error_t *
+svn_fs_x__prop_rep_equal(svn_boolean_t *equal,
+                         svn_fs_t *fs,
+                         node_revision_t *a,
+                         node_revision_t *b,
+                         svn_boolean_t strict,
+                         apr_pool_t *scratch_pool)
+{
+  representation_t *rep_a = a->prop_rep;
+  representation_t *rep_b = b->prop_rep;
+  apr_hash_t *proplist_a;
+  apr_hash_t *proplist_b;
+
+  /* Mainly for a==b==NULL */
+  if (rep_a == rep_b)
+    {
+      *equal = TRUE;
+      return SVN_NO_ERROR;
+    }
+
+  /* Committed property lists can be compared quickly */
+  if (   rep_a && rep_b
+      && svn_fs_x__is_revision(rep_a->id.change_set)
+      && svn_fs_x__is_revision(rep_b->id.change_set))
+    {
+      /* MD5 must be given. Having the same checksum is good enough for
+         accepting the prop lists as equal. */
+      *equal = memcmp(rep_a->md5_digest, rep_b->md5_digest,
+                      sizeof(rep_a->md5_digest)) == 0;
+      return SVN_NO_ERROR;
+    }
+
+  /* Skip the expensive bits unless we are in strict mode.
+     Simply assume that there is a different. */
+  if (!strict)
+    {
+      *equal = FALSE;
+      return SVN_NO_ERROR;
+    }
+
+  /* At least one of the reps has been modified in a txn.
+     Fetch and compare them. */
+  SVN_ERR(svn_fs_x__get_proplist(&proplist_a, fs, a, scratch_pool));
+  SVN_ERR(svn_fs_x__get_proplist(&proplist_b, fs, b, scratch_pool));
+
+  *equal = svn_fs__prop_lists_equal(proplist_a, proplist_b, scratch_pool);
+  return SVN_NO_ERROR;
+}
+
+
+svn_error_t *
 svn_fs_x__file_checksum(svn_checksum_t **checksum,
                         node_revision_t *noderev,
                         svn_checksum_kind_t kind,

Modified: subversion/trunk/subversion/libsvn_fs_x/fs_x.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/fs_x.h?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/fs_x.h (original)
+++ subversion/trunk/subversion/libsvn_fs_x/fs_x.h Wed Feb 26 23:15:25 2014
@@ -88,10 +88,21 @@ svn_error_t *svn_fs_x__file_length(svn_f
                                    node_revision_t *noderev,
                                    apr_pool_t *pool);
 
-/* Return TRUE if the representation keys in A and B both point to the
-   same representation, else return FALSE. */
-svn_boolean_t svn_fs_x__noderev_same_rep_key(representation_t *a,
-                                             representation_t *b);
+/* Return TRUE if the representations in A and B have equal contents, else
+   return FALSE. */
+svn_boolean_t svn_fs_x__file_text_rep_equal(representation_t *a,
+                                            representation_t *b);
+
+/* Set *EQUAL to TRUE if the property representations in A and B within FS
+   have equal contents, else set it to FALSE.  If STRICT is not set, allow
+   for false negatives.
+   Use SCRATCH_POOL for temporary allocations. */
+svn_error_t *svn_fs_x__prop_rep_equal(svn_boolean_t *equal,
+                                      svn_fs_t *fs,
+                                      node_revision_t *a,
+                                      node_revision_t *b,
+                                      svn_boolean_t strict,
+                                      apr_pool_t *scratch_pool);
 
 
 /* Return a copy of the representation REP allocated from POOL. */

Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/tree.c?rev=1572336&r1=1572335&r2=1572336&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/tree.c Wed Feb 26 23:15:25 2014
@@ -1603,7 +1603,8 @@ x_props_changed(svn_boolean_t *changed_p
 
   SVN_ERR(get_dag(&node1, root1, path1, TRUE, pool));
   SVN_ERR(get_dag(&node2, root2, path2, TRUE, pool));
-  return svn_fs_x__dag_things_different(changed_p, NULL, node1, node2);
+  return svn_fs_x__dag_things_different(changed_p, NULL, node1, node2,
+                                        FALSE, pool);
 }
 
 
@@ -1842,6 +1843,7 @@ merge(svn_stringbuf_t *conflict_p,
   */
   {
     node_revision_t *tgt_nr, *anc_nr, *src_nr;
+    svn_boolean_t same;
 
     /* Get node revisions for our id's. */
     SVN_ERR(svn_fs_x__get_node_revision(&tgt_nr, fs, target_id, pool));
@@ -1850,16 +1852,15 @@ merge(svn_stringbuf_t *conflict_p,
 
     /* Now compare the prop-keys of the skels.  Note that just because
        the keys are different -doesn't- mean the proplists have
-       different contents.  But merge() isn't concerned with contents;
-       it doesn't do a brute-force comparison on textual contents, so
-       it won't do that here either.  Checking to see if the propkey
-       atoms are `equal' is enough. */
-    if (! svn_fs_x__noderev_same_rep_key(src_nr->prop_rep, anc_nr->prop_rep))
+       different contents. */
+    SVN_ERR(svn_fs_x__prop_rep_equal(&same, fs, src_nr, anc_nr, TRUE, pool));
+    if (! same)
       return conflict_err(conflict_p, target_path);
 
     /* The directory entries got changed in the repository but the directory
        properties did not. */
-    if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
+    SVN_ERR(svn_fs_x__prop_rep_equal(&same, fs, tgt_nr, anc_nr, TRUE, pool));
+    if (! same)
       {
         /* There is an incoming prop change for this directory.
            We will accept it only if the directory changes were mere updates
@@ -3294,7 +3295,8 @@ x_contents_changed(svn_boolean_t *change
 
   SVN_ERR(get_dag(&node1, root1, path1, TRUE, pool));
   SVN_ERR(get_dag(&node2, root2, path2, TRUE, pool));
-  return svn_fs_x__dag_things_different(NULL, changed_p, node1, node2);
+  return svn_fs_x__dag_things_different(NULL, changed_p, node1, node2,
+                                        FALSE, pool);
 }