[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1687029 - /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Wed, 24 Jun 2015 01:22:58 +0200

Bert and Stefan,

The following patch also appears to fix the bug -- all tests pass.
However, see my further comments below.

[[[
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1687145)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -2791,18 +2791,12 @@ merge_dir_opened(void **new_dir_baton,

               SVN_ERR(svn_wc__node_is_added(&added, merge_b->ctx->wc_ctx,
                                             local_abspath, scratch_pool));

               db->tree_conflict_reason = added ? svn_wc_conflict_reason_added
                                                :
svn_wc_conflict_reason_obstructed;
-
- if ((merge_b->merge_source.ancestral ||
merge_b->reintegrate_merge)
- && !(pdb && pdb->shadowed))
- {
- store_path(merge_b->skipped_abspaths, local_abspath);
- }
             }
         }

       /* Handle pending conflicts */
       SVN_ERR(mark_dir_edited(merge_b, db, local_abspath, scratch_pool));

@@ -5395,20 +5389,12 @@ record_skips_in_mergeinfo(const char *me
                                         merge_b, skipped_abspath,
                                         iterpool));
       if (obstruction_state == svn_wc_notify_state_obstructed
           || obstruction_state == svn_wc_notify_state_missing)
         continue;

- /* Make sure this is not a tree-conflicted path either. We don't
- * want to add mergeinfo on such nodes since it causes problems down
- * the line (see issue #4582, "reintegrate complains about missing
- * ranges from node unrelated to branch") */
- if (svn_hash_gets(merge_b->tree_conflicted_abspaths,
- skipped_abspath) != NULL)
- continue;
-
       /* Add an empty range list for this path.

          ### TODO: This works fine for a file path skipped because it is
          ### missing as long as the file's parent directory is present.
          ### But missing directory paths skipped are not handled yet,
          ### see issue #2915.
]]]

Rather than adding code to ignore a 'skipped' path if it is also a
tree conflict path, this patch instead removes the code which recorded
an add-vs-add tree conflict victim as 'skipped'. That code block was
added recently (in http://svn.apache.org/r1666690) by Bert. Bert, can
you comment on it?

[[[
------------------------------------------------------------------------
r1666690 | rhuijben | 2015-03-14 15:30:17 +0100 (Sat, 14 Mar 2015) | 26 lines

In the libsvn_client merge code: When reporting and recording that we skip a
node because it is shadowed on an ancestor, properly record that the
ancestor is skipped instead of the descendant itself.

* subversion/libsvn_client/merge.c
  (record_skip): Only record a skip, if we really skipped this node, and
    not one of its ancestors. To do this handle this add a parent-baton
    argument to this function.
  (merge_file_changed,
   merge_file_added,
   merge_file_deleted): Update caller.

  (merge_dir_opened): Not only skip the directory add, but also record it
    as skipped.

  (merge_dir_changed,
   merge_dir_added,
   merge_dir_deleted,
   merge_node_absent): Update caller.

* subversion/tests/cmdline/merge_tree_conflict_tests.py
  (merge_obstruction_recording): New function.
  (test_list): Add merge_obstruction_recording.

Found by: Pete Harlan <pchpublic88{_AT_}gmail.com>
]]]

However, neither the current code nor the code resulting from this
patch looks entirely correct. Notice the first statement shown in the
patch context:

  SVN_ERR(svn_wc__node_is_added(&added,...));
  ...reason = added ? ...reason_added : ...reason_obstructed;

That looks at the WC schedule to decide whether to call this an
'added' or an 'obstructed' conflict. For a merge, I think if the
directory exists in the working state, then we should report the
reason as 'added' regardless whether the dir was added in a commit on
the checked-out branch or locally added in the WC. And I'm not
entirely sure when we should call it 'obstructed' -- maybe only when
there is a local unversioned object on disk where the WC working state
expects there is nothing?

- Julian
Received on 2015-06-24 01:31:39 CEST

This is an archived mail posted to the Subversion Dev mailing list.