[Svnmerge] [PATCH] Fix for bidirectional merging corner cases

[Raman Gupta]

A refactorization of the code in revision 18696 that checks for reflected revisions broke it for a corner case. The current code may incorrectly consider the first revision to be reflected even when it is not. This can occur if the first revision included a change to the merge property, but one that did not change the merged revision list for the source branch.

See my patch below. Since old_revs is not initialized in the loop that checks the merge_metadata_changed revs, the first rev to be checked is ALWAYS considered reflected.

Note that the actual incidence of this bug is reduced (but not eliminated) by a mistake elsewhere. In analyze_head_revs, the base revision for analysis is selected:

    # Calculate the base of analysis. If there is a "1-XX" interval in the
    # merged_revs, we do not need to check those.
    base = 1
    r = opts["merged-revs"].normalized()
    if r and r[0][0] == 1:
        base = r[0][1]

Note that here, as long as the first merged revision is 1, the base is selected to be an already merged revision. Since this may become the first revision to be checked in analyze_revs (though redundantly because it has already been merged), the algorithm works because old_revs is initialized during that first redundant loop. However, there are two situations in which this problem will still occur despite the error above:

1) if a revision list that only includes true candidate revisions is passed to avail or merge with -r, and

2) if the last revision to be merged DID NOT include a change to the merge properties (such as in my reproduction recipe).

I have attached a reproduction recipe for this problem. In an empty directory, run template.sh first to create the template repo, and then run test.sh as many times as you wish.

Here are the patches (they are independent, but I wouldn't recommend applying the base + 1 patch without the first patch because it will only increase the chance for buggy behavior):

A refactorization of the code in revision 18696 that checks for reflected revisions
broke it for some corner cases. The first revision to be checked may be considered
reflected even when it is not. This can occur if the first revision that included a
change to the merge property did not affect the source branch.

* contrib/client-side/svnmerge.py
  (analyze_revs): If old_revs is not initialized in the loop that checks revisions
    in which the merge props changed, initialize it. This prevents the first revision
    from being considered reflected even if it is not.

Patch by: Raman Gupta <rocketraman at fastmail.fm>
Review by: ?

Index: svnmerge.py
===================================================================
--- svnmerge.py (revision 19635)
+++ svnmerge.py (working copy)
@@ -882,6 +882,8 @@
         old_revs = None
         for rev in merge_metadata.changed_revs():
             new_revs = merge_metadata.get(rev).get(target_dir)
+            if old_revs is None:
+                old_revs = get_revlist_prop(url, opts["prop"], rev-1).get(target_dir)
             if new_revs != old_revs:
                 reflected_revs.append("%s" % rev)
             old_revs = new_revs



Here is a log message/patch for the base + 1 problem (yes, all the tests pass):

Small optimization in obtaining the revisions to be checked for merging. The last 
merged revision is included in analyze_head_revs, but later excluded by the algorithm
anyway, resulting in some unnecessary work.

* contrib/client-side/svnmerge.py
  (analyze_head_revs): Don't include the last merged revision in the list of
    revisions to be checked for merging.

Patch by: Raman Gupta <rocketraman at fastmail.fm>
Review by: ?

Index: svnmerge.py
===================================================================
--- svnmerge.py (revision 19635)
+++ svnmerge.py (working copy)
@@ -905,7 +907,7 @@
     base = 1
     r = opts["merged-revs"].normalized()
     if r and r[0][0] == 1:
-        base = r[0][1]
+        base = r[0][1] + 1

     # See if the user filtered the revision set. If so, we are not
     # interested in something outside that range.


Cheers,
Raman
-------------- next part --------------
A non-text attachment was scrubbed...
Name: template.sh
Type: application/x-shellscript
Size: 1175 bytes
Desc: not available
Url : /pipermail/svnmerge/attachments/20060513/18a41912/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.sh
Type: application/x-shellscript
Size: 2579 bytes
Desc: not available
Url : /pipermail/svnmerge/attachments/20060513/18a41912/attachment-0001.bin