[Svnmerge] [PATCH] fix bugs and tests from r22788

[lsuvkne at onemodel.org]

>>> On Sat, Jul 7, 2007 at  1:53 pm, dustin at zmanda.com wrote: 
> This is a *great* explanation of the problem!  Great enough that I think
> it's worth quoting in full :)

Thanks.  This took a while.
 

> Take a look at earlier commit messages already in the logs ‑‑ you'll
> note that they describe specific functions that were changed. 
> [snip]

How about something like this?

[[[
Fix bug & related test failures caused by r22788, to improve default revision range 
set by "svnmerge init" if none provided by user, for scenario where merge source 
is a copy of the merge target (i.e., merging from branch back to trunk). 

* contrib/client‑side/svnmerge/svnmerge.py
  (get_containing_logentry_xml_text): New function.
  (get_copyfrom): Add copy_committed_in_rev to return values; refactor to 
clarify.
  (action_init): Use copy_committed_in_rev from get_copyfrom call for correct 
conditional default revision range; refactor to clarify.

* contrib/client‑side/svnmerge/svnmerge_test.py:  update affected tests

Patch by:  Luke Call <lsuvkne at onemodel.org>
Reviewed by: Dustin J. Mitchell <dustin at zmanda.com> (Dustin clarified comments.)
]]]


> 
>> +        # initializing to 1‑6 explicitly because this test was written
>> +        # to verify behavior in the situation where changes on the 
>> +        # merge source (test‑branch) were made after the changes on the 
> merge 
>> +        # target (trunk); test‑branch was created from trunk's r6, and the
>> +        # changes this tests for came after r6.  An alternative would
>> +        # be to change the setUp method to create test‑branch before
>> +        # the files are added in trunk, if it is more important to test the
>> +        # default init behavior here.
>> +        self.svnmerge("init ‑r 1‑6")
>> +        
> 
> I think that this comment will confuse the dickens out of folks looking
> back on this section of the code.  Let's use the shorter "specify
> revisions explicitly to avoid incidentally testing automatic revision
> guessing"

Yes, yours is a definite improvement.


>> @@ ‑834,12 +853,24 @@
>>  
>>          # Merge into trunk
>>          self.svnmerge("merge ‑vv ‑S branch2",
>> ‑                      match=r"merge ‑r 18:19")
>> +                      match=r"merge ‑‑force ‑r 15:19")
>> +        
>> +        # Before this file was modified in r22788, trunk was initialized 
>> +        # above to test‑branch2 at r17, so it didn't try to merge in r15 of
>> +        # test‑branch2; since now we initialize trunk to an earlier revision
>> +        # of test‑branch2 (see related changes in this patch) and in r15 
>> +        # test‑branch2 had a property change ('/trunk:1‑15',  due 
>> +        # to its own svnmerge init), the merge just performed tried to 
> bring
>> +        # over that property change; normally a user might have to 
>> +        # fix that manually, but since we know that that property change is
>> +        # not fitting here on trunk, we'll get rid of it:
>> +        self.launch("svn resolved .", match="Resolved conflicted state of 
> '.'")
> 
> This case baffled me for a while, and while the above explanation is
> accurate, I think that it's unrelated to the subject of the test.  I'd
> rather simply specify the 'init' revisions that used to be implicit,
> with a comment to that effect, and leave the remainder of the test alone
> (modulo ‑‑force).

You're right that the comment needs to be simplified.  I don't know of a way
to get rid of the "svn resolved" call though, because otherwise the commit 
fails.  How about leaving it, but changing the comment to just something like
"get rid of [extraneous] property conflict caused by previous merges, to allow 
committing"?

(While I haven't studied them closely, this sounds related to the recent discussions
on transitive/graph/"multi-hop"... merges.)

> The svnmerge.py changes look good.  I wonder if we should think of just
> be using a real XML parser instead of this 're' magic, but that's a
> wonderment for another time.
> 
> I guess the only other change is to actually test the new functionality
> you described and implemented.  Unless I missed something in the test
> changes?

>>> On Sat, Jul 7, 2007 at  6:11 pm "Dustin J. Mitchell" <dustin at zmanda.com> wrote: 
> I'm happy to commit this patch as it stands, and since Luke is working
> on patches that are probably of higher value, I'll write some additional
> tests for the new behavior and submit them to the list for review.  I
> won't commit Luke's patch immediately, so please speak up if you have
> objections.
> 
> Dustin

It's better to have tests written to address it explicitly as you've suggested, rather
than just something that will break if behavior changes (as I left it), and I greatly 
appreciate your offer to take care of that.

If you commit my patch (even roughly) as it stands, feel free to improve 
comments as discussed above, or as you see fit.

Thanks, Dustin! 

-Luke