On Wed, Feb 9, 2011 at 7:46 AM, Stefan Sperling <s...@elego.de> wrote:
> 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.> > Oops, will fix. > * 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? > Hmm ... I'll look into that. The test went from XFAIL to XPASS, and I was working off the "minimum required to get test to pass" mantra. > + *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. > Thanks. These variables were all wrapped in the if (merge_required) block, and I simply removed that and adjusted indentation. > > > > - 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) > >
