[Svnmerge] Bugs when merging revisions that modify and deletefiles

[Giovanni Bajo]

On 10/01/2007 22.15, Archie Cobbs wrote:

> For example, suppose a file is changed in rev X and then renamed
> (i.e., copied and deleted) in rev Y.
> 
> Then "svn merge --force" of X and Y will cause the changes in revision
> X to be lost, no?

Absolutely not!

The only difference is that if you merge X and Y within a single "svn merge" 
invocation, it will work as expected. But if you first merge X, and then merge 
  Y (without committing in the middle, of course), the rename will not fully 
work: the delete part of it will be skipped because X has "local 
modifications". But those modifications are just part of the previous merge! 
They are not local modifications by the user (since svnmerge.py always starts 
with a clean working copy)!

As Karsten explained, svnmerge.py is percolated with assumption that merging X 
and Y is the same than merging X first and Y next. svnmerge.py does its best 
to minimize svn merge invocations of course, but it does that for 
*optimization* purposes, not for *semantic* purpose. Without "svn merge 
--force", this invariant is not true.

 > One piece of information that would make the implications of
 > adding "--force" easier to understand (at least for me) would
 > be to know what the exact effect of "--force" is when given
 > to "svn merge".

I tried to look at SVN source code, which I'm not familiar with. In 
libsvn_client, diff.c is where the merge is handled.

===================================================
^L
/*** Callbacks for 'svn merge', invoked by the repos-diff editor. ***/

struct merge_cmd_baton {
   svn_boolean_t force;
   svn_boolean_t dry_run;
   [...]
===================================================

This force flag is propagated and copied around, but used only twice:

===================================================
static svn_error_t *
merge_file_deleted(svn_wc_adm_access_t *adm_access,
[...]
       err = svn_client__wc_delete(mine, parent_access, merge_b->force,
                                   merge_b->dry_run, FALSE, NULL, NULL,
                                   merge_b->ctx, subpool);
===================================================
===================================================
static svn_error_t *
merge_dir_deleted(svn_wc_adm_access_t *adm_access,
[...]
         err = svn_client__wc_delete(path, parent_access, merge_b->force,
                                     merge_b->dry_run, FALSE,
                                     merge_delete_notify_func, &mdb,
                                     merge_b->ctx, subpool);

===================================================

So, for both files and directories, it's passed to svn_client__wc_delete() 
(delete.c), which in turn uses it this way:

===================================================
{

   if (!force && !keep_local)
     /* Verify that there are no "awkward" files */
     SVN_ERR(svn_client__can_delete(path, ctx, pool));

   if (!dry_run)
     /* Mark the entry for commit deletion and perform wc deletion */
     SVN_ERR(svn_wc_delete3(path, adm_access,
                            ctx->cancel_func, ctx->cancel_baton,
                            notify_func, notify_baton, keep_local, pool));
   return SVN_NO_ERROR;
}
===================================================

svn_client__can_delete() calls svn_client_status2(), passing it the callback 
function find_undeletables():

===================================================
   /* Check for error-ful states. */
   if (status->text_status == svn_wc_status_obstructed)
     sb->err = svn_error_createf(SVN_ERR_NODE_UNEXPECTED_KIND, NULL,
                                 _("'%s' is in the way of the resource "
                                   "actually under version control"),
                                 svn_path_local_style(path, sb->pool));
   else if (! status->entry)
     sb->err = svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
                                 _("'%s' is not under version control"),
                                 svn_path_local_style(path, sb->pool));

   else if ((status->text_status != svn_wc_status_normal
             && status->text_status != svn_wc_status_deleted
             && status->text_status != svn_wc_status_missing)
            ||
            (status->prop_status != svn_wc_status_none
             && status->prop_status != svn_wc_status_normal))
     sb->err = svn_error_createf(SVN_ERR_CLIENT_MODIFIED, NULL,
                                 _("'%s' has local modifications"),
                                 svn_path_local_style(path, sb->pool));
===================================================


So, it looks like that if --force is not specificied, svn merge will fail if a 
file being deleted by the merge is:

1) Obstructed
2) Unversioned
3) Locally modified (merged, already deleted, missing).

Case #3 is uninteresting as we said, because svnmerge.py runs only in clean 
working copies, so the only possible local modifications are those produed by 
svnmerge.py itself in previous invocations of "svn merge" within the same 
"svnmerge.py merge" operation: here we *REALLY* want --force.

Case #2 is probably open for discussion. I understand that in some side case 
this can be a problem, but normally I guess that if there are unversioned 
files with the same name of a file being deleted by a merge, they are probably 
just leftovers of previous "svn switch" or manual copies between branches or 
whatnot.

I can't comment on case #1 because I have no clue what an "obstructed" file is.

I'm still +1 on the change though. I was worried that --force had too many 
meaning, but it looks like is confined to merging deletion of files, so I feel 
it's pretty safe.
-- 
Giovanni Bajo