On Sun, Feb 13, 2011 at 2:15 AM, Daniel Shahaf <d...@daniel.shahaf.name>wrote:
> Daniel Becroft wrote on Sat, Feb 12, 2011 at 08:37:12 +1000: > > On Sat, Feb 12, 2011 at 7:31 AM, Daniel Shahaf <d...@daniel.shahaf.name > >wrote: > > > > > Daniel Becroft wrote on Sat, Feb 12, 2011 at 06:27:31 +1000: > > > > On Fri, Feb 11, 2011 at 11:26 PM, Daniel Shahaf < > d...@daniel.shahaf.name > > > >wrote: > > > > > Daniel Becroft wrote on Thu, Feb 10, 2011 at 07:21:30 +1000: > > > > > > @@ -1118,6 +1120,33 @@ merge_binary_file(svn_skel_t **work_items, > > > > > > + /* Attempt to merge the binary file. At the moment, we can > only > > > > > > + handle the special case: if the LEFT side of the merge is > equal > > > > > > + to WORKING, then we can copy RIGHT directly. */ > > > > > > > > > > The comment in libsvn_client mentioned two special case, what > happened > > > > > to the other one? Does the existing wc code already handle it? > (I'd be > > > > > surprised) > > > > > > > > > > - Alternately, if the 'left' side of the merge doesn't > exist > > > in > > > > > - the repository, and the 'right' side of the merge is > > > > > - identical to the WC, pretend we did the merge (a no-op). > > > > > > > > > > > > > I've been trying to think of a valid scenario for this to occur, but > I > > > can't > > > > seem to think of one. There's a comment further up: > > > > > > > > > > Concocted: > > > % svn add foo.bin > > > % svn ci -m r5 > > > % svn rm ^/foo.bin -m r6 > > > % svnmucc -m r7 put foo.bin ^/foo.bin > > > % svn merge -c7 ^/ ./ > > > > > > > Maybe it's early, and the coffee hasn't kicked in yet, but wouldn't (and > > shouldn't) this give a tree-conflict? foo.bin@7 and foo.bin in the WC > are > > two different nodes (with the same name). > > > > I hadn't considered that this might be a tree conflict (an add of an > already-existing file with the same contents). My point was not the > tree conflict but that the scenario described in the "Alternately" can > happen. > I've looked through this several times now to try and be sure that I'm not missing anything. Previously, the "Special case" block handled both when to merge the binary file, as well as how. Now, the "when" logic is now the same logic as text files, and the how has been moved into libsvn_wc (rather than libsvn_client). I've attached the file version of the patch. > > > /* Other easy outs: if the merge target isn't under version > > > > control, or is just missing from disk, fogettaboutit. There's > no > > > > way svn_wc_merge4() can do the merge. */ > > > > > > > > This should apply to all situations (binary and text files), so I > think > > > this > > > > second "special case" is redundant. > > > > > > > > > > Not sure. If the merge-left does not exist, and the local file doesn't > > > exist either, then the situation is 'compatible' and can be merged... > > > > > > > Isn't this just an incoming add (which is handled by a different > function)? > > > > I don't know the code very well. If you're saying that at the point of > the comment we know that the merge-left exists, then I agree that it > makes sense to flag a conflict if the merge target is non-existent. > Yep, this is already done as part of the standard merge logic (ie why doesn't left exist? It is an incoming add, or already deleted in the WC, etc). > > > > > I can submit a follow-up patch that fixes this as well, if necessary. > > > > > > That would be great, assuming that the FALSE *really* should be changed > > > to TRUE. (I haven't investigated that myself.) > > > > > > > Not sure what is is in reference to. I was thinking of just putting a if > > (content_state) before the local modifications check (again, coffee may > not > > have kicked in yet). > > > > I was trying to say: "It would be great if you could look further into > that matter, but I haven't done so myself yet so I don't know if a patch > is required or the current code is correct." > Gotcha (I just couldn't figure out where the TRUE/FALSE reference came from), but anywho ... > In other words, thanks for looking into this, but I don't know myself > whether a follow-up patch would be necessary. > > > Cheers, > > Daniel^2. > > HTH Yep. I've attached the (final) version of the patch (generated with the C functions this time), and the log message is below. [[[ Fix issue #3686 - executable bit not set during merge. The cause was the special case in libsvn_client, which bypassed the use of the workqueue. This logic has now been moved into libsvn_wc. Additionally, this change allows the status of binary files (during a dry-run merge) to be reported correctly (previously, all binary files were reported as conflicted). * subversion/libsvn_client/merge.c (merge_file_changed): Remove binary-merge special case (now in libsvn_wc). Remove merge_required variable (resulting in indentation changes). * subversion/libsvn_wc/merge.c (merge_binary_file): Add dry_run parameter. Add the special case merging of binary files. (svn_wc__internal_merge): Remove dry_run check for binary files, and pass to merge_binary_file instead. * subversion/tests/cmdline/merge_tests.py (merge_change_to_file_with_executable): Remove @XFail decorator. ]]] Let me know if there's anything else I need to fix up. Cheers, Daniel B.
Index: subversion/tests/cmdline/merge_tests.py =================================================================== --- subversion/tests/cmdline/merge_tests.py (revision 1072928) +++ subversion/tests/cmdline/merge_tests.py (working copy) @@ -16469,7 +16469,6 @@ def subtree_merges_inherit_invalid_working_mergein #---------------------------------------------------------------------- # Test for issue #3686 'executable flag not correctly set on merge' # See http://subversion.tigris.org/issues/show_bug.cgi?id=3686 -@XFail() @Issue(3686) @SkipUnless(svntest.main.is_posix_os) def merge_change_to_file_with_executable(sbox): Index: subversion/libsvn_wc/merge.c =================================================================== --- subversion/libsvn_wc/merge.c (revision 1072928) +++ subversion/libsvn_wc/merge.c (working copy) @@ -1094,6 +1094,7 @@ merge_binary_file(svn_skel_t **work_items, const char *left_label, const char *right_label, const char *target_label, + svn_boolean_t dry_run, const svn_wc_conflict_version_t *left_version, const svn_wc_conflict_version_t *right_version, const char *detranslated_target_abspath, @@ -1111,13 +1112,39 @@ merge_binary_file(svn_skel_t **work_items, const char *merge_dirpath, *merge_filename; const char *conflict_wrk; svn_skel_t *work_item; + svn_boolean_t same_contents = FALSE; SVN_ERR_ASSERT(svn_dirent_is_absolute(target_abspath)); *work_items = NULL; svn_dirent_split(&merge_dirpath, &merge_filename, target_abspath, pool); + + /* Attempt to merge the binary file. At the moment, we can only + handle the special case: if the LEFT side of the merge is equal + to WORKING, then we can copy RIGHT directly. */ + SVN_ERR(svn_io_files_contents_same_p(&same_contents, + left_abspath, + target_abspath, + scratch_pool)); + if (same_contents) + { + if (!dry_run) + { + SVN_ERR(svn_wc__wq_build_file_install(&work_item, + db, target_abspath, + right_abspath, + FALSE /* use_commit_times */, + FALSE /* record_fileinfo */, + result_pool, scratch_pool)); + *work_items = svn_wc__wq_merge(*work_items, work_item, result_pool); + } + + *merge_outcome = svn_wc_merge_merged; + return SVN_NO_ERROR; + } + /* Give the conflict resolution callback a chance to clean up the conflict before we mark the file 'conflicted' */ if (conflict_func) @@ -1357,26 +1384,23 @@ svn_wc__internal_merge(svn_skel_t **work_items, if (is_binary) { - if (dry_run) - /* in dry-run mode, binary files always conflict */ - *merge_outcome = svn_wc_merge_conflict; - else - SVN_ERR(merge_binary_file(work_items, - merge_outcome, - db, - left_abspath, - right_abspath, - target_abspath, - left_label, - right_label, - target_label, - left_version, - right_version, - detranslated_target_abspath, - mimeprop, - conflict_func, - conflict_baton, - result_pool, scratch_pool)); + SVN_ERR(merge_binary_file(work_items, + merge_outcome, + db, + left_abspath, + right_abspath, + target_abspath, + left_label, + right_label, + target_label, + dry_run, + left_version, + right_version, + detranslated_target_abspath, + mimeprop, + conflict_func, + conflict_baton, + result_pool, scratch_pool)); } else { Index: subversion/libsvn_client/merge.c =================================================================== --- subversion/libsvn_client/merge.c (revision 1072928) +++ subversion/libsvn_client/merge.c (working copy) @@ -1300,7 +1300,6 @@ merge_file_changed(const char *local_dir_abspath, { merge_cmd_baton_t *merge_b = baton; apr_pool_t *subpool = svn_pool_create(merge_b->pool); - svn_boolean_t merge_required = TRUE; enum svn_wc_merge_outcome_t merge_outcome; SVN_ERR_ASSERT(mine_abspath && svn_dirent_is_absolute(mine_abspath)); @@ -1449,78 +1448,42 @@ merge_file_changed(const char *local_dir_abspath, if (older_abspath) { svn_boolean_t has_local_mods; + + /* xgettext: the '.working', '.merge-left.r%ld' and + '.merge-right.r%ld' strings are used to tag onto a file + name in case of a merge conflict */ + const char *target_label = _(".working"); + const char *left_label = apr_psprintf(subpool, + _(".merge-left.r%ld"), + older_rev); + const char *right_label = apr_psprintf(subpool, + _(".merge-right.r%ld"), + yours_rev); + conflict_resolver_baton_t conflict_baton = { 0 }; + const svn_wc_conflict_version_t *left; + const svn_wc_conflict_version_t *right; + SVN_ERR(svn_wc_text_modified_p2(&has_local_mods, merge_b->ctx->wc_ctx, mine_abspath, FALSE, subpool)); - /* Special case: if a binary file's working file is - exactly identical to the 'left' side of the merge, then don't - allow svn_wc_merge to produce a conflict. Instead, just - overwrite the working file with the 'right' side of the - merge. Why'd we check for local mods above? Because we want - to do a different notification depending on whether or not - the file was locally modified. + conflict_baton.wrapped_func = merge_b->ctx->conflict_func; + conflict_baton.wrapped_baton = merge_b->ctx->conflict_baton; + conflict_baton.conflicted_paths = &merge_b->conflicted_paths; + conflict_baton.pool = merge_b->pool; - Alternately, if the 'left' side of the merge doesn't exist in - the repository, and the 'right' side of the merge is - identical to the WC, pretend we did the merge (a no-op). */ - if ((mimetype1 && svn_mime_type_is_binary(mimetype1)) - || (mimetype2 && svn_mime_type_is_binary(mimetype2))) - { - /* For adds, the 'left' side of the merge doesn't exist. */ - svn_boolean_t older_revision_exists = - !merge_b->add_necessitated_merge; - svn_boolean_t same_contents; - SVN_ERR(svn_io_files_contents_same_p(&same_contents, - (older_revision_exists ? - older_abspath : yours_abspath), - mine_abspath, subpool)); - if (same_contents) - { - if (older_revision_exists && !merge_b->dry_run) - { - SVN_ERR(svn_io_file_move(yours_abspath, mine_abspath, - subpool)); - } - merge_outcome = svn_wc_merge_merged; - merge_required = FALSE; - } - } + SVN_ERR(make_conflict_versions(&left, &right, mine_abspath, + svn_node_file, merge_b)); + SVN_ERR(svn_wc_merge4(&merge_outcome, merge_b->ctx->wc_ctx, + older_abspath, yours_abspath, mine_abspath, + left_label, right_label, target_label, + left, right, + merge_b->dry_run, merge_b->diff3_cmd, + merge_b->merge_options, prop_changes, + conflict_resolver, &conflict_baton, + merge_b->ctx->cancel_func, + merge_b->ctx->cancel_baton, + subpool)); - if (merge_required) - { - /* xgettext: the '.working', '.merge-left.r%ld' and - '.merge-right.r%ld' strings are used to tag onto a file - name in case of a merge conflict */ - const char *target_label = _(".working"); - const char *left_label = apr_psprintf(subpool, - _(".merge-left.r%ld"), - older_rev); - const char *right_label = apr_psprintf(subpool, - _(".merge-right.r%ld"), - yours_rev); - conflict_resolver_baton_t conflict_baton = { 0 }; - const svn_wc_conflict_version_t *left; - const svn_wc_conflict_version_t *right; - - conflict_baton.wrapped_func = merge_b->ctx->conflict_func; - conflict_baton.wrapped_baton = merge_b->ctx->conflict_baton; - conflict_baton.conflicted_paths = &merge_b->conflicted_paths; - conflict_baton.pool = merge_b->pool; - - SVN_ERR(make_conflict_versions(&left, &right, mine_abspath, - svn_node_file, merge_b)); - SVN_ERR(svn_wc_merge4(&merge_outcome, merge_b->ctx->wc_ctx, - older_abspath, yours_abspath, mine_abspath, - left_label, right_label, target_label, - left, right, - merge_b->dry_run, merge_b->diff3_cmd, - merge_b->merge_options, prop_changes, - conflict_resolver, &conflict_baton, - merge_b->ctx->cancel_func, - merge_b->ctx->cancel_baton, - subpool)); - } - if (content_state) { if (merge_outcome == svn_wc_merge_conflict)
