[Svnmerge] [PATCH] Bidirectional merging patch for svnmerge.py

[Raman Gupta]

Giovanni Bajo wrote:
> Raman Gupta <rocketraman at fastmail.fm> wrote:
> 
>>> Attached is a bidirectional merging patch for the latest version of
>>> svnmerge.py (rev 238).  It removes reflected revisions (i.e. changes
>>> on the branch that were originally merged from the target, and therefore
>>> should generally not be merged back into the target) from the
>>> available/merge list. Reflected revisions can cause spurious
>>> conflicts if any changes were made to the merged code on the branch.
> 
> Hi Raman, thanks for the patch. I don't feel very fond of this feature because
> I don't think it really solves anything, but I'm not going to stay in its way.
> I admit I have no widespread experience with bidi merges, so I'm going to trust
> your and others' word on this.

Its pretty easy to demonstrate the use case:

1) Make change A on branch 1.

2) Merge change A to branch 2.

3) Make change B on Change A on branch 2.

4) Merge change B back to branch 1.

Clearly, the only thing that should be merged back to branch 1 is change
B. But in this situation, svn/svnmerge will *always* result in a
conflict. With the --bidi flag, this is handled flawlessly.

> So I'll comment only on its technical merit and happily commit it when
> it's technically sound.

Wonderful.

>>> The upside to this is that no dot-files are required, the downside
>>> is that it is slow because it requires a remote svn diff call for
>>> each possible revision to be merged.
> 
> I appreciate that it's disabled by default, but it's still very slow in my
> opinion. I think you can achieve a much better speed if you add "--verbose" to
> the single "svn log" invokation done in analyze_revs. Does it make sense to
> you?

No, unfortunately log --verbose does not print the diffs for the
integrated property, which is necessary in order to determine whether or
not the revision is reflected. That information can only be obtained via
"diff".

>>> +    def __len__(self):
>>> +        return len(self._revs.keys())
> 
> len(self._revs) is sufficient.

Changed.

>>>      # 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]
>>> +        base = r[0][1] + 1
> 
> I don't think this is correct. Can you explain it?

Sure, this is unrelated to the bidi patch (my bad) but it is a little
optimization. If the interval is 1-XX, that means XX has already been
merged. Therefore, is is only required to analyze XX + 1 for merging. In
the end it works the same way as before because the merge command checks
the merged property and removes XX from the list anyway, but I noticed
this because one more reflected revision check was occurring than I
expected.

>>> tracking (merging from multiple heads)"),
>>> +    Option("-b", "--bidi", value=True, default=False,
>>> +              help="check for reflected revisions that can cause conflicts
> "
>>> +                  "when merging bidirectionally between branches)"), ]
> 
> closing parens ')' without matching opening parens. I'd appreciate if you also
> add some blurb to the "svnmerge merge" description which explains what
> reflected revisions are. In fact, I was going to add something about phantom
> revisions in the "svnmerge avail" description.

Oops, fixed/added.

> Thanks again for the patch!

Attached, version 2.

> Giovanni Bajo

Raman Gupta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bidi.patch
Type: text/x-patch
Size: 9402 bytes
Desc: not available
Url : /pipermail/svnmerge/attachments/20060219/526ce057/attachment.bin