On Fri, Feb 11, 2011 at 11:26 PM, Daniel Shahaf <d...@daniel.shahaf.name>wrote:
> [ 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'.
>
I wasn't aware of these options. Thanks.
> > 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).
>
I've been trying to think of a valid scenario for this to occur, but I can't
seem to think of one. There's a comment further up:
/* Other easy outs: if the merge target isn't under version
control, or is just missing from disk, fogettaboutit. There's no
way svn_wc_merge4() can do the merge. */
This should apply to all situations (binary and text files), so I think this
second "special case" is redundant.
> > + 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
>
Oops, fixed.
> > + 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.
>
I copied this directly from the call further down, where a file is copied
depending on which version to use in a conflict. Even if "theirs" is used,
we still pass in FALSE for record_fileinfo.
The RECORD_FILEINFO flag controls whether the file's information should be
recorded into the wc.db. It seems that we only pass TRUE to this if we are
installing a fresh file. It makes sense to pass in FALSE during a merge,
otherwise the file won't be seen as modified.
PS: The two calls in update_editor.c don't pass in NULL, but pass in a value
based on comparing against NULL.
record_fileinfo = new_contents == NULL;
record_fileinfo = install_from == NULL;
* 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?
>
Which svn_wc__wq_build_file_install() call do you mean?
We return SVN_NO_ERROR immediately after building adding this workqueue
item, so the other one for the binary file conflict resolution will not get
called.
> > + }
> > +
> > + *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().)
>
Agreed, however this was always the case. The only change I made to this
section was indentation
due to removing the surrounding if (merge_required) { } clause. I didn't
want to make other changes
that would be clouded.
I can submit a follow-up patch that fixes this as well, if necessary.
> > 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)
> > {
>
Thanks for the review, Daniel.
Cheers,
Daniel^2.
