James Westby wrote: Hello James, thank you very much for reviewing the branch at hand. I have revised it to accommodate most of your suggestions. Please have a look at the attached incremental/full diffs as well as at my in-line responses below. You are obviously free to track the changes made in the course of the review here: bzr+ssh://bazaar.launchpad.net/~al-maisan/bzr-builddeb/merge-package > Review: Resubmit >> 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. Good point, alias removed. > === 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 Done. > the TODO can go above. yep. > @@ -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. Done. > === 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. Ah, OK, I did not know they're all in there. Done. > +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? You're right. The function was not needed any longer due to usage of util.find_changelog(), hence removed. > +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) Done. > +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. Has to be .. you wrote it :) > + 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. Good point. > Then also nest two trys, so that target isn't unlocked if there was a > problem before it was locked. OK. > Also, locking tree rather than target is probably going to save some > head scratching later if we add something that needs to read tree. Right. > source.lock_read() > try: > tree.lock_read() > try: > .... > finally: > tree.unlock() > finally: > source.unlock() Done. > + 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. That's certainly a good design principle to adhere to. Not sure whether/how it applies here though. > + # "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. Hmm .. I did away with the list comprehension and only unpack now. > + # 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. Done. > + 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. Another very good point. Code revised accordingly. > 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. How about this? {{{ The "merge-package" command has detected diverged upstream branches for the merge source and target. A shared upstream revision was constructed to remedy the problem. However, merging the shared upstream revision into the merge target resulted in conflicts. Please proceed as follows: 1 - Resolve the current merge conflicts in the merge target directory and commit the changes. 2 - Perform a plain "bzr merge " command, resolve any ensuing packaging branch conflicts and commit once satisfied with the changes. }}} > > === 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() Done. > + > +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" Fixed. > + 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) OK. Done. > 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. Good point. Code simplified accordingly. > + 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? This was a remnant of a previous experiment. Fixed/removed. > 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. OK. I added some more stringent (post target packaging branch fix) tests (in test_debian_upstream_newer()). Please let me know what you think. > 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. Thank you! Best regards -- Muharem Hrnjadovic