> Hello James, > > this branch implements the 'merge-package' command as specified here: > http://pastebin.com/f7214e464. > > Please take a look and let me know what you think. === modified file '__init__.py' --- __init__.py 2009-07-26 15:51:02 +0000 +++ __init__.py 2009-08-06 09:59:34 +0000 @@ -39,7 +39,8 @@ "merge_upstream": ["mu"], "import_dsc": [], "bd_do": [], - "mark_uploaded": [] + "mark_uploaded": [], + "merge_package": ["mp"] } for command, aliases in commands.iteritems(): I fear this alias will conflict with something in the future (merge-proposal perhaps), perhaps it is better to leave it off for now and let people set it as they like. === modified file 'cmds.py' --- cmds.py 2009-07-26 18:21:49 +0000 +++ cmds.py 2009-08-19 09:06:47 +0000 @@ -870,6 +871,39 @@ t.unlock() +class cmd_merge_package(Command): + """Merges source packaging branch into target packaging branch. + + This will first check whether the upstream branches have diverged. + + If that's the case an attempt will be made to fix the upstream ancestry + so that the user only needs to deal wth packaging branch merge issues. + + In the opposite case a normal merge will be performed. + """ + takes_args = ['source'] + + def run(self, source): + source_branch = target_branch = None + # Get the target branch. + try: + tree = WorkingTree.open_containing('.')[0] + target_branch = tree.branch + except NotBranchError: + raise BzrCommandError( + "There is no tree to merge the source branch in to") + # Get the source branch. + try: + source_branch = Branch.open(source) + except NotBranchError: + raise BzrCommandError("Invalid source branch URL?") + + fix_ancestry_as_needed(tree, source_branch) + + # Merge source packaging branch in to the target packaging branch. + tree.merge_from_branch(source_branch) + + class cmd_test_builddeb(Command): """Run the builddeb test suite""" @@ -880,4 +914,3 @@ passed = selftest(test_suite_factory=test_suite) # invert for shell exit code rules return not passed - === modified file 'import_dsc.py' --- import_dsc.py 2009-07-26 16:44:17 +0000 +++ import_dsc.py 2009-08-19 08:58:47 +0000 @@ -1570,7 +1570,7 @@ finally: shutil.rmtree(tempdir) - def _extract_upstream_tree(self, upstream_tip, basedir): + def extract_upstream_tree(self, upstream_tip, basedir): # Extract that to a tempdir so we can get a working # tree for it. # TODO: should stack rather than trying to use the repository, @@ -1582,6 +1582,13 @@ self.upstream_tree = dir_to.open_workingtree() self.upstream_branch = self.upstream_tree.branch + def _extract_upstream_tree(self, upstream_tip, basedir): + # This method is now being used outside this module and hence + # not really private any longer. + # TODO: obsolete/remove this method and start using + # extract_upstream_tree() instead. + self.extract_upstream_tree(upstream_tip, basedir) + def _create_empty_upstream_tree(self, basedir): to_location = os.path.join(basedir, "upstream") to_transport = get_transport(to_location) This can just be _extract_upstream_tree = extract_upstream_tree the TODO can go above. @@ -1616,6 +1623,10 @@ raise def _revid_of_upstream_version_from_branch(self, version): + """The private method below will go away eventually.""" + return self.revid_of_upstream_version_from_branch(version) + + def revid_of_upstream_version_from_branch(self, version): assert isinstance(version, str) tag_name = self.upstream_tag_name(version) if self._has_version(self.branch, tag_name): Same. === added file 'merge_package.py' --- merge_package.py 1970-01-01 00:00:00 +0000 +++ merge_package.py 2009-08-19 09:19:38 +0000 +class WrongBranchType(errors.BzrError): + _fmt = "The merge target is not a packaging branch." + + +class InvalidChangelogFormat(errors.BzrError): + _fmt = "The debian/changelog is empty or not in valid format." + + +class SourceUpstreamConflictsWithTargetPackaging(errors.BzrError): + _fmt = ( + "The source upstream branch conflicts with " + "the target packaging branch") The rest of the errors live in errors.py, we should probably continue that. +def _read_file(branch, path): + """Get content of file for given `branch` and `path. + + :param branch: A Branch object containing the file of interest. + :param path: The path of the file to read. + """ + try: + tree = branch.basis_tree() + tree.lock_read() + content = tree.get_file_text(tree.path2id(path)) + tree.unlock() + except errors.NoSuchId: + raise WrongBranchType() + + return content + Please try: finally: any locking. However, I'm surprised you needed to add this locking, the branch should already be locked shouldn't it? +def _latest_version(branch): + """Version of the most recent source package upload in the given `branch`. + + :param branch: A Branch object containing the source upload of interest. + """ + upload_version = '' + changelog = _read_file(branch, "debian/changelog") + + for line in changelog.splitlines(): + # Look for the top-level changelog stanza, extract the + # upload version from it and break on success. + match = re.search('^.+\(([^)]+)\).*$', line) + if match is not None: + (upload_version,) = match.groups(1) + break + + upload_version = upload_version.strip() + if len(upload_version) <= 0: + raise InvalidChangelogFormat() + + return Version(upload_version) Urgh, use the Changelog class from debian_bundle.changelog and its .version attribute. (Note the max_blocks parameter used elsewhere, e.g. utils.py find_changelog) +def fix_ancestry_as_needed(tree, source): + """Manipulate the merge target's ancestry to avoid upstream conflicts. + + Merging J->I given the following ancestry tree is likely to result in + upstream merge conflicts: + + debian-upstream ,------------------H + A-----------B \ + ubuntu-upstream \ \`-------G \ + \ \ \ \ + debian-packaging \ ,---------D--------\-----------J + C \ \ + ubuntu-packaging `----E------F--------I + + Here there was a new upstream release (G) that Ubuntu packaged (I), and + then another one that Debian packaged, skipping G, at H and J. + + Now, the way to solve this is to introduce the missing link. + + debian-upstream ,------------------H------. + A-----------B \ \ + ubuntu-upstream \ \`-------G-----------\------K + \ \ \ \ + debian-packaging \ ,---------D--------\-----------J + C \ \ + ubuntu-packaging `----E------F--------I + + at K, which isn't a real merge, as we just use the tree from H, but add + G as a parent and then we merge that in to Ubuntu. + + debian-upstream ,------------------H------. + A-----------B \ \ + ubuntu-upstream \ \`-------G-----------\------K + \ \ \ \ \ + debian-packaging \ ,---------D--------\-----------J \ + C \ \ \ + ubuntu-packaging `----E------F--------I------------------L + + At this point we can merge J->L to merge the Debian and Ubuntu changes. + + :param tree: The `WorkingTree` of the merge target branch. + :param source: The merge source (packaging) branch. + """ Nice docstring. + upstreams_diverged = False + t_upstream_reverted = False + target = tree.branch + + try: + source.lock_read() + target.lock_read() Please lock_read() outside the try, otherwise if there is an exception in the lock_read call it will try and unlock and unlocked branch, causing an exception in the recovery code, masking the real problem. Then also nest two trys, so that target isn't unlocked if there was a problem before it was locked. Also, locking tree rather than target is probably going to save some head scratching later if we add something that needs to read tree. source.lock_read() try: tree.lock_read() try: .... finally: tree.unlock() finally: source.unlock() + finally: + source.unlock() + target.unlock() Don't unlock until you stop reading from them, that's just asking for races if you have to re-lock. + # "Unpack" the upstream versions and revision ids for the merge source and + # target branch respectively. + [(usource_v, usource_revid), (utarget_v, utarget_revid)] = upstream_vdata Unpacking here after you did the list comprehension for half the data before seems a little replicated. + # Merge upstream branch tips to obtain a shared upstream parent. This + # will add revision K (see graph above) to a temporary merge target + # upstream tree. + try: + tmp_target_upstream_tree.lock_write() Outside the try again please. + try: + tree.merge_from_branch(tmp_target_upstream_tree.branch) + tree.commit('Merging source packaging branch in to target.') + except ConflictsInTree: + raise SourceUpstreamConflictsWithTargetPackaging() I would prefer to use the return of merge_from_branch to detect conflicts, rather than the exception on commit. This is the big question I have left though. How does the user continue after this? There should be a clear way to continue once they have committed, and the exception should tell them how to proceed. === added file 'tests/test_merge_package.py' --- tests/test_merge_package.py 1970-01-01 00:00:00 +0000 +++ tests/test_merge_package.py 2009-08-19 09:44:38 +0000 +def _prepend_log(text, path): + content = open(path).read() + fh = open(path, 'wb') + fh.write(text+content) + fh.close() Please try: finally: the close() + +class MergePackageTests(TestCaseWithTransport): + + def test_debian_upstream_newer(self): + """Diverging upstreams (debian newer) don't cause merge conflicts. + + The debian and ubuntu upstream branches will differ with regard to + the content of the file 'c'. + + Furthermore the respective packaging branches will have a text + conflict in 'debian/changelog'. + + The upstream conflict will be resolved by fix_ancestry_as_needed(). + Please note that the debian ancestry is more recent. + """ + ubup_o, debp_n = self._setup_debian_upstrem_newer() typo: "upstream" + def _setup_branch(self, name, vdata, tree=None, log_format=None): + vids = list(string.ascii_uppercase) + days = range(len(string.ascii_uppercase)) + + if tree is None: + tree = self.make_branch_and_tree(name) You need to be more careful about locking. tree = tree.lock_write() self.addCleanup(tree.unlock) and a couple of other places in the tests. + def commit(msg, version): + vid = vids.pop(0) + if log_format is not None: + cle = changelog(version, vid) + p = '%s/work/%s/debian/changelog' % (self.test_base_dir, name) + _prepend_log(cle, p) + revid = tree.commit('%s: %s' % (vid, msg), rev_id='%s-%s' % (name, vid)) + setattr(self, revid_name(vid), revid) + tree.branch.tags.set_tag(version, revid) There's not really a need to control the revids if you are not going to make use of that, especially since you save the returned revids anyway. + for version, paths, utree, urevid in vdata: + msg = '' + if utree is not None: + tree.merge_from_branch( + utree.branch, to_revision=urevid, merge_type=WeaveMerger) Why are you setting the merge type? You have done a good job of testing the behaviour, but I would also like to see checks of the ancestry, so that you know the merges were done in the right direction etc. Also, a blackbox test or three would be useful, but they are difficult to exercise properly, so coverage of everything you would like may be tricky. This is generally very good though, good clean code, thank you. James