[Svnmerge] r22788: some discussion

[Luke Call]

>  On 13/07/2007 1.08, Giovanni Bajo wrote:
>> I'm try to make some sense out of this r22788 confusion. I saw several patches 
>> from you and Dustin, but I'm still unclear about the root cause of confusion. 
>> I'll quote a mail of yours:
>> 
>> =============================================================================
>> The problem is that r22788 is initializing to the wrong revision number.  If
>> we copy, say, branch from trunk (becoming, respectively, the merge source and
>>  [snip]
 >>
>> Let's move to examples to see if I understood you. I don't like the terms 
>> "trunk" or "branch" either. I think it's misleading. Let's call all of them 
>> "branches".
>> [snip]
> 
> I went ahead and reviewed this patch of yours:
> http://subversion.tigris.org/nonav/issues/showattachment.cgi/650/revise-2nd-r22788-and-tests.patch
> [snip]
 >
> The attached module implements a simple class to parse svn log XML streams 
> through the standard xml.dom.pulldom. I think this should be enough to extract 
> the information you need. My time is over for today, but if you want to pick 
> it up from here, you could try putting this class into svnmerge.py to 
> implement the get_copyfrom() function.
> 
> [If you do so, please submit a first patch that only converts the existing 
> function to pulldom, and then a second patch that fixes the bug introduced 
> with r22788].

I have posted three new patches in connection with issue #2812
(http://subversion.tigris.org/issues/show_bug.cgi?id=2812).

The first one makes use of the xml routine you provided, and is
available here: 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/670/usePulldomNotXml.patch

The second one adds some error & status checking which make bugs more
clear and easier to find.  Someone better at python may see much better
ways: 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/671/addErrCheckingToPulldomUsage.patch


The third one tries to finish the work
started by r22788 on making action_init behavior more helpful.  It is
available here: 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/672/furtherInitLogicFixesAsBegunByr22788.patch

I added some comments to clarify this part of action_init, because it 
takes a while to figure at first.

I went ahead and changed branch_dir and branch_props to target_dir and
target_props.  I realize variable name changes are generally better done
in a separate patch.  In this case a primary goal was clarifying this
code, which has a tricky job to do.  Since I had added comments and new
code where it was important to call things "target*" for clarity, this
other change seemed necessary to avoid confusion, in order to refer
consistently to source and target, rather than to source and branch.  I
will take out that change if preferred.

Also, action_init was taking a global value (opts["revision"]),
assigning to it, then checking conditions against it.  Instead, I
created a local variable to clarify the distinction between global and
local.  This way it is made clearer that opts["revision"] is unchanged
since it was set in main(), and not something the program may or may not
have changed later during execution.  The old way works, but it is
unclear and takes some study to determine why it is safe.

(The variable "revs" is unchanged, but it has two different types which
makes the code harder to read...)

I changed testUninitForce to check for regular expressions instead of
specific revision numbers, because the specific numbers changed but were
not the point of the test.

I removed testCheckInitializeEverything, because it tested for the
earlier behavior; the new test, testInitScenarios, covers the same
ground plus more.  I.e., testCheckInitializeEverything previously
verified that test-branch would be initialized up to the latest
repository revision, or 1-13.  But really, since test-branch was copied
from trunk, it should be initialized to 1-6, because test-branch was
created by copying from trunk revision 6, and I think we would want
svnmerge.py to get all possible changes on trunk since that point, in
the general case.  That is what it does now.

Let me know what else I should do for this bugfix.  Thanks.