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
> -_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?
> + *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)
Received on 2011-02-08 22:47:18 CET