On Wed, Feb 09, 2011 at 07:34:53AM +1000, Daniel Becroft wrote: > Hi, > > Attached is a completed patch to resolve issue 3686[1], where the executable > bit is not maintained during the merge of a binary file. > > I thought about making this change more generic, and applying it to text > files as well (there was discussion with performance of simple-case merging > a month ago on users@), but thought I'd leave that for later. I didn't want > to worry about (potential) line-endings, keywords, etc. problems.
Some comments inline. > > [[[ > 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): Removed binary-merge special case (now in libsvn_wc). > Removed merge_required variable (resulting in indentation changes). > > * subversion/libsvn_wc/merge.c > (merge_binary_file): Added dry_run parameter. Add the special case merging > of binary files. The log messages doesn't mention the caller of merge_binary_file() which was also changed. > > * subversion/tests/cmdline/merge_tests.py > (merge_change_to_file_with_executable): Remove @XFail decorator. > ]]] > > [1] > http://subversion.tigris.org/ds/viewMessage.do?dsForumId=463&dsMessageId=2635024 > > Cheers, > Daniel B. > > --- > Daniel Becroft > Index: subversion/tests/cmdline/merge_tests.py > =================================================================== > --- subversion/tests/cmdline/merge_tests.py (revision 1068136) > +++ subversion/tests/cmdline/merge_tests.py (working copy) > @@ -16469,7 +16469,6 @@ > #---------------------------------------------------------------------- > # 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 1068136) > +++ subversion/libsvn_wc/merge.c (working copy) > @@ -1094,6 +1094,7 @@ > 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,31 @@ > 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_io_file_move(right_abspath, target_abspath, > + scratch_pool)); Isn't calling svn_io_file_move() directly still bypassing the work queue? Shouldn't this code queue a task instead? > + *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 +1376,23 @@ > > 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 1068136) > +++ subversion/libsvn_client/merge.c (working copy) > @@ -1300,7 +1300,6 @@ > { > 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)); > @@ -1452,75 +1451,38 @@ > 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. > + /* 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; The above will fail to compile with C89 compilers. Variables must be declared at the beginning of a scope. > > - 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; > - } > - } > + 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; > > - 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; > + 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)); > > - 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)
