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

[Raman Gupta]

David James wrote:
> On 5/13/06, Raman Gupta <rocketraman at fastmail.fm> wrote:
>> * 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.
> 
> Nice catch, Raman! Could you convert your 'template.sh' test into a
> regular test that will run in svnmerge_test.py? If not, don't worry --
> I'll get to it.

Sure, but it might take me a few days...

>> Index: svnmerge.py
>> [...]
>>         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
> 
> Nice fix! I am sorry that it took me so long to respond to your patch.
> 
> There's a small problem, though: What if 'url' was just created in
> rev, and therefore does not exist in rev-1? In this case,
> get_revlist_prop will throw a LaunchException, and we will need to
> deal with this.
> 
> I fixed your patch to deal with this problem. I also fixed a similar
> bug in the VersionedProperty class and committed your first patch in
> r20064.

I'm not too familiar with the relatively new RevisionLog and
VersionedProperty classes, so I can't say I completely follow what
you've done. However, I have a couple of comments anyway.

In this new section of code:

            try:
                old_value = self.raw_get(log.begin-1)
            except LaunchError:
                # The specified URL might not exist before the
                # range of the log. If so, we can safely assume
                # that the property was empty at that time.
                old_value = { }

Wouldn't it be better to check the situation of the non-existent URL
explicitly rather than relying on LaunchError? LaunchError might occur
for other reasons.

In this section of code (the one which my original patch addressed),
my patch was setting old_revs only for the first loop -- it looks like
here some extra work is being done because old_revs is initialized at
the beginning of the loop every time, which shouldn't be required
because of the last line in the loop:

        for rev in merge_metadata.changed_revs():
            old_revs = merge_metadata.get(rev-1).get(target_dir)
            new_revs = merge_metadata.get(rev).get(target_dir)
            if new_revs != old_revs:
                reflected_revs.append("%s" % rev)
            old_revs = new_revs

Cheers,
Raman