[Svnmerge] [PATCH] fix bugs and tests from r22788
lsuvkne at onemodel.org
lsuvkne at onemodel.org
Sat Jun 30 12:20:24 PDT 2007
Better late than never. Part of the test failures (missing "--force" in
tests) were caused by r23052. But r22788, a good idea that was just
incomplete, is more interesting. See if my thinking makes sense or not.
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).
r22788 is the cause of a discussion we had earlier, though we didn't realize
it then:
>> + """todo:
>> + Why does init require parameters in this case?‑‑try it without the
>> + "‑r 1‑13" parameter to see the errors. Is this a
>> + bug that ppl will run into a lot & should be fixed somehow, maybe by
>> + excluding that revision range, in init, automatically? (why
>> + did init, w/o parameters, only init to 1‑6, when it wasn't created
>> + until 12?) Similarly, if you just do "svnmerge init" then
>> + "svnmerge block 12", it fails because it gets the logs to only process
>> + revision #'s that actually show up in the logs as changed for that
>> + particular file (to omit things changed on other urls, I think), and
>> + 12 isn't one of those.
>> + """
>
>I ran into this problem too ‑‑ it would be nice to fix, but that's
>another patch :)
To reproduce a simplified example of the problem, one can insert this into
svnmerge_test.py, inside the class TestCase_TestRepo(TestCase_SvnMerge), and
run "svnmerge_test.py TestCase_TestRepo.testMergeFileRenameOntoModifiedFile".
If you trade the commented line for the one after it, then it gets no error.
def testScenarioCausedByr22788(self):
os.chdir(self.test_path)
os.chdir("trunk")
#self.svnmerge2(["init", self.test_path + "/test-branch", "-r 1-13"])
self.svnmerge2(["init", self.test_path + "/test-branch"])
self.multilaunch("""
svn up
svn ci -m 'initialize svnmerge'
""")
os.chdir("../trunk")
self.svnmerge("merge")
r22788 also broke compliance with this statement in "svnmerge help init":
"If SOURCE is specified, all the revisions in SOURCE are marked as already
merged"
This was no longer true, in the case where the source was copied from the
target.
This gets the error:
LaunchError: (1, 'svn merge --force -r 12:13
file:///tmp/__svnmerge_test/repo/branches/test-branch .', "svn: Unable to
find repository location for
'file:///tmp/__svnmerge_test/repo/branches/test-branch' in revision 12\n")
..because test-branch didn't exist in r12, but svnmerge init thinks it did.
It shows that r22788 messes up default behavior even in a simple case.
So I provided additional logic to complete the helpful thinking behind r22788,
and renamed a few variables for clarity and readability (like clearing up the
source/target role issue a little).
It passes the tests, at least on gnu/linux (suse 10.0) and windows xp. I
think the existing tests as modified are adequate for this change; they aren't
designed specifically to communicate it, as would be ideal, but they do
exercise the code and fail when needed.
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
]]]
Best,
-Luke
-------------- next part --------------
Index: contrib/client-side/svnmerge/svnmerge_test.py
===================================================================
--- contrib/client-side/svnmerge/svnmerge_test.py (revision 25555)
+++ contrib/client-side/svnmerge/svnmerge_test.py (working copy)
@@ -499,7 +499,10 @@
def testBlocked(self):
# Initialize svnmerge
- self.svnmerge("init")
+ # (Initializing explicitly to versions 1-6; see comments under
+ # testBasic() for why.)
+ self.svnmerge("init -r 1-6")
+
self.launch("svn commit -F svnmerge-commit-message.txt",
match=r"Committed revision")
@@ -551,7 +554,16 @@
self.svnmerge("avail -B", match=r"\A$")
def testBasic(self):
- self.svnmerge("init")
+ # 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")
+
p = self.getproperty()
self.assertEqual("/trunk:1-6", p)
@@ -695,7 +707,10 @@
def testTrimmedAvailMerge(self):
"""Check that both avail and merge do not search for phantom revs too hard."""
- self.svnmerge("init")
+ # (Initializing explicitly to versions 1-6; see comments under
+ # testBasic() for why.)
+ self.svnmerge("init -r 1-6")
+
self.svnmerge("avail -vv -r8-9", match=r"svn log.*-r8:9")
self.svnmerge("merge -F -vv -r8-9", match=r"svn log.*-r8:9")
self.svnmerge("avail -vv -r2", nonmatch=r"svn log")
@@ -703,7 +718,11 @@
def testMergeRecordOnly(self):
"""Check that flagging revisions as manually merged works."""
- self.svnmerge("init")
+ """Check that both avail and merge do not search for phantom revs too hard."""
+ # (Initializing explicitly to versions 1-6; see comments under
+ # testBasic() for why.)
+ self.svnmerge("init -r 1-6")
+
self.svnmerge("avail -vv -r9", match=r"svn log.*-r9:9")
self.svnmerge("merge --record-only -F -vv -r9",
nonmatch=r"svn merge -r 8:9")
@@ -737,7 +756,7 @@
self.launch("svn commit -m \"Change to test1 on trunk\"",
match=r"Committed revision 16")
- self.svnmerge("integrated", match=r"^13-14$")
+ self.svnmerge("integrated", match=r"^13$")
os.chdir("..")
os.chdir("test-branch")
@@ -746,7 +765,7 @@
self.launch("svn update", match=r"At revision 16")
self.svnmerge("avail -vv --bidirectional", match=r"16$")
- self.svnmerge("merge -vv --bidirectional", match=r"merge -r 15:16")
+ self.svnmerge("merge -vv --bidirectional", match=r"merge --force -r 15:16")
p = self.getproperty()
self.assertEqual("/trunk:1-16", p)
self.svnmerge("integrated", match=r"^3-16$")
@@ -772,7 +791,7 @@
# Now check reflected revision is excluded with --bidirectional flag.
self.svnmerge("avail -vv --bidirectional", match=r"18$")
- self.svnmerge("merge -vv --bidirectional", match=r"merge -r 17:18")
+ self.svnmerge("merge -vv --bidirectional", match=r"merge --force -r 17:18")
p = self.getproperty()
self.assertEqual("/branches/test-branch:1-18", p)
@@ -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 '.'")
+
p = self.getproperty()
- self.assertEqual("/branches/test-branch:1-16 /branches/test-branch2:1-19", p)
+ self.assertEqual("/branches/test-branch:1-13 /branches/test-branch2:1-19", p)
self.svnmerge("integrated -S branch2", match=r"^14-19$")
- self.svnmerge("integrated -S ../test-branch", match=r"^13-16$")
+ self.svnmerge("integrated -S ../test-branch", match=r"^13$")
self.launch("svn commit -F svnmerge-commit-message.txt",
match=r"Committed revision 20")
@@ -855,7 +886,7 @@
# should be available for test-branch with --bidirectional flag.
self.svnmerge("avail -vv --bidirectional", match=r"20$")
- self.svnmerge("merge -vv --bidirectional", match=r"merge -r 17:20")
+ self.svnmerge("merge -vv --bidirectional", match=r"merge --force -r 17:20")
p = self.getproperty()
self.assertEqual("/trunk:1-20", p)
Index: contrib/client-side/svnmerge/svnmerge.py
===================================================================
--- contrib/client-side/svnmerge/svnmerge.py (revision 25555)
+++ contrib/client-side/svnmerge/svnmerge.py (working copy)
@@ -757,22 +757,79 @@
assert url[:len(root)] == root, "url=%r, root=%r" % (url, root)
return url[len(root):]
+def get_containing_logentry_xml_text(logText, containedPatternMatchObject):
+ # (get the text of the single logentry xml element surrounding the
+ # pattern that we previously matched in the caller.
+ # (Not sure why the
+ # non-greedy quantifiers in a regexp "logEntryContainingFoundAddActionPattern"
+ # didn't seem to work for this--i.e., putting in '.*?' just before and
+ # after the mention of "actionAddPathPattern" in the section of
+ # get_copyfrom() right after this is called, caused it to get the
+ # whole regex which contains many log entries--not just the single
+ # log entry I want. (todo: use an xml parser instead? no matter for now)
+ start = 0
+ while start != -1:
+ # find the latest occurrence of "<logentry" before the
+ # preceding pattern match (in effect, search from it backwards
+ # to find its containing element).
+ mightBeStart = logText.find("<logentry", start + 1) # " + 1" to prevent looping forever; this string isn't first in the file, so we won't miss the first instance.
+ if mightBeStart == -1 or mightBeStart > containedPatternMatchObject.start():
+ break
+ start = mightBeStart
+
+ end = logText.find("</logentry>", containedPatternMatchObject.end()) + 11
+ if start == 0 or end == -1:
+ raise Exception, "Did not find " + \
+ "element as expected: 'A' log entry for " + \
+ "'" + repos_path + "' should be surrounded by " + \
+ "'logentry' xml element, in log text '" + \
+ + logText + "'"
+ return logText[start:end]
+
def get_copyfrom(dir):
"""Get copyfrom info for a given target (it represents the directory from
where it was branched). NOTE: repos root has no copyfrom info. In this case
- None is returned."""
+ None is returned.
+
+ Returns the:
+ - source file or directory from which the copy was made
+ - the revision from which that source was copied
+ - the revision in which the copy was committed
+ """
repos_path = target_to_repos_relative_path(dir)
- out = launchsvn('log -v --xml --stop-on-copy "%s"' % dir,
+ actionAddPattern = r'(<path\s[^>]*action="A"[^>]*>%s</path>)'
+ logText = launchsvn('log -v --xml --stop-on-copy "%s"' % dir,
split_lines=False)
- out = out.replace("\n", " ")
+ logText = logText.replace("\n", " ")
+
+ # first get the source; abort if unavailable (not a copy)
try:
- m = re.search(r'(<path\s[^>]*action="A"[^>]*>%s</path>)'
- % re.escape(repos_path), out)
- source = re.search(r'copyfrom-path="([^"]*)"', m.group(1)).group(1)
- rev = re.search(r'copyfrom-rev="([^"]*)"', m.group(1)).group(1)
- return source, rev
+ actionAddPatternWithPath = actionAddPattern % re.escape(repos_path)
+ addPathPatternMatch = re.search(actionAddPatternWithPath, logText)
+ source = re.search(r'copyfrom-path="([^"]*)"', \
+ addPathPatternMatch.group(1)).group(1)
except AttributeError:
- return None,None
+ return None,None,None
+
+ # get the revision from which source was copied (if the above re.search
+ # succeeded, yet this throws an exception, we probably want to know).
+ rev = re.search(r'copyfrom-rev="([^"]*)"',
+ addPathPatternMatch.group(1)).group(1)
+
+ # Get the revision in which the copy was committed.
+ logentryElementText = get_containing_logentry_xml_text(logText, addPathPatternMatch)
+ logEntryPatternContainingFoundAddAction = \
+ '\<logentry\s+revision\=\"(\d+)\".*' + \
+ actionAddPatternWithPath + \
+ '.*\</logentry\>'
+ match = re.search(logEntryPatternContainingFoundAddAction, logentryElementText)
+ if match == None:
+ raise Exception, "pattern: " + \
+ logEntryPatternContainingFoundAddAction + " ...not found in " + \
+ logentryElementText
+ copy_committed_in_rev = match.group(1)
+
+ return source, rev, copy_committed_in_rev
def get_latest_rev(url):
"""Get the latest revision of the repository of which URL is part."""
@@ -1013,43 +1070,44 @@
else:
assert False, "unhandled display style: %s" % display_style
-def action_init(branch_dir, branch_props):
+def action_init(target_dir, target_props):
"""Initialize a branch for merges."""
# Check branch directory is ready for being modified
- check_dir_clean(branch_dir)
-
+ check_dir_clean(target_dir)
+
# If the user hasn't specified the revisions to use, see if the
# "source" is a branch from the current tree and if so, we can use
# the version data obtained from it.
- if not opts["revision"]:
- cf_source, cf_rev = get_copyfrom(opts["source-url"])
- branch_path = target_to_repos_relative_path(branch_dir)
+ revisionRange = opts["revision"]
+ if not revisionRange:
+ cf_source_path, cf_rev, copy_committed_in_rev = get_copyfrom(opts["source-url"])
+ target_path = target_to_repos_relative_path(target_dir)
- # If the branch_path is the source path of "source",
+ # If the target_path is the same path from which "source" was
+ # copied when it was created,
# then "source" was branched from the current working tree
# and we can use the revisions determined by get_copyfrom
- if branch_path == cf_source:
+ if target_path == cf_source_path:
report('the source "%s" is a branch of "%s"' %
- (opts["source-url"], branch_dir))
- opts["revision"] = "1-" + cf_rev
+ (opts["source-url"], target_dir))
+ revisionRange = "1-" + copy_committed_in_rev
# Get the initial revision set if not explicitly specified.
- revs = opts["revision"] or "1-" + get_latest_rev(opts["source-url"])
- revs = RevisionSet(revs)
+ revisionRange = revisionRange or "1-" + get_latest_rev(opts["source-url"])
+ revs = str(RevisionSet(revisionRange))
report('marking "%s" as already containing revisions "%s" of "%s"' %
- (branch_dir, revs, opts["source-url"]))
-
- revs = str(revs)
+ (target_dir, revisionRange, opts["source-url"]))
+
# If the source-path already has an entry in the svnmerge-integrated
# property, simply error out.
- if not opts["force"] and branch_props.has_key(opts["source-path"]):
+ if not opts["force"] and target_props.has_key(opts["source-path"]):
error('%s has already been initialized at %s\n'
- 'Use --force to re-initialize' % (opts["source-path"], branch_dir))
- branch_props[opts["source-path"]] = revs
+ 'Use --force to re-initialize' % (opts["source-path"], target_dir))
+ target_props[opts["source-path"]] = revs
# Set property
- set_merge_props(branch_dir, branch_props)
+ set_merge_props(target_dir, target_props)
# Write out commit message if desired
if opts["commit-file"]:
@@ -1899,11 +1957,11 @@
report("calculate source path for the branch")
if not source:
if str(cmd) == "init":
- cf_source, cf_rev = get_copyfrom(branch_dir)
- if not cf_source:
+ cf_source_path, cf_rev, copy_committed_in_rev = get_copyfrom(branch_dir)
+ if not cf_source_path:
error('no copyfrom info available. '
'Explicit source argument (-S/--source) required.')
- opts["source-path"] = cf_source
+ opts["source-path"] = cf_source_path
if not opts["revision"]:
opts["revision"] = "1-" + cf_rev
else:
More information about the Svnmerge
mailing list