[PATCH] #2403: Incorrect merge of binary file when already existing

Well, this wasn't exactly what I had in mind after talking with Ben,
but given the way the code is structued for creation of an empty file
(for a file which doesn't exist in the repository) to be used as the
'left' side of the merge (libsvn_client/repos_diff.c:add_file() calls
get_empty_file()), recording some context in the baton seemed like a
simpler way to go.

Alternate implementation suggestions welcome.

[[[
Fix issue #2403, incorrect merge of a binary file which already exists
in the repository and working copy. Problem scenario:
1. Perform a 3-way merge into your WC (using two URLs), where the
left/older side doesn't contain the file being merged, and the
right/newer side is identical to the content in your WC.
2. Merge does not report a conflict.
3. WC is left in a conflicted state.

* subversion/libsvn_client/diff.c
  (struct merge_cmd_baton): Add new add_necessitated_merge flag
   indicating that invocation of the merge_file_added callback
   required delegation to the merge_file_changed callback. This is
   important for detecting whether the left side of a 3-way merge
   actually exists, or is only present for implementation purposes.

  (merge_file_changed): 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).

  (merge_file_added): Set add_necessitated_merge to TRUE before
   calling merge_file_changed().

  (svn_client_merge2, svn_client_merge_peg2): Initialize
   add_necessitated_merge to FALSE.
]]]

Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 19623)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -691,6 +691,10 @@
   const svn_opt_revision_t *revision; /* Revision of second URL in the merge */
   svn_client_ctx_t *ctx;
 
+ /* Whether invocation of the merge_file_added callback required
+ delegation to the merge_file_changed callback. */
+ svn_boolean_t add_necessitated_merge;
+
   /* The diff3_cmd in ctx->config, if any, else null. We could just
      extract this as needed, but since more than one caller uses it,
      we just set it up when this baton is created. */
@@ -828,22 +833,31 @@
       /* Special case: if a binary file isn't locally modified, and 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. */
+ overwrite the working file with the 'right' side of the merge.
+
+ 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 ((! has_local_mods)
           && ((mimetype1 && svn_mime_type_is_binary(mimetype1))
               || (mimetype2 && svn_mime_type_is_binary(mimetype2))))
         {
+ svn_boolean_t older_revision_exists =
+ !merge_b->add_necessitated_merge;
           svn_boolean_t same_contents;
+ const char *comparison_path = older_revision_exists ? older : yours;
+
           SVN_ERR(svn_io_files_contents_same_p(&same_contents,
- older, mine, subpool));
+ comparison_path,
+ mine, subpool));
           if (same_contents)
             {
- if (! merge_b->dry_run)
- SVN_ERR(svn_io_file_rename(yours, mine, subpool));
+ if (older_revision_exists && !merge_b->dry_run)
+ SVN_ERR(svn_io_file_rename(yours, mine, subpool));
               merge_outcome = svn_wc_merge_merged;
               merge_required = FALSE;
             }
- }
+ }
 
       if (merge_required)
         {
@@ -1002,6 +1016,8 @@
           }
         else
           {
+ /* Indicate that we're merging as the result of an add. */
+ merge_b->add_necessitated_merge = TRUE;
             SVN_ERR(merge_file_changed(adm_access, content_state, prop_state,
                                        mine, older, yours,
                                        rev1, rev2,
@@ -2713,6 +2729,7 @@
   merge_cmd_baton.revision = revision2;
   merge_cmd_baton.path = path2;
   merge_cmd_baton.added_path = NULL;
+ merge_cmd_baton.add_necessitated_merge = FALSE;
   merge_cmd_baton.ctx = ctx;
   merge_cmd_baton.pool = pool;
 
@@ -2841,6 +2858,7 @@
   merge_cmd_baton.revision = revision2;
   merge_cmd_baton.path = path;
   merge_cmd_baton.added_path = NULL;
+ merge_cmd_baton.add_necessitated_merge = FALSE;
   merge_cmd_baton.ctx = ctx;
   merge_cmd_baton.pool = pool;
 

On Thu, 11 May 2006, Daniel Rall wrote:

> I talked this over on IRC with Ben Sussman, and he recommended
> avoiding creation of an empty file for a non-existent resource which
> is part of a 3-way merge (possibly only for the "involves binary
> files" special case we're dealing with for this issue), then the
> refactoring the libsvn_client/diff.c to deal with that as necessary.
> I tend to favor this strategy too.
>
> An alternate strategy would be to modify merge_file_changed() to
> accept some additional context (e.g. a new parameter) indicating
> whether the files involved in the merge actually exist.
>
>
>
> On Wed, 10 May 2006, Daniel Rall wrote:
>
> > http://subversion.tigris.org/issues/show_bug.cgi?id=2403
> > http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=104672
> >
> > Hypothesis: When doing a 3-way merge of a binary file where the left
> > (older) side of the merge doesn't exist, and the right (yours) side of
> > the merge is the same as the file in the WC (mine), we're attempting
> > to compare the left side of the merge to the working copy, rather than
> > detecting that the left side doesn't exist (as opposed to it just
> > being an empty file), and comparing the right side to the WC instead.
> >
> >
> > Example of what's effectively a no-op:
> >
> > svn merge file:///repos/trunk file:///repos/branches/B
> >
> > older, r0 -----------> yours, r4
> > (does not exist) calc diff "New file"
> >
> > |
> > | apply
> > | diff
> > V
> >
> > mine, r4+
> > "New file"
> >
> >
> > I think we need something like the patch below (which passes
> > merge_tests.py). This implementation has the problem that you can't
> > detect the difference between an empty right (older) side of the merge
> > except for revision 0 (an empty repository). Assuming that this
> > approach seems correct to everyone else, the client code could be
> > changed to either a) not write an empty file for files which don't
> > exist in the repos, or b) pass some flag indicating that an empty file
> > also signifies that a file doesn't exist in the repository.
> >
> > Thoughts?
> >
> >
> > Index: subversion/libsvn_client/diff.c
> > ===================================================================
> > --- subversion/libsvn_client/diff.c (revision 19605)
> > +++ subversion/libsvn_client/diff.c (working copy)
> > @@ -828,22 +828,35 @@
> > /* Special case: if a binary file isn't locally modified, and 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. */
> > + overwrite the working file with the 'right' side of the merge.
> > +
> > + 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. */
> > if ((! has_local_mods)
> > && ((mimetype1 && svn_mime_type_is_binary(mimetype1))
> > || (mimetype2 && svn_mime_type_is_binary(mimetype2))))
> > {
> > + /* ### FIXME: What about nodes deleted after rev 0? Handle
> > + ### detection of non-existent files higher up the stack
> > + ### in libsvn_client, and don't write an empty file for
> > + ### that case. Assure that any code which was dependent
> > + ### upon the empty file still works. */
> > + svn_boolean_t older_revision_exists = (older_rev > 0);
> > svn_boolean_t same_contents;
> > + const char *comparison_path = older_revision_exists ? older : yours;
> > +
> > SVN_ERR(svn_io_files_contents_same_p(&same_contents,
> > - older, mine, subpool));
> > + comparison_path,
> > + mine, subpool));
> > if (same_contents)
> > {
> > - if (! merge_b->dry_run)
> > - SVN_ERR(svn_io_file_rename(yours, mine, subpool));
> > + if (older_revision_exists && !merge_b->dry_run)
> > + SVN_ERR(svn_io_file_rename(yours, mine, subpool));
> > merge_outcome = svn_wc_merge_merged;
> > merge_required = FALSE;
> > }
> > - }
> > + }