In automatic (tracked) merges, detect when a revision to be merged is itself the result of a merge in the opposite direction (often called a reflective merge), and skip any such revisions. This enables the user to sync a branch that has previously been reintegrated to its parent, without needing to do either of the two currently recommended work-arounds (the 'keep-alive dance' or deleting and recreating the branch). Future improvements: - Detect when a revision contains both reflective merges and other merges, and if so then issue a friendly diagnostic message to let the user know that they will need to merge the other changes manually because this case is beyond the capability of Subversion's automatic merge tracking. - Examine subtree mergeinfo as well as merge-root mergeinfo. (At present, any revision that has reflective mergeinfo at the merge-root will be skipped across all sub-trees, but a revision that only shows up as reflective if we look at its subtree mergeinfo will not be skipped.) - Work also for a reverse merge. (Just need to generalize the range arithmetic w.r.t. exclusive and inclusive end-points.) - Work also for a single-file merge. (But, ugh, the code is completely different.) ### Test failures: ### blame_tests.py 10: test 'svn blame -g' ### blame_tests.py 11: don't look for merged files out of range ### log_tests.py 16-19: test 'svn log -g' on ... ### merge_tests.py 93: natural history filtering permits valid mergeinfo * subversion/libsvn_client/merge.c (show_mergeinfo): New (for debugging only). (fetch_repos_mergeinfo, incoming_mergeinfo_diff, is_reflected_merge, find_reflected_rev, remove_reflected_revs): New functions. (do_directory_merge): Call remove_reflected_revs(). * subversion/tests/cmdline/merge_reintegrate_tests.py (simple_reintegrate, simple_sync_merge): New functions. (reintegrate_keep_alive1, reintegrate_keep_alive2): New tests. (test_list): Add the new tests. --This line, and those below, will be ignored-- Index: subversion/libsvn_client/merge.c =================================================================== --- subversion/libsvn_client/merge.c (revision 1231904) +++ subversion/libsvn_client/merge.c (working copy) @@ -8348,6 +8348,317 @@ remove_noop_subtree_ranges(const merge_s return SVN_NO_ERROR; } +/* ### for debug prints */ +static const char * +show_mergeinfo(svn_mergeinfo_t mergeinfo, + apr_pool_t *scratch_pool) +{ + svn_string_t *str; + + svn_error_clear(svn_mergeinfo__to_formatted_string( + &str, mergeinfo, "DBG: ", scratch_pool)); + return str->data; +} + +/* Set *MERGEINFO to the mergeinfo from the repository node URL at REVNUM. + * If there is no mergeinfo, the result is empty (not null). + * Use RA_SESSION, reparenting it temporarily. + */ +static svn_error_t * +fetch_repos_mergeinfo(svn_mergeinfo_t *mergeinfo, + const char *url, + svn_revnum_t revnum, + svn_ra_session_t *ra_session, + apr_pool_t *result_pool) +{ + SVN_ERR(svn_client__get_repos_mergeinfo( + mergeinfo, ra_session, url, revnum, + svn_mergeinfo_inherited, TRUE /* squelch_incapable */, + result_pool)); + + if (! *mergeinfo) + *mergeinfo = apr_hash_make(result_pool); + return SVN_NO_ERROR; +} + +/* Find the changes in mergeinfo between the left and right sides of SOURCE. + * Set *DELETED to the deleted mergeinfo and *ADDED to the added mergeinfo; + * neither of these results will be null. + * + * ### TODO: Optimization: cache the mergeinfo (in SOURCE?), instead of + * always fetching from the repo. + */ +static svn_error_t * +incoming_mergeinfo_diff(svn_mergeinfo_t *deleted, + svn_mergeinfo_t *added, + const merge_source_t *source, + svn_ra_session_t *ra_session, + svn_client_ctx_t *ctx, + apr_pool_t *result_pool, + apr_pool_t *scratch_pool) +{ + svn_mergeinfo_t mergeinfo_left, mergeinfo_right; + + SVN_ERR(fetch_repos_mergeinfo(&mergeinfo_left, + source->url1, source->rev1, + ra_session, scratch_pool)); + SVN_ERR(fetch_repos_mergeinfo(&mergeinfo_right, + source->url2, source->rev2, + ra_session, scratch_pool)); + + SVN_ERR(svn_mergeinfo_diff2(deleted, added, + mergeinfo_left, mergeinfo_right, + TRUE /* consider_inheritance */, + result_pool, scratch_pool)); + return SVN_NO_ERROR; +} + + +/* Look at the overall change in mergeinfo (that is, compare + * mergeinfo in SOURCE left with SOURCE right), and see if + * there is any addition of mergeinfo about merges *from* TARGET. + * + * Set *reflected_completely to true if the change in mergeinfo contains + * any merges from TARGET. + * + * TODO: Set *reflected_completely to true if the change in mergeinfo + * consists of nothing more that changes that are present in TARGET, + * taking into consideration that a merge 'MB' from another branch 'B' might + * show up because of having been merged via TARGET. So, if the merge + * directly from TARGET is 'MT', and the corresponding change(s) on TARGET + * brought in the change 'MB', then we can ignore 'MB' because we know it + * was a transitive merge. (### What if in fact MB was transitive in the + * other direction?) + */ +static svn_error_t * +is_reflected_merge(svn_boolean_t *reflected, + svn_boolean_t *reflected_completely, + apr_array_header_t *children_with_mergeinfo, + const merge_source_t *source, + svn_ra_session_t *ra_session, + svn_client_ctx_t *ctx, + apr_pool_t *scratch_pool) +{ + svn_client__merge_path_t *root_child = + APR_ARRAY_IDX(children_with_mergeinfo, 0, svn_client__merge_path_t *); + svn_mergeinfo_t deleted; + svn_mergeinfo_t added; + + SVN_ERR(incoming_mergeinfo_diff(&deleted, &added, source, ra_session, + ctx, scratch_pool, scratch_pool)); + /* Find which differences represent merges from TARGET_LOCATIONS. */ + SVN_ERR(svn_mergeinfo_intersect2(&deleted, deleted, + root_child->implicit_mergeinfo, + FALSE /* consider_inheritance */, + scratch_pool, scratch_pool)); + SVN_ERR(svn_mergeinfo_intersect2(&added, added, + root_child->implicit_mergeinfo, + FALSE /* consider_inheritance */, + scratch_pool, scratch_pool)); + + if (apr_hash_count(deleted)) + SVN_DBG(("r%ld:%ld mi deleted:\n%s", + source->rev1, source->rev2, show_mergeinfo(deleted, scratch_pool))); + if (apr_hash_count(added)) + SVN_DBG(("r%ld:%ld mi added:\n%s", + source->rev1, source->rev2, show_mergeinfo(added, scratch_pool))); + + *reflected = (apr_hash_count(deleted) || apr_hash_count(added)); + *reflected_completely = FALSE; + return SVN_NO_ERROR; +} + + +/* Are we trying to merge into TGT a change that contains a merge + * *from* TGT? If so, that is probably not desired + * and will probably result in conflicts; certainly it is logically + * wrong. The exception is if rollback, reverse-merging or other + * forms of undoing are involved; but these are not fully supported + * by merge tracking. + * + * At the very least we should detect and warn. Better, we could + * automatically skip such already-present changes. + * + * If the entire incoming change (or any single revision of it) says + * it is (or includes) a merge from TGT, then we know it is not a + * safe change to merge, and we can at least warn about it. + * + * If we can detect that one revision of the incoming change consists + * entirely of a change merged from TGT then we should skip it. That + * will enable us to automatically ignore the following changes in SRC: + * + * * the result of a reintegrate merge from TGT + * * the result of a cherry-pick merge from TGT + * * the result of any tracked merge from some other branch, that + * was equivalent (in the merge tracking sense) to a merge from TGT + */ + +/* Given an incoming (multi-rev) change SOURCE, and a target described by + * (the first item of) CHILDREN_WITH_MERGEINFO, set *REV to the first + * reflective merge in SOURCE, or SVN_INVALID_REVNUM if none. + * + * This implements algorithm 1. + * + * ### TODO: Support a reverse range. Presently assumes a forward range. + * + * Algorithm 1: + * + * def find(RANGE): + * # Precondition: rev range RANGE = SOURCE may contain a reflected merge. + * # Postcondition: returns revision R which is in RANGE, or NULL. + * if is_reflected_merge(RANGE): + * if RANGE is a single (operative) revision: + * return R = that revision + * else: + * R := find(first half of RANGE) + * if R: + * return R + * R := find(second half of RANGE) + * assert(R) # because if RANGE not reflective we should + * have taken the outer 'else' branch + * return R + * else: + * return NULL + * + * Algorithm 2: + * + * Note: more efficient than algorithm 1. + * + * def find(RANGE): + * # Precondition: RANGE contains a reflected merge. + * # Postcondition: returns revision R which is in RANGE. + * if RANGE is a single (operative) revision: + * return R = that revision + * else: + * RANGE1 := first half of RANGE + * if is_reflected_merge(RANGE1): + * return find(RANGE1) + * else: + * RANGE2 := second half of RANGE + * return find(RANGE2) + * + * Start: + * if is_reflected_merge(SOURCE): + * return find(SOURCE) + * else: + * return NULL + */ +static svn_error_t * +find_reflected_rev(svn_revnum_t *rev, + apr_array_header_t *children_with_mergeinfo, + const merge_source_t *source, + svn_ra_session_t *ra_session, + svn_client_ctx_t *ctx, + apr_pool_t *scratch_pool) +{ + svn_boolean_t reflected, reflected_completely; + + SVN_ERR(is_reflected_merge(&reflected, &reflected_completely, + children_with_mergeinfo, source, + ra_session, ctx, scratch_pool)); + /* SVN_DBG(("r=%ld:%ld reflected=%d\n", source->rev1, source->rev2, reflected)); */ + + if (reflected) + { + if (source->rev1 + 1 == source->rev2) + { + *rev = source->rev2; + } + else + { + svn_revnum_t half_way = (source->rev1 + source->rev2) / 2; + merge_source_t *source1 = subrange_source(source, source->rev1, + half_way, scratch_pool); + SVN_ERR(find_reflected_rev(rev, children_with_mergeinfo, source1, + ra_session, ctx, scratch_pool)); + if (! SVN_IS_VALID_REVNUM(*rev)) + { + merge_source_t *source2 + = subrange_source(source, half_way, source->rev2, scratch_pool); + SVN_ERR(find_reflected_rev(rev, children_with_mergeinfo, source2, + ra_session, ctx, scratch_pool)); + SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(*rev)); + } + } + } + else + { + *rev = SVN_INVALID_REVNUM; + } + + return SVN_NO_ERROR; +} + +/* Skip any individual change (revision) on the source that was the + * result of a merge from the (current) target branch. This is a + * special case of support for 'reflective' merges. In particular, + * we want to skip any source change that was the result of a + * reintegrate merge from the (current) target. + * + * If it's a record-only merge, then we don't care and we must + * allow this case because it's the historically recommended + * way of keeping a reintegrated branch alive. + */ +static svn_error_t * +remove_reflected_revs(const merge_source_t *source, + svn_ra_session_t *ra_session, + apr_array_header_t *children_with_mergeinfo, + merge_cmd_baton_t *merge_b, + apr_pool_t *result_pool, + apr_pool_t *scratch_pool) +{ + svn_client__merge_path_t *root_child = + APR_ARRAY_IDX(children_with_mergeinfo, 0, svn_client__merge_path_t *); + const merge_source_t *this_source = source; + apr_pool_t *iterpool = svn_pool_create(scratch_pool); + + if (! root_child->implicit_mergeinfo || merge_b->record_only) + return SVN_NO_ERROR; + + while (this_source->rev1 < this_source->rev2) + { + svn_revnum_t rev; + int i; + + svn_pool_clear(iterpool); + SVN_ERR(find_reflected_rev(&rev, children_with_mergeinfo, this_source, + merge_b->ra_session1, + merge_b->ctx, iterpool)); + if (rev == SVN_INVALID_REVNUM) + { + break; + } + + /* Part of the incoming change is reflective. */ + SVN_DBG((_("Skipping reflective revision r%ld\n"), rev)); + + /* Remove this revision from target and all children. */ + for (i = 0; i < children_with_mergeinfo->nelts; i++) + { + svn_client__merge_path_t *child + = APR_ARRAY_IDX(children_with_mergeinfo, i, + svn_client__merge_path_t *); + + if (child->remaining_ranges && child->remaining_ranges->nelts) + { + SVN_ERR(svn_rangelist_remove(&child->remaining_ranges, + svn_rangelist__initialize( + rev - 1, rev, TRUE, iterpool), + child->remaining_ranges, + FALSE, result_pool)); + } + } + + this_source = subrange_source(this_source, + rev /*exclusive*/, source->rev2 /*inclusive*/, + scratch_pool); + } + + svn_pool_destroy(iterpool); + return SVN_NO_ERROR; +} + + /* Helper for do_merge() when the merge target is a directory. Perform a merge of changes in SOURCE to the working copy path @@ -8486,6 +8797,13 @@ do_directory_merge(svn_mergeinfo_catalog if (SVN_IS_VALID_REVNUM(new_range_start)) range.start = new_range_start; + /* Don't merge any revision that represents a reflected merge (a merge + of a change that was earlier merged *from* the current target). */ + SVN_ERR(remove_reflected_revs(source, + ra_session, + notify_b->children_with_mergeinfo, + merge_b, scratch_pool, iterpool)); + /* Remove inoperative ranges from any subtrees' remaining_ranges to spare the expense of noop editor drives. */ if (!is_rollback) Index: subversion/tests/cmdline/merge_reintegrate_tests.py =================================================================== --- subversion/tests/cmdline/merge_reintegrate_tests.py (revision 1231904) +++ subversion/tests/cmdline/merge_reintegrate_tests.py (working copy) @@ -2541,7 +2541,138 @@ expected_skip, [], None, None, None, None, True, True, '--reintegrate', A_path) - + +#---------------------------------------------------------------------- + +def simple_reintegrate(sbox, source, target_wcpath): + """""" + was_cwd = os.getcwd() + os.chdir(sbox.wc_dir) + svntest.actions.run_and_verify_svn(None, None, [], + 'merge', '--reintegrate', + source, target_wcpath) + os.chdir(was_cwd) + +def simple_sync_merge(sbox, source, target_wcpath): + """""" + was_cwd = os.getcwd() + os.chdir(sbox.wc_dir) + svntest.actions.run_and_verify_svn(None, None, [], + 'merge', + source, target_wcpath) + os.chdir(was_cwd) + +@SkipUnless(server_has_mergeinfo) +def reintegrate_keep_alive1(sbox): + "reintegrate with automatic keep-alive" + + # Test the ability to keep using a branch after reintegrating it. + # Make a branch, reintegrate it to trunk, and then sync the branch without + # first doing the 'keep-alive dance' (a record-only merge) that was + # required in the past. + + # Make A_COPY branch in r2, and do a few more commits to A in r3-6. + sbox.build() + wc_dir = sbox.wc_dir + expected_disk, expected_status = set_up_branch(sbox) + + # Make a change on the branch + sbox.simple_mkdir('A_COPY/NewDir') + sbox.simple_commit() + + # Sync the branch + sbox.simple_update() + simple_sync_merge(sbox, '^/A', 'A_COPY') + sbox.simple_commit() + + # Reintegrate + simple_reintegrate(sbox, '^/A_COPY', 'A') + sbox.simple_commit() + + # Try to sync the branch again, without first doing a keep-alive dance. + # If the 'keep alive' works automatically then there should be no + # conflicts. In fact in this case there should be no change at all + # because it is already up to date and the reintegrate commit that + # happened on the trunk should be ignored. + sbox.simple_update() + simple_sync_merge(sbox, '^/A', 'A_COPY') + +@SkipUnless(server_has_mergeinfo) +def reintegrate_keep_alive2(sbox): + "reintegrate with automatic keep-alive" + + # Test the ability to keep using a branch after reintegrating it. + # + # Like reintegrate_keep_alive1(), but with more complex merges involving + # branches other than the two principal branches. Specifically, changes on + # 'trunk' (A) will include changes made in a temporary side-branch + # (SIDE_BRANCHn) off A, and changes on the child branch (A_COPY) will + # include changes made in a temporary side-branch (FEATn) off A_COPY. + + # Make a branch A_COPY in r2, and then a few more commits to A, ending at r7. + # Also make a branch A_COPY_2 in r3, which we'll use as a short-term feature branch. + sbox.build() + wc_dir = sbox.wc_dir + expected_disk, expected_status = set_up_branch(sbox) + + def change_via_side_branch(branch, side_branch): + """Work on the branch BRANCH, through a short-term feature branch named + SIDE_BRANCH.""" + sbox.simple_repo_copy(branch, side_branch) + sbox.simple_update() + sbox.simple_mkdir(side_branch + '/NewVia' + side_branch) + sbox.simple_commit(side_branch) + simple_reintegrate(sbox, '^/' + side_branch, branch) + sbox.simple_commit() + + # Work on the branch + sbox.simple_mkdir('A_COPY/BranchFix1') + sbox.simple_commit() + change_via_side_branch('A_COPY', 'BRANCH_DEV1') + + # Work on trunk + sbox.simple_mkdir('A/TrunkFix1') + sbox.simple_commit() + change_via_side_branch('A', 'TRUNK_DEV1') + + # Sync the branch + sbox.simple_update() + simple_sync_merge(sbox, '^/A', 'A_COPY') + sbox.simple_commit() + + # Work on the branch + sbox.simple_mkdir('A_COPY/BranchFix2') + sbox.simple_commit() + change_via_side_branch('A_COPY', 'BRANCH_DEV2') + + # Work on trunk (after the latest sync, but still allowed by reintegrate) + sbox.simple_mkdir('A/TrunkFix2') + sbox.simple_commit() + change_via_side_branch('A', 'TRUNK_DEV2') + + # Reintegrate + sbox.simple_update() + simple_reintegrate(sbox, '^/A_COPY', 'A') + sbox.simple_commit() + + # Work on the branch + sbox.simple_mkdir('A_COPY/BranchFix3') + sbox.simple_commit() + change_via_side_branch('A_COPY', 'BRANCH_DEV3') + + # Work on trunk + sbox.simple_mkdir('A/TrunkFix3') + sbox.simple_commit() + change_via_side_branch('A', 'TRUNK_DEV3') + + # Try to sync the branch again, without first doing a keep-alive dance. + # Subversion should automatically skip the reintegrate merge revisions and + # merge the other changes (TrunkFix2, TRUNK_DEV2, TrunkFix3, TRUNK_DEV3) + # with no conflicts. + sbox.simple_update() + simple_sync_merge(sbox, '^/A', 'A_COPY') + + ######################################################################## # Run the tests @@ -2565,6 +2696,8 @@ test_list = [ None, reintegrate_creates_bogus_mergeinfo, no_source_subtree_mergeinfo, reintegrate_replaced_source, + reintegrate_keep_alive1, + reintegrate_keep_alive2, ] if __name__ == '__main__':