On Wed, Feb 9, 2011 at 7:46 AM, Stefan Sperling <stsp_at_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
> > -_at_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)
>
>
Received on 2011-02-08 22:55:46 CET