When a merge deletes a file, check whether the file content is the same, delete it if so, and skip it if not. This improves the decision whether to skip, which was previously based only on whether the file had uncommitted modifications. The comparison is not yet implemented for directories. This changes the meaning of the "force" flag to all of the svn_client "merge" APIs. ### Incomplete. See '###'. merge_tests.py 3 61 77 93 are failing. * subversion/libsvn_client/merge.c (files_contents_same_p): New function. (merge_file_deleted): Ignore the "force" flag; instead, delete the file if it's identical and otherwise skip it. (single_file_merge_get_file): Improve the doc string. (do_file_merge): Fix the call to merge_file_deleted() so it can see the file being deleted. This is required by its API although it didn't previously care. * subversion/tests/cmdline/merge_tests.py (delete_file_and_dir): Update the expected results. (svn_mkdir, svn_mkfile, svn_copy, svn_delete, svn_commit, svn_move): New helper functions. (del_identical_file, del_sched_add_hist_file, del_differing_file): New test functions. (test_list): Add the new tests. * subversion/include/svn_client.h (svn_client_merge3): Update documentation for the "force" parameter accordingly. Index: subversion/include/svn_client.h =================================================================== --- subversion/include/svn_client.h (revision 32369) +++ subversion/include/svn_client.h (working copy) @@ -2492,10 +2492,17 @@ svn_client_diff_summarize_peg(const char * and the addition of another, but if this flag is TRUE, unrelated * items will be diffed as if they were related. * + * ### OLD: * If @a force is not set and the merge involves deleting locally modified or * unversioned items the operation will fail. If @a force is set such items * will be deleted. * + * ### NEW: + * If @a force is false and the merge involves deleting a file whose + * content differs from the source-left version, or a locally modified + * directory, or an unversioned item, then the operation will fail. If + * @a force is true then all such items will be deleted. + * * @a merge_options (an array of const char *), if non-NULL, * is used to pass additional command line arguments to the merge * processes (internal or external). @see Index: subversion/libsvn_client/merge.c =================================================================== --- subversion/libsvn_client/merge.c (revision 32369) +++ subversion/libsvn_client/merge.c (working copy) @@ -49,6 +49,7 @@ #include "private/svn_wc_private.h" #include "private/svn_mergeinfo_private.h" +#include "private/svn_debug_private.h" #include "svn_private_config.h" @@ -974,6 +975,24 @@ merge_file_added(svn_wc_adm_access_t *ad return SVN_NO_ERROR; } +/* Compare the contents of the files OLDER and MINE. Set *SAME to true if + * they are identical, false otherwise. + * ### This is presently a raw comparison: no translation of keywords or EOL style. + */ +static svn_error_t * +files_contents_same_p(svn_boolean_t *same, + const char *older, + const char *mine, + apr_pool_t *pool) +{ + SVN_ERR_ASSERT(older); + SVN_ERR_ASSERT(mine); + + SVN_ERR(svn_io_files_contents_same_p(same, older, mine, pool)); + + return SVN_NO_ERROR; +} + /* An svn_wc_diff_callbacks3_t function. */ static svn_error_t * merge_file_deleted(svn_wc_adm_access_t *adm_access, @@ -1005,27 +1024,54 @@ merge_file_deleted(svn_wc_adm_access_t * return SVN_NO_ERROR; } + dbg_printf("## merge_file_deleted(mine ='%s'\n" + " older ='%s'\n" + " yours ='%s'\n" + " target='%s'\n" + " url ='%s')\n", + mine, older, yours, merge_b->target, merge_b->url); + SVN_ERR(svn_io_check_path(mine, &kind, subpool)); switch (kind) { + svn_boolean_t same; + case svn_node_file: - svn_path_split(mine, &parent_path, NULL, subpool); - SVN_ERR(svn_wc_adm_retrieve(&parent_access, adm_access, parent_path, - subpool)); - /* Passing NULL for the notify_func and notify_baton because - repos_diff.c:delete_entry() will do it for us. */ - err = svn_client__wc_delete(mine, parent_access, merge_b->force, - merge_b->dry_run, FALSE, NULL, NULL, - merge_b->ctx, subpool); - if (err) + /* ### Crude test. Should use WC entry to quickly determine it's + * identical in easy cases e.g. where base-rev==old and not locally + * modified. Must test properties as well. Must account for keywords and + * EOL style. To do all of this, "older" and "mine" must be repos or WC + * references, not just names of temporary files. */ + SVN_ERR(files_contents_same_p(&same, older, mine, subpool)); + + /* If the files are identical, attempt deletion */ + if (same || merge_b->force) { - if (state) - *state = svn_wc_notify_state_obstructed; - svn_error_clear(err); + svn_path_split(mine, &parent_path, NULL, subpool); + SVN_ERR(svn_wc_adm_retrieve(&parent_access, adm_access, parent_path, + subpool)); + + /* Passing NULL for the notify_func and notify_baton because + repos_diff.c:delete_entry() will do it for us. */ + err = svn_client__wc_delete(mine, parent_access, TRUE, + merge_b->dry_run, FALSE, NULL, NULL, + merge_b->ctx, subpool); + if (err) + { + if (state) + *state = svn_wc_notify_state_obstructed; + svn_error_clear(err); + } + else if (state) + { + *state = svn_wc_notify_state_changed; + } } - else if (state) + else { - *state = svn_wc_notify_state_changed; + /* The files differ, so skip instead of deleting */ + if (state) + *state = svn_wc_notify_state_obstructed; } break; case svn_node_dir: @@ -3148,10 +3194,11 @@ mark_mergeinfo_as_inheritable_for_a_rang } -/* Get REVISION of the file at URL. SOURCE is a path that refers to that +/* Get REVISION of the file at URL. WC_TARGET is a path that refers to that file's entry in the working copy, or NULL if we don't have one. Return in *FILENAME the name of a file containing the file contents, in *PROPS a hash containing the properties and in *REV the revision. All allocation occurs +### REV description is wrong in POOL. */ static svn_error_t * single_file_merge_get_file(const char **filename, @@ -4662,8 +4709,8 @@ do_file_merge(const char *url1, SVN_ERR(merge_file_deleted(adm_access, &text_state, target_wcpath, - NULL, - NULL, + tmpfile1, + tmpfile2, mimetype1, mimetype2, props1, merge_b)); Index: subversion/tests/cmdline/merge_tests.py =================================================================== --- subversion/tests/cmdline/merge_tests.py (revision 32369) +++ subversion/tests/cmdline/merge_tests.py (working copy) @@ -618,9 +618,9 @@ def delete_file_and_dir(sbox): }) expected_status2.tweak(wc_rev=2) expected_skip = wc.State(B2_path, { - 'lambda' : Item(), 'E' : Item(), }) + ### no longer expect skip lambda: it's identical (?) svntest.actions.run_and_verify_merge(B2_path, '2', '3', B_url, expected_output, expected_disk, @@ -12617,6 +12617,147 @@ def commit_to_subtree_added_by_merge(sbo svntest.actions.run_and_verify_svn(None, ["At revision 5.\n"], [], 'up', wc_dir) + +def svn_mkdir(path): + "Make and add a directory with some default props." + svntest.actions.run_and_verify_svn(None, None, [], 'mkdir', '--parents', path) + +def svn_mkfile(path): + "Make and add a file with some default content." + dirname, filename = os.path.split(path) + svntest.main.file_write(path, "This is the file '" + filename + "'.\n" + + "Original second line.\n") + svntest.actions.run_and_verify_svn(None, None, [], 'add', path) + svntest.actions.run_and_verify_svn(None, None, [], 'propset', + 'p1', 'v1', path) + #expected_output = wc.State(wc_dir, + # {path : Item(verb='Adding')}) + #wc_status.add({path : Item(status=' ', wc_rev=3)}) + #svntest.actions.run_and_verify_commit(wc_dir, expected_output, + # wc_status, None, wc_dir) + +def svn_copy(path1, path2): + "Copy a WC path locally." + svntest.actions.run_and_verify_svn(None, None, [], 'copy', '--parents', path1, path2) + +def svn_delete(path): + "Delete a WC path locally." + svntest.actions.run_and_verify_svn(None, None, [], 'delete', path) + +def svn_commit(path): + "Commit a WC path and return the new revision number." + svntest.actions.run_and_verify_svn(None, svntest.verify.AnyOutput, [], + 'commit', '-m', '', path) + svn_commit.repo_rev += 1 + print "## repo_rev is now", svn_commit.repo_rev + return svn_commit.repo_rev + +def svn_merge(src_change_num, source, target, exp_out=svntest.verify.AnyOutput): + "Merge a single change from path 'source' to path 'target'" + svntest.actions.run_and_verify_svn(None, exp_out, [], + 'merge', '-c', src_change_num, + source, target) + +def del_identical_file(sbox): + "merge tries to delete a file of identical content" + + # Setup a standard greek tree in r1. + sbox.build() + svn_commit.repo_rev = 1 + + saved_cwd = os.getcwd() + os.chdir(sbox.wc_dir) + + source_d = 'source' + target_d = 'target' + source_file = os.path.join(source_d, 'file') + target_file = os.path.join(target_d, 'file') + + # Make an identical copy, and merge a deletion to it. + svn_mkdir(source_d) + svn_mkfile(source_file) + s_rev = svn_commit('.') + svn_copy(source_file, target_file) + svn_delete(source_file) + s_rev = svn_commit('.') + svn_merge(s_rev, source_d, target_d, '--- Merging|D ') + # should be deleted quietly + +def del_sched_add_hist_file(sbox): + "merge tries to delete identical sched-add file" + + # Setup a standard greek tree in r1. + sbox.build() + svn_commit.repo_rev = 1 + + saved_cwd = os.getcwd() + os.chdir(sbox.wc_dir) + + source_d = 'source' + target_d = 'target' + source_file = os.path.join(source_d, 'file') + target_file = os.path.join(target_d, 'file') + + # Merge a creation, and delete by reverse-merging into uncommitted WC. + svn_mkdir(source_d) + svn_copy(source_d, target_d) + s_rev = svn_commit('.') + svn_mkfile(source_file) + s_rev = svn_commit('.') + svn_merge(s_rev, source_d, target_d, '--- Merging|A ') + svn_merge(-s_rev, source_d, target_d, '--- Reverse-merging|D ') + # should be deleted quietly + + os.chdir(saved_cwd) + +def del_differing_file(sbox): + "merge tries to delete a file of different content" + + # Setup a standard greek tree in r1. + sbox.build() + svn_commit.repo_rev = 1 + + saved_cwd = os.getcwd() + os.chdir(sbox.wc_dir) + + source_d = 'source' + target_d = 'target' + source_file = os.path.join(source_d, 'file') + target_file = os.path.join(target_d, 'file') + + # Copy a file, text-modify it, and merge a deletion to it. + svn_mkdir(source_d) + svn_mkfile(source_file) + s_rev = svn_commit('.') + svn_copy(source_file, target_file) + svntest.main.file_append(target_file, "An extra line in the target.\n") + svn_delete(source_file) + s_rev = svn_commit(source_d) + svn_merge(s_rev, source_d, target_d, 'Skipped') + # should complain and "skip" it + + source_d = 'source2' + target_d = 'target2' + source_file = os.path.join(source_d, 'file') + target_file = os.path.join(target_d, 'file') + + # Copy a file, text-modify it, commit, and merge a deletion to it. + svn_mkdir(source_d) + svn_mkfile(source_file) + s_rev = svn_commit('.') + svn_copy(source_file, target_file) + svntest.main.file_append(target_file, "An extra line in the target.\n") + svn_delete(source_file) + s_rev = svn_commit('.') + svn_merge(s_rev, source_d, target_d, 'Skipped') + # should complain and "skip" it + + # Make a prop-modified copy, and merge a deletion to it. + # ... + + os.chdir(saved_cwd) + + ######################################################################## # Run the tests @@ -12791,6 +12932,9 @@ test_list = [ None, SkipUnless(subtrees_with_empty_mergeinfo, server_has_mergeinfo), SkipUnless(commit_to_subtree_added_by_merge, svntest.main.is_ra_type_dav), + del_identical_file, + del_sched_add_hist_file, + del_differing_file, ] if __name__ == '__main__':