[PATCH] Fix for issue 3067

Hi All,

I hereby attach the fix to issue 3067.

I am bit skeptical about performance of 'guess_live_ranges' in this patch.
But it would perform poorly only for the situations when
merge_src_subtree_at_revision2 does not exist and that too for one invocation.

Would like to know what others would think about it.

Thanks
With regards
Kamesh Jayachandran

Index: subversion/libsvn_client/mergeinfo.h
===================================================================
--- subversion/libsvn_client/mergeinfo.h (revision 31109)
+++ subversion/libsvn_client/mergeinfo.h (working copy)
@@ -56,6 +56,8 @@
                                            to a merge.*/
   svn_boolean_t indirect_mergeinfo;
   svn_boolean_t scheduled_for_deletion; /* PATH is scheduled for deletion. */
+ svn_boolean_t is_merge_source_deleted; /* corresponding merge source URL
+ of PATH is no more live. */
 } svn_client__merge_path_t;
 
 
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 31109)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -1485,6 +1485,214 @@
 }
 
 
+/*
+ Helper for get_applicable_requested_merge_ranges.
+ Populates *LIVE_RANGELIST with set of revision ranges
+ where PRIMARY_URL was LIVE.
+ If there are many PRIMARY_URL incarnations between
+ START_REV(youngest) and END_REV, Youngest incarnation is taken.
+ ### Or should we take all incarnations?.
+
+ If PRIMARY_URL_at_PEG_REV does not exist, it sets
+ *MERGE_SOURCE_DELETED to TRUE and calls this function recursively
+ with PEG_REV=PEG_REV-1 and START_REV=START_REV-1 if START_REV > END_REV.
+
+ If no PRIMARY_URL exists in anywhere between START_REV and END_REV,
+ populates *LIVE_RANGELIST with empty list.
+
+ Uses RA_SESSION to run the location segments report.
+
+ If IS_ROLLBACK is TRUE, segment ranges are reversed and populated
+ in LIVE_RANGELIST.
+
+ Cascades CTX.
+ All the allocations are made from POOL.
+*/
+static svn_error_t *
+guess_live_ranges(apr_array_header_t **live_rangelist,
+ svn_boolean_t *merge_source_deleted,
+ svn_revnum_t peg_rev,
+ svn_revnum_t start_rev,
+ svn_revnum_t end_rev,
+ const char *mergeinfo_path,
+ const char *primary_url,
+ svn_ra_session_t *ra_session,
+ svn_boolean_t is_rollback,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ apr_array_header_t *segments;
+ apr_array_header_t *history_rangelist =
+ apr_array_make(pool, 0, sizeof(svn_merge_range_t *));
+ const char *rel_mergeinfo_path = mergeinfo_path + 1;
+ const char *rel_source_path;
+ const char *session_url;
+ svn_error_t *err;
+
+ SVN_ERR(svn_ra_get_session_url(ra_session, &session_url, pool));
+
+ SVN_ERR(svn_client__path_relative_to_root(&rel_source_path,
+ primary_url,
+ session_url,
+ FALSE,
+ ra_session,
+ NULL,
+ pool));
+
+ err = svn_client__repos_location_segments(&segments, ra_session,
+ rel_source_path, peg_rev,
+ start_rev, end_rev, ctx, pool);
+
+ /* ### TODO: Is the behavior spelled out here correct and safe???
+ ### Can it break down with mixed-rev working copies?
+ ### Can it break down with non-ineritable ranges resulting
+ ### from missing subtrees?
+
+ The call to svn_client__repos_location_segments() above can have
+ any of two outcomes.
+
+ 1. REL_SOURCE_PATH_at_PEG_REV might not exist and hence errors of the
+ following kind,
+ SVN_ERR_FS_NOT_FOUND,
+ SVN_ERR_RA_DAV_PATH_NOT_FOUND
+ SVN_ERR_CLIENT_UNRELATED_RESOURCES
+ SVN_ERR_RA_DAV_REQUEST_FAILED (get this error over ra_neon!?).
+ When we get this error we re-run location segments report for
+ REL_SOURCE_PATH@(PEG_REV-1) for the range START_REV-1:END_REV.
+ */
+ if (err)
+ {
+ if (err->apr_err == SVN_ERR_FS_NOT_FOUND
+ || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND
+ || err->apr_err == SVN_ERR_CLIENT_UNRELATED_RESOURCES
+ /* If URL2_at_REVISION2 doesn't exist for a forward merge then we
+ get this error over ra_neon!? */
+ || err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED)
+ {
+ if (start_rev > end_rev)
+ SVN_ERR(guess_live_ranges(live_rangelist, merge_source_deleted,
+ peg_rev-1, start_rev-1, end_rev,
+ mergeinfo_path, primary_url, ra_session,
+ is_rollback, ctx, pool));
+ else
+ *live_rangelist = apr_array_make(pool, 1,
+ sizeof(svn_merge_range_t *));
+ *merge_source_deleted = TRUE;
+ /* Leave early. */
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+ }
+ else
+ return err;
+ }
+ else
+ {
+ /* Here we should have atleast one segment. */
+ if (segments->nelts)
+ {
+ int i;
+ for (i = 0; i < segments->nelts; i++)
+ {
+ svn_location_segment_t *segment =
+ APR_ARRAY_IDX(segments, i, svn_location_segment_t *);
+ if (!segment->path)
+ continue;
+ if (strcmp(segment->path, rel_mergeinfo_path) == 0)
+ {
+ svn_merge_range_t *history_range =
+ apr_pcalloc(pool, sizeof(svn_merge_range_t));
+ history_range->start =
+ is_rollback ? segment->range_end : segment->range_start;
+ history_range->end =
+ is_rollback ? segment->range_start : segment->range_end;
+ history_range->inheritable = TRUE;
+ APR_ARRAY_PUSH(history_rangelist,
+ svn_merge_range_t *) = history_range;
+ }
+ }
+ *live_rangelist = history_rangelist;
+ }
+ }
+ return SVN_NO_ERROR;
+}
+
+/*
+ Helper for filter_merged_revisions.
+
+ If it is invoked for merge target(IS_SUBTREE is FALSE), it populates
+ *REQUESTED_RANGELIST with set of revision ranges upon which merge was
+ requested.
+
+ If it is invoked for subtree of merge target(IS_SUBTREE is TRUE), it
+ populates *REQUESTED_RANGELIST with the help of guess_live_ranges.
+
+ Cascades IS_MERGE_SOURCE_DELETED, MERGEINFO_PATH, PRIMARY_URL,
+ RA_SESSION, CTX.
+
+ All the allocations are made from POOL.
+*/
+static svn_error_t *
+get_applicable_requested_merge_ranges(apr_array_header_t **requested_rangelist,
+ svn_boolean_t *is_merge_source_deleted,
+ svn_revnum_t revision1,
+ svn_revnum_t revision2,
+ const char *mergeinfo_path,
+ const char *primary_url,
+ svn_ra_session_t *ra_session,
+ svn_boolean_t is_rollback,
+ svn_boolean_t is_subtree,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ if (!is_subtree)
+ {
+ /* Convert REVISION1 and REVISION2 to a rangelist. */
+ svn_merge_range_t *range = apr_pcalloc(pool, sizeof(*range));
+ range->start = revision1;
+ range->end = revision2;
+ *requested_rangelist = apr_array_make(pool, 1, sizeof(range));
+ /* Talking about a requested merge range's inheritability doesn't
+ make much sense, but as we are using svn_merge_range_t to describe
+ it we need to pick *something*. Since all the rangelist
+ manipulations in this function either don't consider inheritance
+ by default or we are requesting that they don't (i.e.
+ svn_rangelist_remove and svn_rangelist_intersect) then we could
+ set the inheritability as FALSE, it won't matter either way. */
+ range->inheritable = TRUE;
+ APR_ARRAY_PUSH(*requested_rangelist, svn_merge_range_t *) = range;
+ }
+ else
+ {
+ /* When it is invoked for the merge target we know that PRIMARY_URL
+ to be existing for the revision range of the merge
+ because normalize_merge_sources() is providing it
+ -- see 'MERGEINFO MERGE SOURCE NORMALIZATION'. But if this is
+ invoked for subtree of merge target, then it is possible that
+ PRIMARY_URL_at_REVISION1 and/or PRIMARY_URL_at_REVISION2 don't exist.
+
+ In the case of a forward merge (i.e. REVISION1 < REVISION2) this
+ could be because PRIMARY_URL didn't come into existence until *after*
+ REVISION1 and/or it was deleted prior to REVISION2.
+
+ In the case of a rollback merge (i.e. REVISION1 > REVISION2) this
+ could be because PRIMARY_URL didn't come into existence until *after*
+ REVISION2 and/or it was deleted prior to REVISION1.
+
+ In either case we don't want to try to merge a non-existent source to
+ the subtree so we fill of *REQUESTED_RANGELIST. with only
+ the revision ranges where PRIMARY_URL was in existence. */
+
+ svn_revnum_t peg_rev = is_rollback ? revision1 : revision2;
+ svn_revnum_t start_rev = is_rollback ? revision1 : revision2;
+ svn_revnum_t end_rev = is_rollback ? revision2 : revision1;
+ SVN_ERR(guess_live_ranges(requested_rangelist, is_merge_source_deleted,
+ peg_rev, start_rev, end_rev, mergeinfo_path,
+ primary_url, ra_session, is_rollback,
+ ctx, pool));
+ }
+ return SVN_NO_ERROR;
+}
+
 /*-----------------------------------------------------------------------*/
 
 /*** Determining What Remains To Be Merged ***/
@@ -1506,40 +1714,46 @@
    IMPLICIT_MERGEINFO is the path's natural history described as
    mergeinfo - see svn_client__get_history_as_mergeinfo().
 
+ It sets the CHILD->remaining_ranges with set of revisions to merge.
+
+ It sets the CHILD->is_merge_source_deleted to TRUE if PRIMARY_URL is
+ *not* existing between REVISION1 and REVISION2.
+
+ Cascades REVISION1, REVISION2, PRIMARY_URL, IS_SUBTREE, RA_SESSION, CTX.
+
    NOTE: This should only be called when honoring mergeinfo.
 */
 static svn_error_t *
-filter_merged_revisions(apr_array_header_t **remaining_ranges,
+filter_merged_revisions(svn_client__merge_path_t *child,
                         const char *mergeinfo_path,
                         svn_mergeinfo_t target_mergeinfo,
                         svn_mergeinfo_t implicit_mergeinfo,
                         svn_revnum_t revision1,
                         svn_revnum_t revision2,
- const svn_wc_entry_t *entry,
+ const char *primary_url,
+ svn_ra_session_t *ra_session,
+ svn_boolean_t is_rollback,
+ svn_boolean_t is_subtree,
+ svn_client_ctx_t *ctx,
                         apr_pool_t *pool)
 {
   apr_array_header_t *target_rangelist = NULL;
   svn_mergeinfo_t mergeinfo = implicit_mergeinfo;
   apr_array_header_t *requested_merge;
- svn_merge_range_t *range;
+ SVN_ERR(get_applicable_requested_merge_ranges(
+ &requested_merge,
+ &child->is_merge_source_deleted,
+ revision1,
+ revision2,
+ mergeinfo_path,
+ primary_url,
+ ra_session,
+ is_rollback,
+ is_subtree,
+ ctx,
+ pool));
 
- /* Convert REVISION1 and REVISION2 to a rangelist. */
- range = apr_pcalloc(pool, sizeof(*range));
- requested_merge = apr_array_make(pool, 1, sizeof(range));
- range->start = revision1;
- range->end = revision2;
- /* Talking about a requested merge range's inheritability doesn't
- make much sense, but as we are using svn_merge_range_t to describe
- it we need to pick *something*. Since all the rangelist
- manipulations in this function either don't consider inheritance
- by default or we are requesting that they don't (i.e.
- svn_rangelist_remove and svn_rangelist_intersect) then we could
- set the inheritability as FALSE, it won't matter either way. */
- range->inheritable = TRUE;
-
- APR_ARRAY_PUSH(requested_merge, svn_merge_range_t *) = range;
-
- if (revision1 > revision2) /* Is this a reverse merge? */
+ if (is_rollback) /* Is this a reverse merge? */
     {
       if (target_mergeinfo)
         {
@@ -1573,21 +1787,21 @@
              even if that path has no explicit mergeinfo prior to the
              merge -- See condition 3 in the doc string for
              merge.c:get_mergeinfo_paths(). */
- SVN_ERR(svn_rangelist_intersect(remaining_ranges,
+ SVN_ERR(svn_rangelist_intersect(&(child->remaining_ranges),
                                           target_rangelist,
                                           requested_merge, FALSE, pool));
 
- SVN_ERR(svn_rangelist_reverse(*remaining_ranges, pool));
+ SVN_ERR(svn_rangelist_reverse(child->remaining_ranges, pool));
         }
       else
         {
- *remaining_ranges =
+ child->remaining_ranges =
             apr_array_make(pool, 1, sizeof(svn_merge_range_t *));
         }
     }
   else
     {
- *remaining_ranges = requested_merge;
+ child->remaining_ranges = requested_merge;
 
 /* ### TODO: Which evil shall we choose?
    ###
@@ -1625,14 +1839,19 @@
       /* See earlier comment preceeding svn_rangelist_intersect() for
          why we don't consider inheritance here. */
       if (target_rangelist)
- SVN_ERR(svn_rangelist_remove(remaining_ranges, target_rangelist,
+ SVN_ERR(svn_rangelist_remove(&(child->remaining_ranges),
+ target_rangelist,
                                      requested_merge, FALSE, pool));
     }
   return SVN_NO_ERROR;
 }
 
-/* Populate *REMAINING_RANGES with a list of revision ranges
+/* Helper for do_file_merge and do_directory_merge (by way of
+ populate_remaining_ranges() for the latter).
+
+ Populate CHILD->REMAINING_RANGES with a list of revision ranges
    constructed by taking after removing already-merged ranges.
+
    Cascade SOURCE_ROOT_URL, URL1, REVISION1, URL2, REVISION2, INHERITABLE,
    TARGET_MERGEINFO, IMPLICIT_MERGEINFO, RA_SESSION, ENTRY, CTX, and POOL.
 
@@ -1645,9 +1864,13 @@
    URL1_at_REVISION1, URL2_at_REVISION2, and ENTRY are all on the same line of
    history but ENTRY-REVISION is older than the REVISION1-REVISION2 range, see
    comment re issue #2973 below.
+
+ If IS_SUBTREE is TRUE, then ENTRY describes a subtree of the merge target.
+ Therefore IS_SUBTREE should always be FALSE when calling from
+ do_file_merge().
 */
 static svn_error_t *
-calculate_remaining_ranges(apr_array_header_t **remaining_ranges,
+calculate_remaining_ranges(svn_client__merge_path_t *child,
                            const char *source_root_url,
                            const char *url1,
                            svn_revnum_t revision1,
@@ -1655,23 +1878,27 @@
                            svn_revnum_t revision2,
                            svn_mergeinfo_t target_mergeinfo,
                            svn_mergeinfo_t implicit_mergeinfo,
+ svn_boolean_t is_subtree,
+ svn_boolean_t is_rollback,
                            svn_ra_session_t *ra_session,
                            const svn_wc_entry_t *entry,
                            svn_client_ctx_t *ctx,
                            apr_pool_t *pool)
 {
   const char *mergeinfo_path;
- const char *primary_url = (revision1 < revision2) ? url2 : url1;
+ const char *primary_url = is_rollback ? url1 : url2;
 
- /* Determine which portions of the requested merge range still
- need merging. */
+ /* Determine which of the requested ranges to consider merging... */
   SVN_ERR(svn_client__path_relative_to_root(&mergeinfo_path, primary_url,
                                             source_root_url, TRUE,
                                             ra_session, NULL, pool));
- SVN_ERR(filter_merged_revisions(remaining_ranges, mergeinfo_path,
+ SVN_ERR(filter_merged_revisions(child, mergeinfo_path,
                                   target_mergeinfo, implicit_mergeinfo,
- revision1, revision2, entry, pool));
+ revision1, revision2, primary_url,
+ ra_session, is_rollback,
+ is_subtree, ctx, pool));
 
+
   /* Issue #2973 -- from the continuing series of "Why, since the advent of
      merge tracking, allowing merges into mixed rev and locally modified
      working copies isn't simple and could be considered downright evil".
@@ -1698,7 +1925,7 @@
      So in the name of user friendliness, return an error suggesting a helpful
      course of action.
   */
- if (((*remaining_ranges)->nelts == 0)
+ if (((child->remaining_ranges)->nelts == 0)
       && (revision2 < revision1)
       && (entry->revision <= revision2))
     {
@@ -1855,6 +2082,8 @@
 
    See `MERGEINFO MERGE SOURCE NORMALIZATION' for more requirements
    around the values of URL1, REVISION1, URL2, and REVISION2.
+
+ Cascades IS_ROLLBACK.
 */
 static svn_error_t *
 populate_remaining_ranges(apr_array_header_t *children_with_mergeinfo,
@@ -1865,6 +2094,7 @@
                           svn_revnum_t revision2,
                           svn_boolean_t inheritable,
                           svn_boolean_t honor_mergeinfo,
+ svn_boolean_t is_rollback,
                           svn_ra_session_t *ra_session,
                           const char *parent_merge_src_canon_path,
                           svn_wc_adm_access_t *adm_access,
@@ -1933,12 +2163,14 @@
                                  MIN(revision1, revision2),
                                  adm_access, merge_b->ctx, pool));
 
- SVN_ERR(calculate_remaining_ranges(&(child->remaining_ranges),
+ SVN_ERR(calculate_remaining_ranges(child,
                                          source_root_url,
                                          child_url1, revision1,
                                          child_url2, revision2,
                                          child->pre_merge_mergeinfo,
                                          child->implicit_mergeinfo,
+ i > 0 ? TRUE : FALSE,
+ is_rollback,
                                          ra_session, child_entry, merge_b->ctx,
                                          pool));
     }
@@ -2422,6 +2654,11 @@
               /* Nothing to merge to this child. We'll claim we have
                  it up to date so the server doesn't send us
                  anything. */
+
+ if (child->remaining_ranges->nelts == 0
+ && child->is_merge_source_deleted)
+ continue;
+
               SVN_ERR(reporter->set_path(report_baton, child_repos_path,
                                          revision2, depth, FALSE,
                                          NULL, pool));
@@ -4022,6 +4259,7 @@
   if (honor_mergeinfo)
     {
       const char *source_root_url;
+ svn_client__merge_path_t *item = apr_pcalloc(pool, sizeof(*item));
             
       SVN_ERR(svn_ra_get_repos_root2(merge_b->ra_session1,
                                      &source_root_url, pool));
@@ -4046,13 +4284,14 @@
          by REVISION1:REVISION2. */
       if (!merge_b->record_only)
         {
- SVN_ERR(calculate_remaining_ranges(&remaining_ranges,
+ SVN_ERR(calculate_remaining_ranges(item,
                                              source_root_url,
                                              url1, revision1, url2, revision2,
                                              target_mergeinfo,
- implicit_mergeinfo,
- merge_b->ra_session1,
+ implicit_mergeinfo, FALSE,
+ is_rollback, merge_b->ra_session1,
                                              entry, ctx, pool));
+ remaining_ranges=item->remaining_ranges;
         }
     }
 
@@ -4367,7 +4606,7 @@
                                     source_root_url,
                                     url1, revision1, url2, revision2,
                                     inheritable, honor_mergeinfo,
- ra_session, mergeinfo_path,
+ is_rollback, ra_session, mergeinfo_path,
                                     adm_access, merge_b));
 
   /* Always start with a range which describes our most inclusive merge. */
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 31109)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -11010,8 +11010,6 @@
 # that don't exist at the start of a merge range shouldn't break the
 # merge'. Specifically see
 # http://subversion.tigris.org/issues/show_bug.cgi?id=3067#desc5
-#
-# Set as XFail until that issue is resolved.
 def dont_merge_revs_into_subtree_that_predate_it(sbox):
   "dont merge revs into a subtree that predate it"
 
@@ -11509,8 +11507,8 @@
                          server_has_mergeinfo),
               SkipUnless(reverse_merge_away_all_mergeinfo,
                          server_has_mergeinfo),
- XFail(SkipUnless(dont_merge_revs_into_subtree_that_predate_it,
- server_has_mergeinfo)),
+ SkipUnless(dont_merge_revs_into_subtree_that_predate_it,
+ server_has_mergeinfo),
               XFail(SkipUnless(merge_chokes_on_renamed_subtrees,
                                server_has_mergeinfo)),
               SkipUnless(dont_explicitly_record_implicit_mergeinfo,

[[[
Fix issue 3067.

* subversion/libsvn_client/mergeinfo.h
  (svn_client__merge_path_t): Add 'is_merge_source_deleted' to track the
   dead merge source.
* subversion/libsvn_client/merge.c
  (guess_live_ranges, get_applicable_requested_merge_ranges): New functions.
  (filter_merged_revisions): Removed remaining_ranges instead pass
   child and populate child->remaining_ranges.
   Remove parameter entry.
   New parameters primary_url, ra_session, is_rollback, is_subtree, ctx.
   Make use of 'get_applicable_requested_merge_ranges' to determine
   the applicable merge ranges for the target and its child with
   mergeinfo instead of doing it in this function itself.
   Changes to reflect the change in parameter remaining_ranges.
  (calculate_remaining_ranges): Removed remaining_ranges instead pass
   child and populate child->remaining_ranges.
   New parameters is_rollback, is_subtree.
   Make use of the new parameter is_rollback.
   Call 'filter_merged_revisions' as per its new signature.
   Changes to reflect the change in parameter remaining_ranges.
  (populate_remaining_ranges): New parameter is_rollback.
   Call 'calculate_remaining_ranges' as per its new signature.
  (drive_merge_report_editor): Don't try to set no-op diff for child if
   child->remaining_ranges->nelts == 0 and child->is_merge_source_deleted.
   or else we will get url_corresponding_to_child_at_revision2 does not exist
   error.
  (do_file_merge): Call 'calculate_remaining_ranges' as per its new signature.
  (do_directory_merge): Call 'populate_remaining_ranges' as per its new
   signature.

* subversion/tests/cmdline/merge_tests.py
  (dont_merge_revs_into_subtree_that_predate_it): Remove comment stating the
   testcase as XFail.
  (test_list): Remove XFail marker from
   dont_merge_revs_into_subtree_that_predate_it.

Patch by: me, pburba
]]]