bzr-builddeb

Merge lp:~al-maisan/bzr-builddeb/merge-package into lp:~bzr-builddeb-hackers/bzr-builddeb/trunk-old

merge-package
Merge into trunk-old

Proposed by Muharem Hrnjadovic on 2009-08-19

Status:	Merged
Merged at revision:	not available
Proposed branch:	lp:~al-maisan/bzr-builddeb/merge-package
Merge into:	lp:~bzr-builddeb-hackers/bzr-builddeb/trunk-old
Diff against target:	None lines
To merge this branch:	bzr merge lp:~al-maisan/bzr-builddeb/merge-package
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
James Westby		2009-08-19	Approve on 2009-08-24
Review via email: mp+10369@code.launchpad.net

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2009-08-19:

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.

lp:~al-maisan/bzr-builddeb/merge-package updated on 2009-08-19

391. By Muharem Hrnjadovic on 2009-08-19: More test clean-ups.

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2009-08-19:

Hmm .. some more test code clean-ups .. please see the attached
incremental diff.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

builddeb-inc.diff

Revision history for this message

James Westby (james-w) wrote on 2009-08-19:

Download full text (12.6 KiB)

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

> 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

review: Needs Resubmitting

lp:~al-maisan/bzr-builddeb/merge-package updated on 2009-08-21

392. By Muharem Hrnjadovic on 2009-08-19: Enhancements stemming from James' review comments, round 1
393. By Muharem Hrnjadovic on 2009-08-19: Minor fix.
394. By Muharem Hrnjadovic on 2009-08-19: Enhancements stemming from James' review comments, round 2
395. By Muharem Hrnjadovic on 2009-08-19: Enhancements stemming from James' review comments, round 3
396. By Muharem Hrnjadovic on 2009-08-19: Enhancements stemming from James' review comments, round 4
397. By Muharem Hrnjadovic on 2009-08-19: Enhancements stemming from James' review comments, round 5
398. By Muharem Hrnjadovic on 2009-08-19: Enhancements stemming from James' review comments, round 6
399. By Muharem Hrnjadovic on 2009-08-19: Enhancements stemming from James' review comments, round 7
400. By Muharem Hrnjadovic on 2009-08-20: Minor fix.
401. By Muharem Hrnjadovic on 2009-08-20: Minor test code fix.
402. By Muharem Hrnjadovic on 2009-08-21: Added test that checks the ancestry pre- and post-fix.
403. By Muharem Hrnjadovic on 2009-08-21: Added a more stringent test that checks the ancestry pre- and post-fix.

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2009-08-21:

Download full text (15.2 KiB)

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.

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 <source-packaging-branch>"
          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 <muharem@canonical.com>
Public key id   : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F  5602 219F 6B60 B2BB FCFC

merge-package-inc.diff

merge-package-full.diff

Revision history for this message

James Westby (james-w) wrote on 2009-08-21:

Excerpts from Muharem Hrnjadovic's message of Fri Aug 21 12:51:08 UTC 2009:
> > 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 <source-packaging-branch>"
> command, resolve any ensuing packaging branch conflicts
> and commit once satisfied with the changes.
> }}}

For starters this is bit verbose, we can't really format it like that
in an error message, so it needs to be a couple of sentences.

I don't think they need to know that a new "shared upstream" was
created, you can just say that there are conflicts but they are
not finished.

Should we direct them to "bzr merge-package" again? It becomes
equivalent, but I think it's better to not confuse them with
when they can use "bzr merge" and when they can't.

You don't need to tell them what to do after they've run that again,
it can just tell them when it is done.

Would it be worth saying that if they don't like what they see
they can just "bzr revert" to get back to where they started?

Looking at the incremental diff the major issue I see remaining
is that you lock and unlock the tree, only to lock it again.
Lock it once, and hold that lock until you don't reference the
tree or anything that makes use of it any more.

Writing this mail also interested me in whether you ever get any
conflicts in the case when merging the faked upstream merge
in to the packaging branch in the case where your upstream
version was newer than the other upstream version.

Thanks,

James

Excerpts from Muharem Hrnjadovic's message of Fri Aug 21 12:51:08 UTC 2009:
> > 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 <source-packaging-branch>"
>           command, resolve any ensuing packaging branch conflicts
>           and commit once satisfied with the changes.
> }}}

For starters this is bit verbose, we can't really format it like that
in an error message, so it needs to be a couple of sentences.

I don't think they need to know that a new "shared upstream" was
created, you can just say that there are conflicts but they are
not finished.

Should we direct them to "bzr merge-package" again? It becomes
equivalent, but I think it's better to not confuse them with
when they can use "bzr merge" and when they can't.

You don't need to tell them what to do after they've run that again,
it can just tell them when it is done.

Would it be worth saying that if they don't like what they see
they can just "bzr revert" to get back to where they started?

Thanks,

James

lp:~al-maisan/bzr-builddeb/merge-package updated on 2009-08-24

404. By Muharem Hrnjadovic on 2009-08-24: Target branch is now being locked once only; James' review comments, round 7
405. By Muharem Hrnjadovic on 2009-08-24: Enhanced error message for case where merging the shared upstreams revision results in conflicts; James' review comments, round 8
406. By Muharem Hrnjadovic on 2009-08-24: Minor fix.
407. By Muharem Hrnjadovic on 2009-08-24: Slightly cleaned-up test.

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2009-08-24:

James Westby wrote:
> Excerpts from Muharem Hrnjadovic's message of Fri Aug 21 12:51:08 UTC 2009:
>>> 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 <source-packaging-branch>"
>> command, resolve any ensuing packaging branch conflicts
>> and commit once satisfied with the changes.
>> }}}
>
> For starters this is bit verbose, we can't really format it like that
> in an error message, so it needs to be a couple of sentences.
>
> I don't think they need to know that a new "shared upstream" was
> created, you can just say that there are conflicts but they are
> not finished.
>
> Should we direct them to "bzr merge-package" again? It becomes
> equivalent, but I think it's better to not confuse them with
> when they can use "bzr merge" and when they can't.
>
> You don't need to tell them what to do after they've run that again,
> it can just tell them when it is done.
>
> Would it be worth saying that if they don't like what they see
> they can just "bzr revert" to get back to where they started?

Hello James,

thank you very much for your suggestions above. They are all good. I
hope the following text is more suitable.

{{{
The upstream branches for the merge source and target have diverged.
Unfortunately, the attempt to fix this problem resulted in conflicts.
Please resolve these and re-run the "merge-package" command to finish.
Alternatively, you can restore the original merge target state by
running "bzr revert".
}}}

>
> Looking at the incremental diff the major issue I see remaining
> is that you lock and unlock the tree, only to lock it again.
> Lock it once, and hold that lock until you don't reference the
> tree or anything that makes use of it any more.

Done.

> Writing this mail also interested me in whether you ever get any
> conflicts in the case when merging the faked upstream merge
> in to the packaging branch in the case where your upstream
> version was newer than the other upstream version.

That would be the test_debian_upstream_older() test, and, no, I did not
get any conflicts.

Please find the incremental diff enclosed.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

James Westby wrote:
> Excerpts from Muharem Hrnjadovic's message of Fri Aug 21 12:51:08 UTC 2009:
>>> 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 <source-packaging-branch>"
>>           command, resolve any ensuing packaging branch conflicts
>>           and commit once satisfied with the changes.
>> }}}
> 
> For starters this is bit verbose, we can't really format it like that
> in an error message, so it needs to be a couple of sentences.
> 
> I don't think they need to know that a new "shared upstream" was
> created, you can just say that there are conflicts but they are
> not finished.
> 
> Should we direct them to "bzr merge-package" again? It becomes
> equivalent, but I think it's better to not confuse them with
> when they can use "bzr merge" and when they can't.
> 
> You don't need to tell them what to do after they've run that again,
> it can just tell them when it is done.
> 
> Would it be worth saying that if they don't like what they see
> they can just "bzr revert" to get back to where they started?

Hello James,

thank you very much for your suggestions above. They are all good. I
hope the following text is more suitable.

> 
> Looking at the incremental diff the major issue I see remaining
> is that you lock and unlock the tree, only to lock it again.
> Lock it once, and hold that lock until you don't reference the
> tree or anything that makes use of it any more.

Done.

That would be the test_debian_upstream_older() test, and, no, I did not
get any conflicts.

Please find the incremental diff enclosed.

Best regards

-- 
Muharem Hrnjadovic <muharem@canonical.com>
Public key id   : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F  5602 219F 6B60 B2BB FCFC

merge-package-inc.diff

Revision history for this message

James Westby (james-w) wrote on 2009-08-24:

Excerpts from Muharem Hrnjadovic's message of Mon Aug 24 10:57:08 UTC 2009:
> thank you very much for your suggestions above. They are all good. I
> hope the following text is more suitable.
>
> {{{
> The upstream branches for the merge source and target have diverged.
> Unfortunately, the attempt to fix this problem resulted in conflicts.
> Please resolve these and re-run the "merge-package" command to finish.
> Alternatively, you can restore the original merge target state by
> running "bzr revert".
> }}}

That's better. I think "resolve these and commit" is more explicit, and
I would prefer something other than "original merge target state", as
that sounds a bit jargonish, perhaps 'Alternatively, until you commit
you can use "bzr revert" to restore the state of the unmerged branch.'

It's not as clear what is going on, but I think that makes it easier
to understand :-)

> > Writing this mail also interested me in whether you ever get any
> > conflicts in the case when merging the faked upstream merge
> > in to the packaging branch in the case where your upstream
> > version was newer than the other upstream version.
>
> That would be the test_debian_upstream_older() test, and, no, I did not
> get any conflicts.

Sure, there's one test, and I wouldn't expect any conflicts in that
test, I was thinking more of testing other situations though. Not
as unit tests, but one-off testing to investigate whether you can
provoke conflicts. If it is possible then we may want to investigate
ways to avoid that.

> Please find the incremental diff enclosed.

I'll review this shortly.

Thanks,

James

lp:~al-maisan/bzr-builddeb/merge-package updated on 2009-08-24

408. By Muharem Hrnjadovic on 2009-08-24: Clarified error message in accordance with James' suggestion.

Revision history for this message

James Westby (james-w) wrote on 2009-08-24:

Excerpts from Muharem Hrnjadovic's message of Mon Aug 24 10:57:08 UTC 2009:
> Please find the incremental diff enclosed.

My only quibble with this is the wrapping of the exception message. Please
make it wrap at <80 chars. You can make it resolve to a single string though.

   _fmt = ('...............................................'
           '...............................................'
           '...............................................')

Then I think we should be good to go.

There is documentation needed, but we can get this landed first.

Thanks,

James

lp:~al-maisan/bzr-builddeb/merge-package updated on 2009-08-24

409. By Muharem Hrnjadovic on 2009-08-24: Better formating of error string.

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2009-08-24:

James Westby wrote:
> Excerpts from Muharem Hrnjadovic's message of Mon Aug 24 10:57:08 UTC 2009:
>> Please find the incremental diff enclosed.
>
> My only quibble with this is the wrapping of the exception message. Please
> make it wrap at <80 chars. You can make it resolve to a single string though.
>
> _fmt = ('...............................................'
> '...............................................'
> '...............................................')
>
> Then I think we should be good to go.

done.

> There is documentation needed, but we can get this landed first.

I see. Please point me to an example if available.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2009-08-24:

James Westby wrote:
> Excerpts from Muharem Hrnjadovic's message of Mon Aug 24 10:57:08 UTC 2009:
>> thank you very much for your suggestions above. They are all good. I
>> hope the following text is more suitable.
>>
>> {{{
>> The upstream branches for the merge source and target have diverged.
>> Unfortunately, the attempt to fix this problem resulted in conflicts.
>> Please resolve these and re-run the "merge-package" command to finish.
>> Alternatively, you can restore the original merge target state by
>> running "bzr revert".
>> }}}
>
> That's better. I think "resolve these and commit" is more explicit, and
> I would prefer something other than "original merge target state", as
> that sounds a bit jargonish, perhaps 'Alternatively, until you commit
> you can use "bzr revert" to restore the state of the unmerged branch.'
>
> It's not as clear what is going on, but I think that makes it easier
> to understand :-)

Thank you very much for your help with this :)

>>> Writing this mail also interested me in whether you ever get any
>>> conflicts in the case when merging the faked upstream merge
>>> in to the packaging branch in the case where your upstream
>>> version was newer than the other upstream version.
>> That would be the test_debian_upstream_older() test, and, no, I did not
>> get any conflicts.
>
> Sure, there's one test, and I wouldn't expect any conflicts in that
> test, I was thinking more of testing other situations though. Not
> as unit tests, but one-off testing to investigate whether you can
> provoke conflicts. If it is possible then we may want to investigate
> ways to avoid that.

I can think of a way to set up such a situation. There would need to
be a file that's shared and modified in all four branches: source
upstream, source packaging, target upstream and target packaging.
Furthermore the target upstream branch would have to be more recent one.

I will try to come up with this.

[..]

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

James Westby wrote:
> Excerpts from Muharem Hrnjadovic's message of Mon Aug 24 10:57:08 UTC 2009:
>> thank you very much for your suggestions above. They are all good. I
>> hope the following text is more suitable.
>>
>> {{{
>> The upstream branches for the merge source and target have diverged.
>> Unfortunately, the attempt to fix this problem resulted in conflicts.
>> Please resolve these and re-run the "merge-package" command to finish.
>> Alternatively, you can restore the original merge target state by
>> running "bzr revert".
>> }}}
> 
> That's better. I think "resolve these and commit" is more explicit, and
> I would prefer something other than "original merge target state", as
> that sounds a bit jargonish, perhaps 'Alternatively, until you commit
> you can use "bzr revert" to restore the state of the unmerged branch.'
> 
> It's not as clear what is going on, but I think that makes it easier
> to understand :-)

Thank you very much for your help with this :)

>>> Writing this mail also interested me in whether you ever get any
>>> conflicts in the case when merging the faked upstream merge
>>> in to the packaging branch in the case where your upstream
>>> version was newer than the other upstream version.
>> That would be the test_debian_upstream_older() test, and, no, I did not
>> get any conflicts.
> 
> Sure, there's one test, and I wouldn't expect any conflicts in that
> test, I was thinking more of testing other situations though. Not
> as unit tests, but one-off testing to investigate whether you can
> provoke conflicts. If it is possible then we may want to investigate
> ways to avoid that.

I will try to come up with this.

[..]

Best regards

-- 
Muharem Hrnjadovic <muharem@canonical.com>
Public key id   : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F  5602 219F 6B60 B2BB FCFC

Revision history for this message

James Westby (james-w) wrote on 2009-08-24:

On Mon Aug 24 16:30:19 UTC 2009 Muharem Hrnjadovic wrote:
> > There is documentation needed, but we can get this landed first.
>
> I see. Please point me to an example if available.

Check the doc/user_manual/ directory. We will also want to put a quick
recipe type thing on https://wiki.ubuntu.com/DistributedDevelopment/Documentation
as well.

One thing jumped to mind as well, perhaps there are some options of "merge"
we should support in "merge-package"? We can add these later if needed.

Thanks,

James

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2009-08-24:

James Westby wrote:
> On Mon Aug 24 16:30:19 UTC 2009 Muharem Hrnjadovic wrote:
>>> There is documentation needed, but we can get this landed first.
>> I see. Please point me to an example if available.
>
> Check the doc/user_manual/ directory. We will also want to put a quick
> recipe type thing on https://wiki.ubuntu.com/DistributedDevelopment/Documentation
> as well.

Thanks, I will take a look.

> One thing jumped to mind as well, perhaps there are some options of "merge"
> we should support in "merge-package"? We can add these later if needed.

Hey, that's a good idea :)

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

Revision history for this message

James Westby (james-w) wrote on 2009-08-24:

Looks good, thanks for all your work.

James

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

James Westby

Jelmer Vernooij

Muharem Hrnjadovic

1	=== modified file 'merge_package.py'
2	--- merge_package.py 2009-08-19 09:19:38 +0000
3	+++ merge_package.py 2009-08-19 10:06:11 +0000
4	@@ -89,7 +89,8 @@
5	def _upstream_version_data(source, target):
6	"""Most recent upstream versions/revision IDs of the merge source/target.
7
8	- Please note: both packaing branches must have been read-locked beforehand.
9	+ Please note: both packaging branches must have been read-locked
10	+ beforehand.
11
12	:param source: The merge source branch.
13	:param target: The merge target branch.
14
15	=== modified file 'tests/test_merge_package.py'
16	--- tests/test_merge_package.py 2009-08-19 09:44:38 +0000
17	+++ tests/test_merge_package.py 2009-08-19 10:08:00 +0000
18	@@ -19,13 +19,10 @@
19	# along with bzr-builddeb; if not, write to the Free Software
20	# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
21
22	-import os
23	-import random
24	import string
25	import unittest
26
27	from bzrlib.errors import ConflictsInTree
28	-from bzrlib.merge import WeaveMerger
29	from bzrlib.tests import TestCaseWithTransport
30
31	from bzrlib.plugins.builddeb import merge_package as MP
32	@@ -474,8 +471,7 @@
33	for version, paths, utree, urevid in vdata:
34	msg = ''
35	if utree is not None:
36	- tree.merge_from_branch(
37	- utree.branch, to_revision=urevid, merge_type=WeaveMerger)
38	+ tree.merge_from_branch(utree.branch, to_revision=urevid)
39	utree.branch.tags.merge_to(tree.branch.tags)
40	if urevid is not None:
41	msg += 'Merged tree %s\|%s. ' % (tree_nick(utree), urevid)
42	@@ -491,7 +487,6 @@
43
44
45	if __name__ == '__main__':
46	- # unittest.main()
47	suite = unittest.TestLoader().loadTestsFromTestCase(MergePackageTests)
48	unittest.TextTestRunner(verbosity=2).run(suite)
49

bzr-builddeb

Merge lp:~al-maisan/bzr-builddeb/merge-package into lp:~bzr-builddeb-hackers/bzr-builddeb/trunk-old

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file '__init__.py'
 --- __init__.py	2009-08-06 09:59:34 +0000
 +++ __init__.py	2009-08-19 11:39:16 +0000
@@ -40,7 +40,7 @@
          "import_dsc": [],
          "bd_do": [],
          "mark_uploaded": [],
--        "merge_package": ["mp"]
++        "merge_package": []
+         }
  for command, aliases in commands.iteritems():
 === modified file 'errors.py'
 --- errors.py	2009-04-16 09:30:49 +0000
 +++ errors.py	2009-08-19 14:08:39 +0000
@@ -175,3 +175,22 @@
      def __init__(self, error):
          BzrError.__init__(self, error=error)
++
++
++class SharedUpstreamConflictsWithTargetPackaging(BzrError):
++    _fmt = ('''\
++        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 <source-packaging-branch>"
++              command, resolve any ensuing packaging branch conflicts
++              and commit once satisfied with the changes.
++    ''')
 === modified file 'import_dsc.py'
 --- import_dsc.py	2009-08-19 08:58:47 +0000
 +++ import_dsc.py	2009-08-19 11:43:09 +0000
@@ -1575,6 +1575,7 @@
          # tree for it.
          # TODO: should stack rather than trying to use the repository,
          # as that will be more efficient.
++        # TODO: remove the _extract_upstream_tree alias below.
          to_location = os.path.join(basedir, "upstream")
          dir_to = self.branch.bzrdir.sprout(to_location,
                  revision_id=upstream_tip,
@@ -1582,12 +1583,7 @@
          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)
++    _extract_upstream_tree = extract_upstream_tree
      def _create_empty_upstream_tree(self, basedir):
          to_location = os.path.join(basedir, "upstream")
@@ -1622,11 +1618,11 @@
              shutil.rmtree(tempdir)
              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):
++        # TODO: remove the _revid_of_upstream_version_from_branch alias below.
          assert isinstance(version, str)
          tag_name = self.upstream_tag_name(version)
          if self._has_version(self.branch, tag_name):
@@ -1640,6 +1636,8 @@
          tag_name = self.upstream_tag_name(version)
          return self.branch.tags.lookup_tag(tag_name)
++    _revid_of_upstream_version_from_branch = revid_of_upstream_version_from_branch
++
      def merge_upstream(self, tarball_filename, version, previous_version,
              upstream_branch=None, upstream_revision=None, merge_type=None):
          assert self.upstream_branch is None, \
 === modified file 'merge_package.py'
 --- merge_package.py	2009-08-19 09:19:38 +0000
 +++ merge_package.py	2009-08-19 16:31:25 +0000
@@ -30,37 +30,7 @@
  from bzrlib import errors
  from bzrlib.plugins.builddeb.import_dsc import DistributionBranch
--
--
--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")
--
--
--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
++from bzrlib.plugins.builddeb.util import find_changelog
  def _latest_version(branch):
@@ -68,28 +38,16 @@
      :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)
++    changelog, _ignore = find_changelog(branch.basis_tree(), False)
++
++    return changelog.version
  def _upstream_version_data(source, target):
      """Most recent upstream versions/revision IDs of the merge source/target.
--    Please note: both packaing branches must have been read-locked beforehand.
++    Please note: both packaging branches must have been read-locked
++    beforehand.
      :param source: The merge source branch.
      :param target: The merge target branch.
@@ -150,49 +108,48 @@
      t_upstream_reverted = False
      target = tree.branch
++    source.lock_read()
      try:
--        source.lock_read()
--        target.lock_read()
--        upstream_vdata = _upstream_version_data(source, target)
--        # Did the upstream branches of the merge source and target diverge?
--        revids = [vdata[1] for vdata in upstream_vdata]
--        graph = source.repository.get_graph(target.repository)
--        upstreams_diverged = (len(graph.heads(revids)) > 1)
++        tree.lock_read()
++        try:
++            # "Unpack" the upstream versions and revision ids for the merge
++            # source and target branch respectively.
++            [(us_ver, us_revid), (ut_ver, ut_revid)] = _upstream_version_data(source, target)
++
++            # Did the upstream branches of the merge source/target diverge?
++            graph = source.repository.get_graph(target.repository)
++            upstreams_diverged = (len(graph.heads([us_revid, ut_revid])) > 1)
++        finally:
++            tree.unlock()
      finally:
          source.unlock()
--        target.unlock()
      if not upstreams_diverged:
          return (upstreams_diverged, t_upstream_reverted)
--    # "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
--
      # Instantiate a `DistributionBranch` object for the merge target
      # (packaging) branch.
      db = DistributionBranch(tree.branch, tree.branch)
      tempdir = tempfile.mkdtemp(dir=os.path.join(tree.basedir, '..'))
      # Extract the merge target's upstream tree into a temporary directory.
--    db.extract_upstream_tree(utarget_revid, tempdir)
++    db.extract_upstream_tree(ut_revid, tempdir)
      tmp_target_upstream_tree = db.upstream_tree
      # 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.
++    tmp_target_upstream_tree.lock_write()
      try:
--        tmp_target_upstream_tree.lock_write()
--
--        if usource_v > utarget_v:
++        if us_ver > ut_ver:
              # The source upstream tree is more recent and the temporary
              # target tree needs to be reshaped to match it.
              tmp_target_upstream_tree.revert(
--                None, source.repository.revision_tree(usource_revid))
++                None, source.repository.revision_tree(us_revid))
              t_upstream_reverted = True
          tmp_target_upstream_tree.set_parent_ids(
--            (utarget_revid, usource_revid))
++            (ut_revid, us_revid))
          tmp_target_upstream_tree.commit(
              'Consolidated upstream tree for merging into target branch')
@@ -201,13 +158,13 @@
      # Merge shared upstream parent into the target merge branch. This creates
      # revison L in the digram above.
++    tree.lock_write()
      try:
--        tree.lock_write()
--        try:
--            tree.merge_from_branch(tmp_target_upstream_tree.branch)
--            tree.commit('Merging source packaging branch in to target.')
--        except ConflictsInTree:
--            raise SourceUpstreamConflictsWithTargetPackaging()
++        conflicts = tree.merge_from_branch(tmp_target_upstream_tree.branch)
++        if conflicts > 0:
++            raise errors.SharedUpstreamConflictsWithTargetPackaging()
++        else:
++            tree.commit('Merging shared upstream rev in to target branch.')
      finally:
          tree.unlock()
 === modified file 'tests/test_merge_package.py'
 --- tests/test_merge_package.py	2009-08-19 09:44:38 +0000
 +++ tests/test_merge_package.py	2009-08-21 12:40:48 +0000
@@ -19,16 +19,16 @@
  #    along with bzr-builddeb; if not, write to the Free Software
  #    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
--import os
--import random
  import string
  import unittest
++from debian_bundle.changelog import Version
++
  from bzrlib.errors import ConflictsInTree
--from bzrlib.merge import WeaveMerger
  from bzrlib.tests import TestCaseWithTransport
  from bzrlib.plugins.builddeb import merge_package as MP
++from bzrlib.plugins.builddeb.import_dsc import DistributionBranch
  _Debian_changelog = '''\
  ipsec-tools (%s) unstable; urgency=high
@@ -52,29 +52,31 @@
  def _prepend_log(text, path):
      content = open(path).read()
      fh = open(path, 'wb')
--    fh.write(text+content)
--    fh.close()
++    try:
++        fh.write(text+content)
++    finally:
++        fh.close()
  class MergePackageTests(TestCaseWithTransport):
      def test_latest_upstream_versions(self):
          """Check correctness of upstream version computation."""
--        ubup_o, debp_n = self._setup_debian_upstrem_newer()
++        ubup_o, debp_n, _ubuu, _debu = self._setup_debian_upstream_newer()
          # Ubuntu upstream.
          self.assertEquals(
--            MP._latest_version(ubup_o.branch).upstream_version, '1.1.2')
++            MP._latest_version(ubup_o).upstream_version, '1.1.2')
          # Debian upstream.
          self.assertEquals(
--            MP._latest_version(debp_n.branch).upstream_version, '2.0')
++            MP._latest_version(debp_n).upstream_version, '2.0')
          ubuntup, debianp = self._setup_upstreams_not_diverged()
          # Ubuntu upstream.
          self.assertEquals(
--            MP._latest_version(ubuntup.branch).upstream_version, '1.4')
++            MP._latest_version(ubuntup).upstream_version, '1.4')
          # Debian upstream.
          self.assertEquals(
--            MP._latest_version(debianp.branch).upstream_version, '2.2')
++            MP._latest_version(debianp).upstream_version, '2.2')
      def test_debian_upstream_newer(self):
          """Diverging upstreams (debian newer) don't cause merge conflicts.
@@ -88,23 +90,39 @@
          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()
++        ubup, debp, ubuu, debu = self._setup_debian_upstream_newer()
          # Attempt a plain merge first.
--        conflicts = ubup_o.merge_from_branch(
--            debp_n.branch, to_revision=self.revid_debp_n_C)
++        conflicts = ubup.merge_from_branch(
++            debp.branch, to_revision=self.revid_debp_n_C)
          # There are two conflicts in the 'c' and the 'debian/changelog' files
          # respectively.
          self.assertEquals(conflicts, 2)
--        conflict_paths = sorted([c.path for c in ubup_o.conflicts()])
++        conflict_paths = sorted([c.path for c in ubup.conflicts()])
          self.assertEquals(conflict_paths, [u'c.moved', u'debian/changelog'])
          # Undo the failed merge.
--        ubup_o.revert()
++        ubup.revert()
++
++        # Check the versions present in the tree with the fixed ancestry.
++        v3 = "1.1.2"
++        v4 = "2.0"
++        db1 = DistributionBranch(ubup.branch, ubup.branch)
++        self.assertEqual(db1.has_upstream_version(v3), True)
++        # This version is in the diverged debian upstream tree and will
++        # hence not be present in the target ubuntu packaging branch.
++        self.assertEqual(db1.has_upstream_version(v4), False)
++
++        # The ubuntu upstream branch tip.
++        ubuu_tip = ubuu.branch.revision_history()[-1]
++        # The debian upstream branch tip.
++        debu_tip = debu.branch.revision_history()[-1]
++        # The ubuntu packaging branch tip.
++        ubup_tip_pre_fix = ubup.branch.revision_history()[-1]
          # The first conflict is resolved by calling fix_ancestry_as_needed().
--        upstreams_diverged, t_upstream_reverted = MP.fix_ancestry_as_needed(ubup_o, debp_n.branch)
++        upstreams_diverged, t_upstream_reverted = MP.fix_ancestry_as_needed(ubup, debp.branch)
          # The ancestry did diverge and needed to be fixed.
          self.assertEquals(upstreams_diverged, True)
@@ -112,13 +130,35 @@
          # source upstream branch since the latter was more recent.
          self.assertEquals(t_upstream_reverted, True)
++        # Check the versions present in the tree with the fixed ancestry.
++        db2 = DistributionBranch(ubup.branch, ubup.branch)
++        self.assertEqual(db2.has_upstream_version(v3), True)
++        # The ancestry has been fixed and the missing debian upstream
++        # version should now be present in the target ubuntu packaging
++        # branch.
++        self.assertEqual(db2.has_upstream_version(v4), True)
++
++        # Now let's take a look at the fixed ubuntu packaging branch.
++        ubup_tip_post_fix = ubup.branch.revision_history()[-1]
++        ubup_parents_post_fix = ubup.branch.repository.revision_tree(ubup_tip_post_fix).get_parent_ids()
++
++        # The tip of the fixed ubuntu packaging branch has 2 parents.
++        self.assertEquals(len(ubup_parents_post_fix), 2)
++
++        # The left parent is the packaging branch tip before fixing.
++        self.assertEquals(ubup_parents_post_fix[0], ubup_tip_pre_fix)
++
++        # The right parent is derived from a merge
++        ubup_parents_sharedupstream = ubup.branch.repository.revision_tree(ubup_parents_post_fix[1]).get_parent_ids()
++        self.assertEquals(ubup_parents_sharedupstream, [ubuu_tip, debu_tip])
++
          # Try merging again.
--        conflicts = ubup_o.merge_from_branch(
--            debp_n.branch, to_revision=self.revid_debp_n_C)
++        conflicts = ubup.merge_from_branch(
++            debp.branch, to_revision=self.revid_debp_n_C)
          # And, voila, only the packaging branch conflict remains.
          self.assertEquals(conflicts, 1)
--        conflict_paths = sorted([c.path for c in ubup_o.conflicts()])
++        conflict_paths = sorted([c.path for c in ubup.conflicts()])
          self.assertEquals(conflict_paths, [u'debian/changelog'])
      def test_debian_upstream_older(self):
@@ -208,9 +248,9 @@
          conflict_paths = sorted([c.path for c in ubuntup.conflicts()])
          self.assertEquals(conflict_paths, [u'debian/changelog'])
--    def _setup_debian_upstrem_newer(self):
++    def _setup_debian_upstream_newer(self):
          """
--        Set up the following test configuration (debian upstrem newer).
++        Set up the following test configuration (debian upstream newer).
          debian-upstream                 ,------------------H
                             A-----------B                    \
@@ -280,11 +320,11 @@
          self._setup_branch(name, vdata, ubup_o, 'u')
          # Return the ubuntu and the debian packaging branches.
--        return (ubup_o, debp_n)
++        return (ubup_o, debp_n, ubuu_o, debu_n)
      def _setup_debian_upstream_older(self):
          """
--        Set up the following test configuration (debian upstrem older).
++        Set up the following test configuration (debian upstream older).
          debian-upstream                 ,----H-------------.
                             A-----------B                    \
@@ -434,6 +474,9 @@
          if tree is None:
              tree = self.make_branch_and_tree(name)
++        tree.lock_write()
++        self.addCleanup(tree.unlock)
++
          def revid_name(vid):
              return 'revid_%s_%s' % (name.replace('-', '_'), vid)
@@ -464,7 +507,7 @@
                  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))
++            revid = tree.commit('%s: %s' % (vid, msg))
              setattr(self, revid_name(vid), revid)
              tree.branch.tags.set_tag(version, revid)
@@ -474,8 +517,7 @@
          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)
++                tree.merge_from_branch(utree.branch, to_revision=urevid)
                  utree.branch.tags.merge_to(tree.branch.tags)
                  if urevid is not None:
                      msg += 'Merged tree %s|%s. ' % (tree_nick(utree), urevid)
@@ -491,7 +533,5 @@
  if __name__ == '__main__':
--    # unittest.main()
      suite = unittest.TestLoader().loadTestsFromTestCase(MergePackageTests)
      unittest.TextTestRunner(verbosity=2).run(suite)
--

 === modified file '__init__.py'
 --- __init__.py	2009-07-26 15:51:02 +0000
 +++ __init__.py	2009-08-19 11:39:16 +0000
@@ -39,7 +39,8 @@
          "merge_upstream": ["mu"],
          "import_dsc": [],
          "bd_do": [],
--        "mark_uploaded": []
++        "mark_uploaded": [],
++        "merge_package": []
+         }
  for command, aliases in commands.iteritems():
 === modified file 'cmds.py'
 --- cmds.py	2009-07-26 18:21:49 +0000
 +++ cmds.py	2009-08-19 09:01:30 +0000
@@ -69,6 +69,7 @@
          DscCache,
          DscComp,
+         )
++from bzrlib.plugins.builddeb.merge_package import fix_ancestry_as_needed
  from bzrlib.plugins.builddeb.source_distiller import (
          FullSourceDistiller,
          MergeModeDistiller,
@@ -719,9 +720,9 @@
                                  "the previous upstream version, %s, in the "
                                  "branch: %s" % (last_version,
                                      db.upstream_tag_name(last_version)))
--                    upstream_tip = db._revid_of_upstream_version_from_branch(
++                    upstream_tip = db.revid_of_upstream_version_from_branch(
                              last_version)
--                    db._extract_upstream_tree(upstream_tip, tempdir)
++                    db.extract_upstream_tree(upstream_tip, tempdir)
                  else:
                      db._create_empty_upstream_tree(tempdir)
                  self.import_many(db, files_list, orig_target)
@@ -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 'errors.py'
 --- errors.py	2009-04-16 09:30:49 +0000
 +++ errors.py	2009-08-19 14:08:39 +0000
@@ -175,3 +175,22 @@
      def __init__(self, error):
          BzrError.__init__(self, error=error)
++
++
++class SharedUpstreamConflictsWithTargetPackaging(BzrError):
++    _fmt = ('''\
++        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 <source-packaging-branch>"
++              command, resolve any ensuing packaging branch conflicts
++              and commit once satisfied with the changes.
++    ''')
 === modified file 'import_dsc.py'
 --- import_dsc.py	2009-07-26 16:44:17 +0000
 +++ import_dsc.py	2009-08-19 11:43:09 +0000
@@ -1570,11 +1570,12 @@
          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,
          # as that will be more efficient.
++        # TODO: remove the _extract_upstream_tree alias below.
          to_location = os.path.join(basedir, "upstream")
          dir_to = self.branch.bzrdir.sprout(to_location,
                  revision_id=upstream_tip,
@@ -1582,6 +1583,8 @@
          self.upstream_tree = dir_to.open_workingtree()
          self.upstream_branch = self.upstream_tree.branch
++    _extract_upstream_tree = extract_upstream_tree
++
      def _create_empty_upstream_tree(self, basedir):
          to_location = os.path.join(basedir, "upstream")
          to_transport = get_transport(to_location)
@@ -1615,7 +1618,11 @@
              shutil.rmtree(tempdir)
              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):
++        # TODO: remove the _revid_of_upstream_version_from_branch alias below.
          assert isinstance(version, str)
          tag_name = self.upstream_tag_name(version)
          if self._has_version(self.branch, tag_name):
@@ -1629,6 +1636,8 @@
          tag_name = self.upstream_tag_name(version)
          return self.branch.tags.lookup_tag(tag_name)
++    _revid_of_upstream_version_from_branch = revid_of_upstream_version_from_branch
++
      def merge_upstream(self, tarball_filename, version, previous_version,
              upstream_branch=None, upstream_revision=None, merge_type=None):
          assert self.upstream_branch is None, \
@@ -1639,14 +1648,14 @@
              if previous_version is not None:
                  if self.has_upstream_version_in_packaging_branch(
                          previous_version.upstream_version):
--                    upstream_tip = self._revid_of_upstream_version_from_branch(
++                    upstream_tip = self.revid_of_upstream_version_from_branch(
                              previous_version.upstream_version)
--                    self._extract_upstream_tree(upstream_tip, tempdir)
++                    self.extract_upstream_tree(upstream_tip, tempdir)
                  elif (upstream_branch is not None and
                        previous_upstream_revision is not None):
                      upstream_tip = RevisionSpec.from_string(previous_upstream_revision).as_revision_id(upstream_branch)
                      assert isinstance(upstream_tip, str)
--                    self._extract_upstream_tree(upstream_tip, tempdir)
++                    self.extract_upstream_tree(upstream_tip, tempdir)
                  else:
                      raise BzrCommandError("Unable to find the tag for the "
                              "previous upstream version, %s, in the branch: "
 === added file 'merge_package.py'
 --- merge_package.py	1970-01-01 00:00:00 +0000
 +++ merge_package.py	2009-08-19 16:31:25 +0000
@@ -0,0 +1,171 @@
++#    merge_package.py -- The plugin for bzr
++#    Copyright (C) 2009 Canonical Ltd.
++#
++#    :Author: Muharem Hrnjadovic <muharem@ubuntu.com>
++#
++#    This file is part of bzr-builddeb.
++#
++#    bzr-builddeb is free software; you can redistribute it and/or modify
++#    it under the terms of the GNU General Public License as published by
++#    the Free Software Foundation; either version 2 of the License, or
++#    (at your option) any later version.
++#
++#    bzr-builddeb is distributed in the hope that it will be useful,
++#    but WITHOUT ANY WARRANTY; without even the implied warranty of
++#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++#    GNU General Public License for more details.
++#
++#    You should have received a copy of the GNU General Public License
++#    along with bzr-builddeb; if not, write to the Free Software
++#    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
++#
++
++import os
++import re
++import sys
++import tempfile
++
++from debian_bundle.changelog import Version
++
++from bzrlib import errors
++
++from bzrlib.plugins.builddeb.import_dsc import DistributionBranch
++from bzrlib.plugins.builddeb.util import find_changelog
++
++
++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.
++    """
++    changelog, _ignore = find_changelog(branch.basis_tree(), False)
++
++    return changelog.version
++
++
++def _upstream_version_data(source, target):
++    """Most recent upstream versions/revision IDs of the merge source/target.
++
++    Please note: both packaging branches must have been read-locked
++    beforehand.
++
++    :param source: The merge source branch.
++    :param target: The merge target branch.
++    """
++    results = list()
++    for branch in (source, target):
++        db = DistributionBranch(branch, branch)
++        uver = _latest_version(branch).upstream_version
++        results.append((uver, db.revid_of_upstream_version_from_branch(uver)))
++
++    return results
++
++
++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.
++    """
++    upstreams_diverged = False
++    t_upstream_reverted = False
++    target = tree.branch
++
++    source.lock_read()
++    try:
++        tree.lock_read()
++        try:
++            # "Unpack" the upstream versions and revision ids for the merge
++            # source and target branch respectively.
++            [(us_ver, us_revid), (ut_ver, ut_revid)] = _upstream_version_data(source, target)
++
++            # Did the upstream branches of the merge source/target diverge?
++            graph = source.repository.get_graph(target.repository)
++            upstreams_diverged = (len(graph.heads([us_revid, ut_revid])) > 1)
++        finally:
++            tree.unlock()
++    finally:
++        source.unlock()
++
++    if not upstreams_diverged:
++        return (upstreams_diverged, t_upstream_reverted)
++
++    # Instantiate a `DistributionBranch` object for the merge target
++    # (packaging) branch.
++    db = DistributionBranch(tree.branch, tree.branch)
++    tempdir = tempfile.mkdtemp(dir=os.path.join(tree.basedir, '..'))
++
++    # Extract the merge target's upstream tree into a temporary directory.
++    db.extract_upstream_tree(ut_revid, tempdir)
++    tmp_target_upstream_tree = db.upstream_tree
++
++    # 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.
++    tmp_target_upstream_tree.lock_write()
++    try:
++        if us_ver > ut_ver:
++            # The source upstream tree is more recent and the temporary
++            # target tree needs to be reshaped to match it.
++            tmp_target_upstream_tree.revert(
++                None, source.repository.revision_tree(us_revid))
++            t_upstream_reverted = True
++
++        tmp_target_upstream_tree.set_parent_ids(
++            (ut_revid, us_revid))
++
++        tmp_target_upstream_tree.commit(
++            'Consolidated upstream tree for merging into target branch')
++    finally:
++        tmp_target_upstream_tree.unlock()
++
++    # Merge shared upstream parent into the target merge branch. This creates
++    # revison L in the digram above.
++    tree.lock_write()
++    try:
++        conflicts = tree.merge_from_branch(tmp_target_upstream_tree.branch)
++        if conflicts > 0:
++            raise errors.SharedUpstreamConflictsWithTargetPackaging()
++        else:
++            tree.commit('Merging shared upstream rev in to target branch.')
++    finally:
++        tree.unlock()
++
++    return (upstreams_diverged, t_upstream_reverted)
 === modified file 'tests/__init__.py'
 --- tests/__init__.py	2009-07-04 20:45:01 +0000
 +++ tests/__init__.py	2009-08-19 09:55:06 +0000
@@ -118,6 +118,7 @@
              'test_config',
              'test_hooks',
              'test_import_dsc',
++            'test_merge_package',
              'test_merge_upstream',
              'test_repack_tarball_extra',
              'test_revspec',
 === 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-21 12:40:48 +0000
@@ -0,0 +1,537 @@
++#!/usr/bin/env python
++# -*- coding: iso-8859-15 -*-
++#    test_merge_package.py -- Merge packaging branches, fix ancestry as needed.
++#    Copyright (C) 2008 Canonical Ltd.
++#
++#    This file is part of bzr-builddeb.
++#
++#    bzr-builddeb is free software; you can redistribute it and/or modify
++#    it under the terms of the GNU General Public License as published by
++#    the Free Software Foundation; either version 2 of the License, or
++#    (at your option) any later version.
++#
++#    bzr-builddeb is distributed in the hope that it will be useful,
++#    but WITHOUT ANY WARRANTY; without even the implied warranty of
++#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++#    GNU General Public License for more details.
++#
++#    You should have received a copy of the GNU General Public License
++#    along with bzr-builddeb; if not, write to the Free Software
++#    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
++
++import string
++import unittest
++
++from debian_bundle.changelog import Version
++
++from bzrlib.errors import ConflictsInTree
++from bzrlib.tests import TestCaseWithTransport
++
++from bzrlib.plugins.builddeb import merge_package as MP
++from bzrlib.plugins.builddeb.import_dsc import DistributionBranch
++
++_Debian_changelog = '''\
++ipsec-tools (%s) unstable; urgency=high
++
++  * debian packaging -- %s
++
++ -- Nico Golde <nion@debian.org>  Tue, %02d May 2009 13:26:14 +0200
++
++'''
++
++_Ubuntu_changelog = '''\
++ipsec-tools (%s) karmic; urgency=low
++
++  * ubuntu packaging -- %s
++
++ -- Jamie Strandboge <jamie@ubuntu.com>  Fri, %02d Jul 2009 13:24:17 -0500
++
++'''
++
++
++def _prepend_log(text, path):
++    content = open(path).read()
++    fh = open(path, 'wb')
++    try:
++        fh.write(text+content)
++    finally:
++        fh.close()
++
++
++class MergePackageTests(TestCaseWithTransport):
++
++    def test_latest_upstream_versions(self):
++        """Check correctness of upstream version computation."""
++        ubup_o, debp_n, _ubuu, _debu = self._setup_debian_upstream_newer()
++        # Ubuntu upstream.
++        self.assertEquals(
++            MP._latest_version(ubup_o).upstream_version, '1.1.2')
++        # Debian upstream.
++        self.assertEquals(
++            MP._latest_version(debp_n).upstream_version, '2.0')
++
++        ubuntup, debianp = self._setup_upstreams_not_diverged()
++        # Ubuntu upstream.
++        self.assertEquals(
++            MP._latest_version(ubuntup).upstream_version, '1.4')
++        # Debian upstream.
++        self.assertEquals(
++            MP._latest_version(debianp).upstream_version, '2.2')
++
++    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, debp, ubuu, debu = self._setup_debian_upstream_newer()
++
++        # Attempt a plain merge first.
++        conflicts = ubup.merge_from_branch(
++            debp.branch, to_revision=self.revid_debp_n_C)
++
++        # There are two conflicts in the 'c' and the 'debian/changelog' files
++        # respectively.
++        self.assertEquals(conflicts, 2)
++        conflict_paths = sorted([c.path for c in ubup.conflicts()])
++        self.assertEquals(conflict_paths, [u'c.moved', u'debian/changelog'])
++
++        # Undo the failed merge.
++        ubup.revert()
++
++        # Check the versions present in the tree with the fixed ancestry.
++        v3 = "1.1.2"
++        v4 = "2.0"
++        db1 = DistributionBranch(ubup.branch, ubup.branch)
++        self.assertEqual(db1.has_upstream_version(v3), True)
++        # This version is in the diverged debian upstream tree and will
++        # hence not be present in the target ubuntu packaging branch.
++        self.assertEqual(db1.has_upstream_version(v4), False)
++
++        # The ubuntu upstream branch tip.
++        ubuu_tip = ubuu.branch.revision_history()[-1]
++        # The debian upstream branch tip.
++        debu_tip = debu.branch.revision_history()[-1]
++        # The ubuntu packaging branch tip.
++        ubup_tip_pre_fix = ubup.branch.revision_history()[-1]
++
++        # The first conflict is resolved by calling fix_ancestry_as_needed().
++        upstreams_diverged, t_upstream_reverted = MP.fix_ancestry_as_needed(ubup, debp.branch)
++
++        # The ancestry did diverge and needed to be fixed.
++        self.assertEquals(upstreams_diverged, True)
++        # The (temporary) target upstream branch had to be reverted to the
++        # source upstream branch since the latter was more recent.
++        self.assertEquals(t_upstream_reverted, True)
++
++        # Check the versions present in the tree with the fixed ancestry.
++        db2 = DistributionBranch(ubup.branch, ubup.branch)
++        self.assertEqual(db2.has_upstream_version(v3), True)
++        # The ancestry has been fixed and the missing debian upstream
++        # version should now be present in the target ubuntu packaging
++        # branch.
++        self.assertEqual(db2.has_upstream_version(v4), True)
++
++        # Now let's take a look at the fixed ubuntu packaging branch.
++        ubup_tip_post_fix = ubup.branch.revision_history()[-1]
++        ubup_parents_post_fix = ubup.branch.repository.revision_tree(ubup_tip_post_fix).get_parent_ids()
++
++        # The tip of the fixed ubuntu packaging branch has 2 parents.
++        self.assertEquals(len(ubup_parents_post_fix), 2)
++
++        # The left parent is the packaging branch tip before fixing.
++        self.assertEquals(ubup_parents_post_fix[0], ubup_tip_pre_fix)
++
++        # The right parent is derived from a merge
++        ubup_parents_sharedupstream = ubup.branch.repository.revision_tree(ubup_parents_post_fix[1]).get_parent_ids()
++        self.assertEquals(ubup_parents_sharedupstream, [ubuu_tip, debu_tip])
++
++        # Try merging again.
++        conflicts = ubup.merge_from_branch(
++            debp.branch, to_revision=self.revid_debp_n_C)
++
++        # And, voila, only the packaging branch conflict remains.
++        self.assertEquals(conflicts, 1)
++        conflict_paths = sorted([c.path for c in ubup.conflicts()])
++        self.assertEquals(conflict_paths, [u'debian/changelog'])
++
++    def test_debian_upstream_older(self):
++        """Diverging upstreams (debian older) 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 older in this case.
++        """
++        ubup_n, debp_o = self._setup_debian_upstream_older()
++
++        # Attempt a plain merge first.
++        conflicts = ubup_n.merge_from_branch(
++            debp_o.branch, to_revision=self.revid_debp_o_C)
++
++        # There are two conflicts in the 'c' and the 'debian/changelog' files
++        # respectively.
++        self.assertEquals(conflicts, 2)
++        conflict_paths = sorted([c.path for c in ubup_n.conflicts()])
++        self.assertEquals(conflict_paths, [u'c.moved', u'debian/changelog'])
++
++        # Undo the failed merge.
++        ubup_n.revert()
++
++        # The first conflict is resolved by calling fix_ancestry_as_needed().
++        upstreams_diverged, t_upstream_reverted = MP.fix_ancestry_as_needed(ubup_n, debp_o.branch)
++
++        # The ancestry did diverge and needed to be fixed.
++        self.assertEquals(upstreams_diverged, True)
++        # The target upstream branch was more recent in this case and hence
++        # was not reverted to the source upstream branch.
++        self.assertEquals(t_upstream_reverted, False)
++
++        # Try merging again.
++        conflicts = ubup_n.merge_from_branch(
++            debp_o.branch, to_revision=self.revid_debp_o_C)
++
++        # And, voila, only the packaging branch conflict remains.
++        self.assertEquals(conflicts, 1)
++        conflict_paths = sorted([c.path for c in ubup_n.conflicts()])
++        self.assertEquals(conflict_paths, [u'debian/changelog'])
++
++    def test_upstreams_not_diverged(self):
++        """Non-diverging upstreams result in a normal merge.
++
++        The debian and ubuntu upstream branches will not have diverged
++        this time.
++
++        The packaging branches will have a conflict in 'debian/changelog'.
++        fix_ancestry_as_needed() will return as soon as establishing that
++        the upstreams have not diverged.
++        """
++        ubuntup, debianp = self._setup_upstreams_not_diverged()
++
++        # Attempt a plain merge first.
++        conflicts = ubuntup.merge_from_branch(
++            debianp.branch, to_revision=self.revid_debianp_C)
++
++        # There is only a conflict in the 'debian/changelog' file.
++        self.assertEquals(conflicts, 1)
++        conflict_paths = sorted([c.path for c in ubuntup.conflicts()])
++        self.assertEquals(conflict_paths, [u'debian/changelog'])
++
++        # Undo the failed merge.
++        ubuntup.revert()
++
++        # The conflict is *not* resolved by calling fix_ancestry_as_needed().
++        upstreams_diverged, t_upstream_reverted = MP.fix_ancestry_as_needed(ubuntup, debianp.branch)
++
++        # The ancestry did *not* diverge.
++        self.assertEquals(upstreams_diverged, False)
++        # The upstreams have not diverged, hence no need to fix/revert
++        # either of them.
++        self.assertEquals(t_upstream_reverted, False)
++
++        # Try merging again.
++        conflicts = ubuntup.merge_from_branch(
++            debianp.branch, to_revision=self.revid_debianp_C)
++
++        # The packaging branch conflict we saw above is still there.
++        self.assertEquals(conflicts, 1)
++        conflict_paths = sorted([c.path for c in ubuntup.conflicts()])
++        self.assertEquals(conflict_paths, [u'debian/changelog'])
++
++    def _setup_debian_upstream_newer(self):
++        """
++        Set up the following test configuration (debian upstream newer).
++
++        debian-upstream                 ,------------------H
++                           A-----------B                    \
++        ubuntu-upstream     \           \`-------G           \
++                             \           \        \           \
++        debian-packaging      \ ,---------D--------\-----------J
++                               C                    \
++        ubuntu-packaging        `----E---------------I
++
++        where:
++             - A = 1.0
++             - B = 1.1
++             - H = 2.0
++
++             - G = 1.1.2
++
++             - C = 1.0-1
++             - D = 1.1-1
++             - J = 2.0-1
++
++             - E = 1.0-1ubuntu1
++             - I = 1.1.2-0ubuntu1
++
++        Please note that the debian and ubuntu branches will have a conflict
++        with respect to the file 'c'.
++        """
++        # Set up the debian upstream branch.
++        name = 'debu-n'
++        vdata = [
++            ('upstream-1.0', ('a',), None, None),
++            ('upstream-1.1', ('b',), None, None),
++            ('upstream-2.0', ('c',), None, None),
++            ]
++        debu_n = self._setup_branch(name, vdata)
++
++        # Set up the debian packaging branch.
++        name = 'debp-n'
++        debp_n = self.make_branch_and_tree(name)
++        debp_n.pull(debu_n.branch, stop_revision=self.revid_debu_n_A)
++
++        vdata = [
++            ('1.0-1', ('debian/', 'debian/changelog'), None, None),
++            ('1.1-1', ('o',), debu_n, self.revid_debu_n_B),
++            ('2.0-1', ('p',), debu_n, self.revid_debu_n_C),
++            ]
++        self._setup_branch(name, vdata, debp_n, 'd')
++
++        # Set up the ubuntu upstream branch.
++        name = 'ubuu-o'
++        ubuu_o = debu_n.bzrdir.sprout(
++            name, revision_id=self.revid_debu_n_B).open_workingtree()
++
++        vdata = [
++            ('upstream-1.1.2', ('c',), None, None),
++            ]
++        self._setup_branch(name, vdata, ubuu_o)
++
++        # Set up the ubuntu packaging branch.
++        name = 'ubup-o'
++        ubup_o = debu_n.bzrdir.sprout(
++            name, revision_id=self.revid_debu_n_A).open_workingtree()
++
++        vdata = [
++            ('1.0-1ubuntu1', (), debp_n, self.revid_debp_n_A),
++            ('1.1.2-0ubuntu1', (), ubuu_o, self.revid_ubuu_o_A),
++            ]
++        self._setup_branch(name, vdata, ubup_o, 'u')
++
++        # Return the ubuntu and the debian packaging branches.
++        return (ubup_o, debp_n, ubuu_o, debu_n)
++
++    def _setup_debian_upstream_older(self):
++        """
++        Set up the following test configuration (debian upstream older).
++
++        debian-upstream                 ,----H-------------.
++                           A-----------B                    \
++        ubuntu-upstream     \           \`-----------G       \
++                             \           \            \       \
++        debian-packaging      \ ,---------D------------\-------J
++                               C                        \
++        ubuntu-packaging        `----E-------------------I
++
++        where:
++             - A = 1.0
++             - B = 1.1
++             - H = 1.1.3
++
++             - G = 2.1
++
++             - C = 1.0-1
++             - D = 1.1-1
++             - J = 1.1.3-1
++
++             - E = 1.0-1ubuntu1
++             - I = 2.1-0ubuntu1
++
++        Please note that the debian and ubuntu branches will have a conflict
++        with respect to the file 'c'.
++        """
++        # Set up the debian upstream branch.
++        name = 'debu-o'
++        vdata = [
++            ('upstream-1.0', ('a',), None, None),
++            ('upstream-1.1', ('b',), None, None),
++            ('upstream-1.1.3', ('c',), None, None),
++            ]
++        debu_o = self._setup_branch(name, vdata)
++
++        # Set up the debian packaging branch.
++        name = 'debp-o'
++        debp_o = self.make_branch_and_tree(name)
++        debp_o.pull(debu_o.branch, stop_revision=self.revid_debu_o_A)
++
++        vdata = [
++            ('1.0-1', ('debian/', 'debian/changelog'), None, None),
++            ('1.1-1', ('o',), debu_o, self.revid_debu_o_B),
++            ('1.1.3-1', ('p',), debu_o, self.revid_debu_o_C),
++            ]
++        self._setup_branch(name, vdata, debp_o, 'd')
++
++        # Set up the ubuntu upstream branch.
++        name = 'ubuu-n'
++        ubuu_n = debu_o.bzrdir.sprout(
++            name, revision_id=self.revid_debu_o_B).open_workingtree()
++
++        vdata = [
++            ('upstream-2.1', ('c',), None, None),
++            ]
++        self._setup_branch(name, vdata, ubuu_n)
++
++        # Set up the ubuntu packaging branch.
++        name = 'ubup-n'
++        ubup_n = debu_o.bzrdir.sprout(
++            name, revision_id=self.revid_debu_o_A).open_workingtree()
++
++        vdata = [
++            ('1.0-1ubuntu1', (), debp_o, self.revid_debp_o_A),
++            ('2.1-0ubuntu1', (), ubuu_n, self.revid_ubuu_n_A),
++            ]
++        self._setup_branch(name, vdata, ubup_n, 'u')
++
++        # Return the ubuntu and the debian packaging branches.
++        return (ubup_n, debp_o)
++
++    def _setup_upstreams_not_diverged(self):
++        """
++        Set up a test configuration where the usptreams have not diverged.
++
++        debian-upstream                       .-----G
++                           A-----------B-----H       \
++        ubuntu-upstream     \           \     \       \
++                             \           \     \       \
++        debian-packaging      \ ,---------D-----\-------J
++                               C                 \
++        ubuntu-packaging        `----E------------I
++
++        where:
++             - A = 1.0
++             - B = 1.1
++             - H = 1.4
++
++             - G = 2.2
++
++             - C = 1.0-1
++             - D = 1.1-1
++             - J = 2.2-1
++
++             - E = 1.0-1ubuntu1
++             - I = 1.4-0ubuntu1
++
++        Please note that there's only one shared upstream branch in this case.
++        """
++        # Set up the upstream branch.
++        name = 'upstream'
++        vdata = [
++            ('upstream-1.0', ('a',), None, None),
++            ('upstream-1.1', ('b',), None, None),
++            ('upstream-1.4', ('c',), None, None),
++            ]
++        upstream = self._setup_branch(name, vdata)
++
++        # Set up the debian upstream branch.
++        name = 'dupstream'
++        dupstream = upstream.bzrdir.sprout(name).open_workingtree()
++        vdata = [
++            ('upstream-2.2', (), None, None),
++            ]
++        dupstream = self._setup_branch(name, vdata, dupstream)
++
++        # Set up the debian packaging branch.
++        name = 'debianp'
++        debianp = self.make_branch_and_tree(name)
++        debianp.pull(dupstream.branch, stop_revision=self.revid_upstream_A)
++
++        vdata = [
++            ('1.0-1', ('debian/', 'debian/changelog'), None, None),
++            ('1.1-1', ('o',), dupstream, self.revid_upstream_B),
++            ('2.2-1', ('p',), dupstream, self.revid_dupstream_A),
++            ]
++        self._setup_branch(name, vdata, debianp, 'd')
++
++        # Set up the ubuntu packaging branch.
++        name = 'ubuntup'
++        ubuntup = upstream.bzrdir.sprout(
++            name, revision_id=self.revid_upstream_A).open_workingtree()
++
++        vdata = [
++            ('1.0-1ubuntu1', (), debianp, self.revid_debianp_A),
++            ('1.4-0ubuntu1', (), upstream, self.revid_upstream_C),
++            ]
++        self._setup_branch(name, vdata, ubuntup, 'u')
++
++        # Return the ubuntu and the debian packaging branches.
++        return (ubuntup, debianp)
++
++    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)
++
++        tree.lock_write()
++        self.addCleanup(tree.unlock)
++
++        def revid_name(vid):
++            return 'revid_%s_%s' % (name.replace('-', '_'), vid)
++
++        def add_paths(paths):
++            qpaths = ['%s/%s' % (name, path) for path in paths]
++            self.build_tree(qpaths)
++            tree.add(paths)
++
++        def changelog(vdata, vid):
++            result = ''
++            day = days.pop(0)
++            if isinstance(vdata, tuple):
++                uver, dver = vdata[:2]
++                ucle = _Ubuntu_changelog % (uver, vid, day)
++                dcle = _Debian_changelog % (dver, vid, day)
++                result = ucle + dcle
++            else:
++                if log_format == 'u':
++                    result = _Ubuntu_changelog % (vdata, vid, day)
++                elif log_format == 'd':
++                    result = _Debian_changelog % (vdata, vid, day)
++
++            return result
++
++        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))
++            setattr(self, revid_name(vid), revid)
++            tree.branch.tags.set_tag(version, revid)
++
++        def tree_nick(tree):
++            return str(tree)[1:-1].split('/')[-1]
++
++        for version, paths, utree, urevid in vdata:
++            msg = ''
++            if utree is not None:
++                tree.merge_from_branch(utree.branch, to_revision=urevid)
++                utree.branch.tags.merge_to(tree.branch.tags)
++                if urevid is not None:
++                    msg += 'Merged tree %s|%s. ' % (tree_nick(utree), urevid)
++                else:
++                    msg += 'Merged tree %s. ' % utree
++            if paths is not None:
++                add_paths(paths)
++                msg += 'Added paths: %s. ' % str(paths)
++
++            commit(msg, version)
++
++        return tree
++
++
++if __name__ == '__main__':
++    suite = unittest.TestLoader().loadTestsFromTestCase(MergePackageTests)
++    unittest.TextTestRunner(verbosity=2).run(suite)
 === modified file 'upstream.py'
 --- upstream.py	2009-07-26 18:21:49 +0000
 +++ upstream.py	2009-08-06 08:40:05 +0000
@@ -79,7 +79,7 @@
          db = DistributionBranch(self.branch, None, tree=self.tree)
          if not db.has_upstream_version_in_packaging_branch(version):
              raise PackageVersionNotPresent(package, version, self)
--        revid = db._revid_of_upstream_version_from_branch(version)
++        revid = db.revid_of_upstream_version_from_branch(version)
          if not db.has_pristine_tar_delta(revid):
              raise PackageVersionNotPresent(package, version, self)
          info("Using pristine-tar to reconstruct the needed tarball.")