[ disclaimer: I'm not familiar with wc internals yet; sorry; hope I'll
learn something while reviewing this ]
Daniel Becroft wrote on Thu, Feb 10, 2011 at 07:21:30 +1000:
> >
> 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).
>
Well written.
> * 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).
>
Two spaces before (. [I guess it was right when you sent it, since your
mail mis-wrapped the above paragraphs as well]
> * 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.
>
Use present tense please.
> * 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/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,
It would have been easier to review the patch if you'd attached
a version generated with 'svn diff -x-pw'.
> Index: subversion/libsvn_wc/merge.c
> ===================================================================
> --- subversion/libsvn_wc/merge.c (revision 1069789)
> +++ subversion/libsvn_wc/merge.c (working copy)
> @@ -1094,6 +1094,7 @@ merge_binary_file(svn_skel_t **work_items,
> 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,
> @@ -1118,6 +1120,33 @@ merge_binary_file(svn_skel_t **work_items,
>
> 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. */
The comment in libsvn_client mentioned two special case, what happened
to the other one? Does the existing wc code already handle it? (I'd be
surprised)
- 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).
> + 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;
> +
subversion/libsvn_wc/merge.c: In function ‘merge_binary_file’:
subversion/libsvn_wc/merge.c:1135: warning: declaration of ‘work_item’ shadows
a previous local
subversion/libsvn_wc/merge.c:1114: warning: shadowed declaration is here
> + SVN_ERR(svn_wc__wq_build_file_install(&work_item,
> + db, target_abspath,
> + right_abspath,
> + FALSE /* use_commit_times */,
> + FALSE /* record_fileinfo */,
About RECORD_FILEINFO:
* Why are you passing FALSE? (I see that other calls do this too, so
this is for my education mainly)) A quick scan makes me expect that
parameter to control the mtime/size caches for modification detection.
* In update_editor.c there are two places that pass NULL (not FALSE) for
that, and have above them comment that says that "If %s, we want to
pass FALSE". What is going on there?
(this is to the list, not specifically at Daniel)
> + result_pool, scratch_pool));
> + *work_items = svn_wc__wq_merge(*work_items, work_item,
> result_pool);
Don't you need to convert the next svn_wc__wq_build_file_install() to
use this 'build-then-merge' approach too? Otherwise, won't it overwrite
your WORK_ITEMS?
> + }
> +
> + *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,10 +1386,6 @@ svn_wc__internal_merge(svn_skel_t **work_items,
>
> 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,
> @@ -1370,6 +1395,7 @@ svn_wc__internal_merge(svn_skel_t **work_items,
> left_label,
> right_label,
> target_label,
> + dry_run,
> left_version,
> right_version,
> detranslated_target_abspath,
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 1069789)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -1300,7 +1300,6 @@ merge_file_changed(const char *local_dir_abspath,
> {
> 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,45 +1448,7 @@ merge_file_changed(const char *local_dir_abspath,
> if (older_abspath)
> {
> svn_boolean_t has_local_mods;
> - 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.
> -
> - 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;
> - }
> - }
> -
> - 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 */
> @@ -1502,6 +1463,9 @@ merge_file_changed(const char *local_dir_abspath,
> 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));
> +
You only need HAS_LOCAL_MODS now when CONTENT_STATE is set. Shouldn't
you skip this call when CONTENT_STATE is NULL then? (It may perform
stat() or read().)
> 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;
> @@ -1519,7 +1483,6 @@ merge_file_changed(const char *local_dir_abspath,
> merge_b->ctx->cancel_func,
> merge_b->ctx->cancel_baton,
> subpool));
> - }
>
> if (content_state)
> {
