[Svnmerge] r22788: some discussion

[Giovanni Bajo]

On 13/07/2007 1.08, Giovanni Bajo wrote:
> Hi Luke,
> 
> 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
> merge target--I prefer the names source and target in the code, since the
> merges could go in any direction), then there are 2 revision numbers of
> interest:
>     1) the particular revision number of the trunk (merge target) from which
>     the branch (merge source) was copied
>     2) and the revion number in which the copy was committed to create the
>     branch (merge source)
> #1 above is the one that r22788 uses. This causes problems I'll demonstrate
> below. #2 is the one that we want to use for initializing a branch in *this
> scenario*.  We want to use #2 because the changes that occur between revisions
> #1 and #2 are not of interest (we are trying to merge in changes that occurred
> ON the branch, which didn't exist before #2).
> =============================================================================
> 
> 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".
> 
> We start from branch A. In commit r20, we create a new branch, B, as a copy 
> from A at 10.
> 
> Now, what should svnmerge init do?
> 
> * In A's working copy, "svnmerge init" should set the property to B:1-20. To 
> the best of my understanding, this is not what it does now.
> * In B's working copy, "svnmerge init" should set the property to A:1-10. To 
> the best of my understanding, this works now.
> 
> So, am I getting this right?

I went ahead and reviewed this patch of yours:
http://subversion.tigris.org/nonav/issues/showattachment.cgi/650/revise-2nd-r22788-and-tests.patch

The first part is a duplicate of Dustin's patch, which I have already 
approved. So that part can be dropped.

The second part is the one I care most, since it tries to fix the above problem.

The patch is a little confused because it also tries to rename some vars: 
please, do not combine unrelated changes like this. If you believe that the 
name of some variables is wrong, please submit a *separate* patch that only 
renames those vars. Anyway, I already told you that I don't like the reasoning 
behind some renames, so please don't send patches renaming "revs" to 
"revisionRange". I prefer short names where they are unambiguous and clear.

As per the actual implementation, I'm impressed by how far you tried to push 
regexp parsing :) I think that regexps worked out decently before, but they're 
the wrong tool now. The information you need must be extracted differently.

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].

Notice that I sticked to Python 2.0 functionalities, since svnmerge.py has to 
be Python 2.0 compatible. This is why I picked up pulldom instead of something 
newer like ElementTree, and didn't implement __iter__ but __getitem__ to 
provide an iterator-like API to the classes. Also, the module is using 
os.popen() as it's a quick example, but should of course relying on the 
existing functions to spawn external processes.
-- 
Giovanni Bajo

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: log.py
URL: </pipermail/svnmerge/attachments/20070713/1994bea3/attachment.ksh>