You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2016/10/13 16:06:38 UTC

svn commit: r1764721 - in /subversion/trunk/subversion: libsvn_client/conflicts.c tests/libsvn_client/conflicts-test.c

Author: stsp
Date: Thu Oct 13 16:06:38 2016
New Revision: 1764721

URL: http://svn.apache.org/viewvc?rev=1764721&view=rev
Log:
Start trying to detect nested moves in the conflict resolver.

This is a bit rough and likely not quite done yet.
But it already makes a test XPASS and it makes this simple test case work:

  svn mv epsilon epsilon2
  svn mv epsilon2/zeta zeta2

The resulting commit looks like this:

   D /trunk/epsilon
   A /trunk/epsilon2 (from /trunk/epsilon:2)
   D /trunk/epsilon2/zeta
   A /trunk/zeta2 (from /trunk/epsilon/zeta:2)

A tree conflict comes about because the branch edits the file epsilon/zeta.

The resolver recognizes that /trunk/epsilon2/zeta is a deleted child of the
moved /trunk/epsilon2 subtree. From this it infers that /trunk/epsilon/zeta
is the corresponding node deleted by this revision. It then determines that
the corresponding copy-to path /trunk/zeta2 is the move destination for
moved-away node /trunk/epsilon2/zeta.

* subversion/libsvn_client/conflicts.c
  (map_deleted_path_to_copy): New helper function. This checks whether a
   deletion happened inside a copy.
  (find_moves_in_revision): Account for moves inside copies. This function
   could use some refactoring for better readibility.
  (find_deleted_rev): Look for deletions which happened inside copies and
   return the appropriate deleted path if one is found.

* subversion/tests/libsvn_client/conflicts-test.c
  (test_funcs): Mark test_merge_incoming_move_dir_with_moved_file as PASS.

Modified:
    subversion/trunk/subversion/libsvn_client/conflicts.c
    subversion/trunk/subversion/tests/libsvn_client/conflicts-test.c

Modified: subversion/trunk/subversion/libsvn_client/conflicts.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/conflicts.c?rev=1764721&r1=1764720&r2=1764721&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/conflicts.c (original)
+++ subversion/trunk/subversion/libsvn_client/conflicts.c Thu Oct 13 16:06:38 2016
@@ -354,6 +354,45 @@ struct copy_info {
   svn_revnum_t copyfrom_rev;
 };
 
+/* If DELETED_RELPATH matches the copyfrom path of a copy in COPIES,
+ * or if DELETED_RELPATH is a child of a copy-to path in COPIES, return
+ * a struct copy_info for the corresponding copy. Else, return NULL. */
+static struct copy_info *
+map_deleted_path_to_copy(const char *deleted_relpath,
+                         apr_hash_t *copies,
+                         apr_pool_t *scratch_pool)
+{
+  apr_hash_index_t *hi;
+
+  for (hi = apr_hash_first(scratch_pool, copies);
+       hi != NULL;
+       hi = apr_hash_next(hi))
+    {
+      apr_array_header_t *copies_with_same_source_path = apr_hash_this_val(hi);
+      int i;
+
+      for (i = 0; i < copies_with_same_source_path->nelts; i++)
+        {
+          struct copy_info *copy;
+          const char *relpath;
+          
+          copy = APR_ARRAY_IDX(copies_with_same_source_path, i,
+                               struct copy_info *);
+          if (strcmp(copy->copyfrom_path, deleted_relpath) == 0)
+            return copy;
+
+          relpath = svn_relpath_skip_ancestor(copy->copyto_path,
+                                              deleted_relpath);
+          /* ### Should probably return the closest path-wise ancestor. */
+          if (relpath)
+            return copy;
+        }
+    }
+
+  return NULL;
+}
+
+
 /* Update MOVES_TABLE and MOVED_PATHS based on information from
  * revision data in LOG_ENTRY, COPIES, and DELETED_PATHS.
  * Use RA_SESSION to perform the necessary requests. */
@@ -369,36 +408,48 @@ find_moves_in_revision(svn_ra_session_t
                        apr_pool_t *scratch_pool)
 {
   apr_pool_t *iterpool;
-  apr_array_header_t *copies_with_same_source_path;
   svn_boolean_t related;
-  int i, j;
+  int i;
 
   iterpool = svn_pool_create(scratch_pool);
   for (i = 0; i < deleted_paths->nelts; i++)
     {
       const char *deleted_repos_relpath;
+      struct copy_info *copy;
+      const char *child_relpath;
+      struct repos_move_info *move;
+      struct repos_move_info *next_move;
+      svn_string_t *author;
+      apr_array_header_t *moves;
+      apr_array_header_t *copies_with_same_source_path;
+      int j;
+
+      svn_pool_clear(iterpool);
 
       deleted_repos_relpath = APR_ARRAY_IDX(deleted_paths, i, const char *);
 
-      /* See if we can match any copies to this deleted path. */
-      copies_with_same_source_path = apr_hash_get(copies,
-                                                  deleted_repos_relpath,
-                                                  APR_HASH_KEY_STRING);
-      if (copies_with_same_source_path == NULL)
+      /* See if we can match a copy to this deleted path. */
+      copy = map_deleted_path_to_copy(deleted_repos_relpath, copies, iterpool);
+      if (copy == NULL)
         continue;
 
-      for (j = 0; j < copies_with_same_source_path->nelts; j++)
+      child_relpath = svn_relpath_skip_ancestor(copy->copyto_path,
+                                                deleted_repos_relpath);
+      if (child_relpath && child_relpath[0] != '\0')
         {
-          struct copy_info *copy;
-          struct repos_move_info *move;
-          struct repos_move_info *next_move;
-          svn_string_t *author;
-          apr_array_header_t *moves;
-          
-          svn_pool_clear(iterpool);
+          /* Consider: cp A B; mv B/foo C/foo
+           * Copyfrom for C/foo is A/foo, even though C/foo was moved here from
+           * B/foo. A/foo was not deleted. It is B/foo which was deleted. */
+          deleted_repos_relpath = svn_relpath_join(copy->copyfrom_path,
+                                                   child_relpath, scratch_pool);
+        }
 
-          copy = APR_ARRAY_IDX(copies_with_same_source_path, j,
-                               struct copy_info *);
+      copies_with_same_source_path = svn_hash_gets(copies,
+                                                   deleted_repos_relpath);
+      copy = NULL;
+      for (j = 0; j < copies_with_same_source_path->nelts; j++)
+        {
+          struct copy_info *this_copy;
 
           /* We found a deleted node which matches the copyfrom path of
            * a copied node. Verify that the deleted node is an ancestor
@@ -406,71 +457,79 @@ find_moves_in_revision(svn_ra_session_t
            * from revision log_entry->revision-1 (where the deleted node is
            * guaranteed to exist) to the copyfrom-revision, we must end up
            * at the copyfrom-path. */
+          this_copy = APR_ARRAY_IDX(copies_with_same_source_path, j,
+                                    struct copy_info *);
           SVN_ERR(check_move_ancestry(&related, ra_session, repos_root_url,
                                       deleted_repos_relpath,
                                       log_entry->revision,
-                                      copy->copyfrom_path,
-                                      copy->copyfrom_rev,
+                                      this_copy->copyfrom_path,
+                                      this_copy->copyfrom_rev,
                                       TRUE, iterpool));
-          if (!related)
-            continue;
-
-          /* Remember details of this move. */
-          move = apr_pcalloc(result_pool, sizeof(*move));
-          move->moved_from_repos_relpath = apr_pstrdup(result_pool,
-                                                       deleted_repos_relpath);
-          move->moved_to_repos_relpath = apr_pstrdup(result_pool,
-                                                     copy->copyto_path);
-          move->rev = log_entry->revision;
-          author = svn_hash_gets(log_entry->revprops, SVN_PROP_REVISION_AUTHOR);
-          move->rev_author = apr_pstrdup(result_pool, author->data);
-          move->copyfrom_rev = copy->copyfrom_rev;
-
-          /* Link together multiple moves of the same node.
-           * Note that we're traversing history backwards, so moves already
-           * present in the list happened in younger revisions. */
-          next_move = svn_hash_gets(moved_paths, move->moved_to_repos_relpath);
-          if (next_move)
+          if (related)
             {
-              /* Tracing back history of the delete-half of the next move
-               * to the copyfrom-revision of the prior move we must end up
-               * at the delete-half of the prior move. */
-              SVN_ERR(check_move_ancestry(&related, ra_session, repos_root_url,
-                                          next_move->moved_from_repos_relpath,
-                                          next_move->rev,
-                                          move->moved_from_repos_relpath,
-                                          move->copyfrom_rev,
-                                          FALSE, iterpool));
-              if (related)
-                {
-                  SVN_ERR_ASSERT(move->rev < next_move->rev);
-
-                  /* Prepend this move to the linked list. */
-                  if (move->next == NULL)
-                    move->next = apr_array_make(
-                                   result_pool, 1,
-                                   sizeof (struct repos_move_info *));
-                  APR_ARRAY_PUSH(move->next,
-                                 struct repos_move_info *) = next_move;
-                  next_move->prev = move;
-                }
+              copy = this_copy;
+              break;
             }
+        }
 
-          /* Make this move the head of our next-move linking map. */
-          svn_hash_sets(moved_paths, move->moved_from_repos_relpath, move);
+      if (copy == NULL)
+        continue;
 
-          /* Add this move to the list of moves in this revision. */
-          moves = apr_hash_get(moves_table, &move->rev, sizeof(svn_revnum_t));
-          if (moves == NULL)
+      /* Remember details of this move. */
+      move = apr_pcalloc(result_pool, sizeof(*move));
+      move->moved_from_repos_relpath = apr_pstrdup(result_pool,
+                                                   deleted_repos_relpath);
+      move->moved_to_repos_relpath = apr_pstrdup(result_pool,
+                                                 copy->copyto_path);
+      move->rev = log_entry->revision;
+      author = svn_hash_gets(log_entry->revprops, SVN_PROP_REVISION_AUTHOR);
+      move->rev_author = apr_pstrdup(result_pool, author->data);
+      move->copyfrom_rev = copy->copyfrom_rev;
+
+      /* Link together multiple moves of the same node.
+       * Note that we're traversing history backwards, so moves already
+       * present in the list happened in younger revisions. */
+      next_move = svn_hash_gets(moved_paths, move->moved_to_repos_relpath);
+      if (next_move)
+        {
+          /* Tracing back history of the delete-half of the next move
+           * to the copyfrom-revision of the prior move we must end up
+           * at the delete-half of the prior move. */
+          SVN_ERR(check_move_ancestry(&related, ra_session, repos_root_url,
+                                      next_move->moved_from_repos_relpath,
+                                      next_move->rev,
+                                      move->moved_from_repos_relpath,
+                                      move->copyfrom_rev,
+                                      FALSE, iterpool));
+          if (related)
             {
-              /* This is the first move in this revision. Create the list. */
-              moves = apr_array_make(result_pool, 1,
-                                     sizeof(struct repos_move_info *));
-              apr_hash_set(moves_table, &move->rev, sizeof(svn_revnum_t),
-                           moves);
+              SVN_ERR_ASSERT(move->rev < next_move->rev);
+
+              /* Prepend this move to the linked list. */
+              if (move->next == NULL)
+                move->next = apr_array_make(
+                               result_pool, 1,
+                               sizeof (struct repos_move_info *));
+              APR_ARRAY_PUSH(move->next,
+                             struct repos_move_info *) = next_move;
+              next_move->prev = move;
             }
-          APR_ARRAY_PUSH(moves, struct repos_move_info *) = move;
         }
+
+      /* Make this move the head of our next-move linking map. */
+      svn_hash_sets(moved_paths, move->moved_from_repos_relpath, move);
+
+      /* Add this move to the list of moves in this revision. */
+      moves = apr_hash_get(moves_table, &move->rev, sizeof(svn_revnum_t));
+      if (moves == NULL)
+        {
+          /* This is the first move in this revision. Create the list. */
+          moves = apr_array_make(result_pool, 1,
+                                 sizeof(struct repos_move_info *));
+          apr_hash_set(moves_table, &move->rev, sizeof(svn_revnum_t),
+                       moves);
+        }
+      APR_ARRAY_PUSH(moves, struct repos_move_info *) = move;
     }
   svn_pool_destroy(iterpool);
 
@@ -611,6 +670,7 @@ find_deleted_rev(void *baton,
   apr_hash_index_t *hi;
   apr_pool_t *iterpool;
   svn_boolean_t deleted_node_found = FALSE;
+  svn_node_kind_t replacing_node_kind = svn_node_none;
   apr_array_header_t *deleted_paths;
   apr_hash_t *copies;
 
@@ -674,20 +734,12 @@ find_deleted_rev(void *baton,
                          struct copy_info *) = copy;
         }
 
-      /* For move detection, store all deleted_paths.
-       *
-       * ### This also stores deletions which happened inside copies.
-       * ### But we are not able to handle them at present.
-       * ### Consider: cp A B; mv B/foo C/foo
-       * ### Copyfrom for C/foo is now A/foo, even though C/foo was moved
-       * ### here from B/foo. We don't detect such moves at present since
-       * ### A/foo was not deleted. It is B/foo which was deleted.
-       */
+      /* For move detection, store all deleted_paths. */
       if (log_item->action == 'D' || log_item->action == 'R')
         APR_ARRAY_PUSH(deleted_paths, const char *) =
           apr_pstrdup(scratch_pool, changed_path);
 
-      /* Check if we found the deleted node we're looking for. */
+      /* Check if we already found the deleted node we're looking for. */
       if (!deleted_node_found &&
           svn_path_compare_paths(b->deleted_repos_relpath, changed_path) == 0 &&
           (log_item->action == 'D' || log_item->action == 'R'))
@@ -726,20 +778,8 @@ find_deleted_rev(void *baton,
               deleted_node_found = (yca_loc != NULL);
             }
 
-          if (deleted_node_found)
-            {
-              svn_string_t *author;
-
-              b->deleted_rev = log_entry->revision;
-              author = svn_hash_gets(log_entry->revprops,
-                                     SVN_PROP_REVISION_AUTHOR);
-              b->deleted_rev_author = apr_pstrdup(b->result_pool, author->data);
-                  
-              if (log_item->action == 'R')
-                b->replacing_node_kind = log_item->node_kind;
-              else
-                b->replacing_node_kind = svn_node_none;
-            }
+          if (deleted_node_found && log_item->action == 'R')
+            replacing_node_kind = log_item->node_kind;
         }
     }
   svn_pool_destroy(iterpool);
@@ -750,8 +790,47 @@ find_deleted_rev(void *baton,
                                  log_entry, copies, deleted_paths,
                                  b->repos_root_url,
                                  b->result_pool, scratch_pool));
+  if (!deleted_node_found)
+    {
+      apr_array_header_t *moves;
+
+      /* If the related node we're looking for was affected by
+       * a nested move we'll find it in here... */
+      moves = apr_hash_get(b->moves_table, &log_entry->revision,
+                           sizeof(svn_revnum_t));
+      if (moves)
+        {
+          int i;
+
+          for (i = 0; i < moves->nelts; i++)
+            {
+              struct repos_move_info *move_info;
+
+              move_info = APR_ARRAY_IDX(moves, i, struct repos_move_info *);
+              if (strcmp(move_info->moved_from_repos_relpath,
+                         b->related_repos_relpath) == 0)
+                {
+                  deleted_node_found = TRUE;
+                  /* ### override the deleted path */
+                  b->deleted_repos_relpath = b->related_repos_relpath;
+                  /* ### somehow set replacing_node_kind here ? */
+                  break;
+                }
+            }
+        }
+    }
+
   if (deleted_node_found)
     {
+      svn_string_t *author;
+
+      b->deleted_rev = log_entry->revision;
+      author = svn_hash_gets(log_entry->revprops,
+                             SVN_PROP_REVISION_AUTHOR);
+      b->deleted_rev_author = apr_pstrdup(b->result_pool, author->data);
+          
+      b->replacing_node_kind = replacing_node_kind;
+
       /* We're done. Abort the log operation. */
       return svn_error_create(SVN_ERR_CEASE_INVOCATION, NULL, NULL);
     }

Modified: subversion/trunk/subversion/tests/libsvn_client/conflicts-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_client/conflicts-test.c?rev=1764721&r1=1764720&r2=1764721&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_client/conflicts-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_client/conflicts-test.c Thu Oct 13 16:06:38 2016
@@ -3276,8 +3276,8 @@ static struct svn_test_descriptor_t test
                         "merge incoming edit for a moved-away working file"),
     SVN_TEST_OPTS_PASS(test_merge_incoming_chained_move_local_edit,
                        "merge incoming chained move vs local edit"),
-    SVN_TEST_OPTS_XFAIL(test_merge_incoming_move_dir_with_moved_file,
-                        "merge incoming moved dir with moved file"),
+    SVN_TEST_OPTS_PASS(test_merge_incoming_move_dir_with_moved_file,
+                       "merge incoming moved dir with moved file"),
     SVN_TEST_OPTS_PASS(test_merge_incoming_file_move_new_line_of_history,
                        "merge incoming file move with new line of history"),
     SVN_TEST_NULL