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

[Dustin J. Mitchell]

On Sat, Jun 30, 2007 at 01:20:24PM -0600, lsuvkne at onemodel.org wrote:
> 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).
> 
> Note however, that this situation reverses if the trunk becomes the merge
> source, and the branch becomes the merge target (i.e., one is merging into a
> branch from trunk, to stay synchronized with the changes on trunk).  In that
> case we do want to initialize to revision #1 above.  I'll try to handle this
> in a subsequent patch.
> 
> (Another scenario is the one that svnmerge already handled automatically: when
> neither the merge source nor target is a copy of the other. In this case it
> inits svnmerge on the target to the latest revision of the merge source,
> saying in effect, "everything has been merged, so start tracking from right
> now).

This is a *great* explanation of the problem!  Great enough that I think
it's worth quoting in full :)

> Is this version comment ok?  Why would we the modified filenames, as shown in
> the hacker's guide example, since they are also in the "log -v" output
> already? Is it a place to comment each modified function? In this case maybe
> it makes more sense to just put in the one comment line, and no filenames, but
> I'm wondering what I'm missing.
> 
> [[[
> Fix bug & related test failures caused by r22788.  Improve default handling of
> "init" command as r22788 intended, for scenario where merge source is a copy
> of the merge target.
> 
> * contrib/client-side/svnmerge/svnmerge.py
> * contrib/client-side/svnmerge/svnmerge_test.py
> ]]]

Take a look at earlier commit messages already in the logs -- you'll
note that they describe specific functions that were changed.  I'll
admit I've been lax in this duty!

> +        # 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"

> @@ -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).

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?

Dustin

-- 
        Dustin J. Mitchell
        Storage Software Engineer, Zmanda, Inc.
        http://www.zmanda.com/