>
> > 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.
>
Hey Stefan,
It was only partially bypassing the work queue. The items to update the
executable bit (and read/write bits) is always done at the end of the
svn_wc_merge4() function, so that's why the test started passing. However,
you are correct - I should have been using the svn_wc__wq_build_file_install
workqueue task to copy the file across (this is what happens when a conflict
is resolved with "theirs-full").
An updated patch is attached.
[[[
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.
(svn_wc__internal_merge): Removed 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.
]]]
Cheers,
Daniel B
(who is not a C programmer by trade ...)
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,41 @@
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_skel_t *work_item;
+
+ 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 +1386,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));
@@ -1449,78 +1448,42 @@
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)
