Merge lp:~spiv/bzr/better-news-merge into lp:bzr

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This patch makes the news_merge plugin more capable. Hopefully the changes to comments and docstrings in the patch explain the changes well enough, but in short it makes it capable of coping with conflicts that span section headings (like 'Bug Fixes'), whereas before it only dealt with conflicts between bullet points within a section. See the comments and code for the details (and limitations and tradeoffs).

With this change I expect it news_merge will handle the vast majority of NEWS merges.

A minor orthogonal change adds a couple of trivial mutters to let you a reader of ~/.bzr.log know when news_merge has been used.

[Robert Collins (lifeless)]

The patch seems to have conflicts and a lot of noise :(

-Rob

[Andrew Bennetts (spiv)]

Oh, I wrote the code against 2.1, but proposed against lp:bzr. Hmm.

[lp:~spiv/bzr/better-news-merge]

4815. By Andrew Bennetts on 2010-03-19

Read section order from template file named in config.

4816. By Andrew Bennetts on 2010-03-19

Add a XXX comment for future improvement.

4817. By Andrew Bennetts on 2010-04-19

First steps towards a better NEWS file parser.

4818. By Andrew Bennetts on 2010-04-20

Rename test.

4819. By Andrew Bennetts on 2010-04-20

Merge lp:bzr.

4820. By Andrew Bennetts on 2010-04-20

Merge object-3way-merge.

4821. By Andrew Bennetts on 2010-04-20

Possibly working news_merge built on a richer structure than just lines.

4822. By Andrew Bennetts on 2010-04-20

Fix some simple bugs.

Unmerged revisions

4822. By Andrew Bennetts on 2010-04-20

Fix some simple bugs.

4821. By Andrew Bennetts on 2010-04-20

Possibly working news_merge built on a richer structure than just lines.

4820. By Andrew Bennetts on 2010-04-20

Merge object-3way-merge.

4819. By Andrew Bennetts on 2010-04-20

Merge lp:bzr.

4818. By Andrew Bennetts on 2010-04-20

Rename test.

4817. By Andrew Bennetts on 2010-04-19

First steps towards a better NEWS file parser.

4816. By Andrew Bennetts on 2010-03-19

Add a XXX comment for future improvement.

4815. By Andrew Bennetts on 2010-03-19

Read section order from template file named in config.

4814. By Andrew Bennetts on 2010-02-12

Teach news_merge to handle conflicts involving section headings as well as bullets.

4813. By Andrew Bennetts on 2010-02-12

Add some simple mutters so that it's easy to tell if news_merge has been triggered.

=== modified file 'NEWS'
--- NEWS	2010-02-12 04:33:05 +0000
+++ NEWS	2010-02-12 12:16:24 +0000
@@ -103,6 +103,7 @@
 * Fix "AttributeError in Inter1and2Helper" during fetch.
   (Martin Pool, #513432)
 
+<<<<<<< TREE
 * Fix ``log`` to better check ancestors even if merged revisions are involved.
   (Vincent Ladeuil, #476293)
 
@@ -145,6 +146,15 @@
 
 Testing
 *******
+=======
+* Ignore ``KeyError`` from ``remove_index`` during ``_abort_write_group``
+  in a pack repository, which can happen harmlessly if the abort occurs during
+  finishing the write group.  Also use ``bzrlib.cleanup`` so that any
+  other errors that occur while aborting the individual packs won't be
+  hidden by secondary failures when removing the corresponding indices.
+  (Andrew Bennetts, #423015)
+  
+>>>>>>> MERGE-SOURCE
 * Using the ``bzrlib.chk_map`` module from within multiple threads at the
   same time was broken due to race conditions with a module level page
   cache. This shows up as a KeyError in the ``bzrlib.lru_cache`` code with
@@ -157,6 +167,30 @@
 * The new ``merge_file_content`` should now be ok with tests to avoid
   regressions.
   (Vincent Ladeuil, #515597)
+  
+Improvements
+************
+
+* The ``news_merge`` plugin is now smarter.  It can resolve conflicts
+  involving section headings as well as bullet points.
+  (Andrew Bennetts)
+
+Internals
+*********
+
+* Use ``bzrlib.cleanup`` rather than less robust ``try``/``finally``
+  blocks in several places in ``bzrlib.merge``.  This avoids masking prior
+  errors when errors like ``ImmortalPendingDeletion`` occur during cleanup
+  in ``do_merge``.
+  (Andrew Bennetts, #517275)
+
+API Changes
+***********
+
+* The ``remove_index`` method of
+  ``bzrlib.repofmt.pack_repo.AggregateIndex`` no longer takes a ``pack``
+  argument.  This argument was always ignored.
+  (Andrew Bennetts, #423015)
 
 bzr 2.1.0rc2
 ############
@@ -416,6 +450,29 @@
   (Martin Pool)
 
 
+bzr 2.0.5 (not released yet)
+############################
+
+:Codename:
+:2.0.5:
+
+Bug Fixes
+*********
+
+* Handle renames correctly when there are files or directories that 
+  differ only in case.  (Chris Jones, Martin Pool, #368931)
+
+* If ``bzr push --create-prefix`` triggers an unexpected ``NoSuchFile``
+  error, report that error rather than failing with an unhelpful
+  ``UnboundLocalError``.
+  (Andrew Bennetts, #423563)
+
+Documentation
+*************
+
+* Added ``location-alias`` help topic.
+  (Andrew Bennetts, #337834)
+
 bzr 2.0.4
 #########
 

=== modified file 'bzrlib/cleanup.py'
--- bzrlib/cleanup.py	2010-01-13 23:16:20 +0000
+++ bzrlib/cleanup.py	2010-02-12 12:16:23 +0000
@@ -31,9 +31,9 @@
 If you want to be certain that the first, and only the first, error is raised,
 then use::
 
-    operation = OperationWithCleanups(lambda operation: do_something())
+    operation = OperationWithCleanups(do_something)
     operation.add_cleanup(cleanup_something)
-    operation.run()
+    operation.run_simple()
 
 This is more inconvenient (because you need to make every try block a
 function), but will ensure that the first error encountered is the one raised,

=== modified file 'bzrlib/help_topics/__init__.py'
--- bzrlib/help_topics/__init__.py	2010-02-04 16:06:36 +0000
+++ bzrlib/help_topics/__init__.py	2010-02-12 12:16:23 +0000
@@ -269,6 +269,9 @@
   bzr+ssh://[e-mail]/~/myproject/trunk
 
 would refer to ``/home/remote/myproject/trunk``.
+
+Many commands that accept URLs also accept location aliases too.  See
+::doc:`location-alias-help`.
 """
 
     return out
@@ -758,6 +761,8 @@
                         'Types of conflicts and what to do about them')
 topic_registry.register('debug-flags', _load_from_file,
                         'Options to show or record debug information')
+topic_registry.register('location-alias', _load_from_file,
+                        'Aliases for remembered locations')
 topic_registry.register('log-formats', _load_from_file,
                         'Details on the logging formats available')
 

=== added file 'bzrlib/help_topics/en/location-alias.txt'
--- bzrlib/help_topics/en/location-alias.txt	1970-01-01 00:00:00 +0000
+++ bzrlib/help_topics/en/location-alias.txt	2010-02-12 12:16:23 +0000
@@ -0,0 +1,19 @@
+Location aliases
+================
+
+Bazaar defines several aliases for locations associated with a branch.  These
+can be used with most commands that expect a location, such as `bzr push`.
+
+The aliases are::
+
+  :parent    the parent of this branch
+  :submit    the submit branch for this branch
+  :public    the public location of this branch
+  :bound     the branch this branch is bound to, for bound branches
+  :push      the saved location used for `bzr push` with no arguments
+  :this      this branch
+
+For example, to push to the parent location::
+
+    bzr push :parent
+

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py	2010-02-04 16:06:36 +0000
+++ bzrlib/log.py	2010-02-12 12:16:23 +0000
@@ -1424,7 +1424,7 @@
         """
         # Revision comes directly from a foreign repository
         if isinstance(rev, foreign.ForeignRevision):
-            return rev.mapping.vcs.show_foreign_revid(rev.foreign_revid)
+            return self._format_properties(rev.mapping.vcs.show_foreign_revid(rev.foreign_revid))
 
         # Imported foreign revision revision ids always contain :
         if not ":" in rev.revision_id:

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2010-02-09 19:04:02 +0000
+++ bzrlib/merge.py	2010-02-12 12:16:23 +0000
@@ -37,6 +37,7 @@
     ui,
     versionedfile
     )
+from bzrlib.cleanup import OperationWithCleanups
 from bzrlib.symbol_versioning import (
     deprecated_in,
     deprecated_method,
@@ -46,11 +47,10 @@
 
 def transform_tree(from_tree, to_tree, interesting_ids=None):
     from_tree.lock_tree_write()
-    try:
-        merge_inner(from_tree.branch, to_tree, from_tree, ignore_zero=True,
-                    interesting_ids=interesting_ids, this_tree=from_tree)
-    finally:
-        from_tree.unlock()
+    operation = OperationWithCleanups(merge_inner)
+    operation.add_cleanup(from_tree.unlock)
+    operation.run_simple(from_tree.branch, to_tree, from_tree,
+        ignore_zero=True, interesting_ids=interesting_ids, this_tree=from_tree)
 
 
 class MergeHooks(hooks.Hooks):
@@ -455,6 +455,7 @@
     def _add_parent(self):
         new_parents = self.this_tree.get_parent_ids() + [self.other_rev_id]
         new_parent_trees = []
+        operation = OperationWithCleanups(self.this_tree.set_parent_trees)
         for revision_id in new_parents:
             try:
                 tree = self.revision_tree(revision_id)
@@ -462,14 +463,9 @@
                 tree = None
             else:
                 tree.lock_read()
+                operation.add_cleanup(tree.unlock)
             new_parent_trees.append((revision_id, tree))
-        try:
-            self.this_tree.set_parent_trees(new_parent_trees,
-                                            allow_leftmost_as_ghost=True)
-        finally:
-            for _revision_id, tree in new_parent_trees:
-                if tree is not None:
-                    tree.unlock()
+        operation.run_simple(new_parent_trees, allow_leftmost_as_ghost=True)
 
     def set_other(self, other_revision, possible_transports=None):
         """Set the revision and tree to merge from.
@@ -626,7 +622,8 @@
                                change_reporter=self.change_reporter,
                                **kwargs)
 
-    def _do_merge_to(self, merge):
+    def _do_merge_to(self):
+        merge = self.make_merger()
         if self.other_branch is not None:
             self.other_branch.update_references(self.this_branch)
         merge.do_merge()
@@ -646,26 +643,19 @@
                     sub_tree.branch.repository.revision_tree(base_revision)
                 sub_merge.base_rev_id = base_revision
                 sub_merge.do_merge()
+        return merge
 
     def do_merge(self):
+        operation = OperationWithCleanups(self._do_merge_to)
         self.this_tree.lock_tree_write()
-        try:
-            if self.base_tree is not None:
-                self.base_tree.lock_read()
-            try:
-                if self.other_tree is not None:
-                    self.other_tree.lock_read()
-                try:
-                    merge = self.make_merger()
-                    self._do_merge_to(merge)
-                finally:
-                    if self.other_tree is not None:
-                        self.other_tree.unlock()
-            finally:
-                if self.base_tree is not None:
-                    self.base_tree.unlock()
-        finally:
-            self.this_tree.unlock()
+        operation.add_cleanup(self.this_tree.unlock)
+        if self.base_tree is not None:
+            self.base_tree.lock_read()
+            operation.add_cleanup(self.base_tree.unlock)
+        if self.other_tree is not None:
+            self.other_tree.lock_read()
+            operation.add_cleanup(self.other_tree.unlock)
+        merge = operation.run_simple()
         if len(merge.cooked_conflicts) == 0:
             if not self.ignore_zero and not trace.is_quiet():
                 trace.note("All changes applied successfully.")
@@ -765,10 +755,26 @@
             warnings.warn("pb argument to Merge3Merger is deprecated")
 
     def do_merge(self):
+        operation = OperationWithCleanups(self._do_merge)
+        operation.add_cleanup(self.pb.clear)
         self.this_tree.lock_tree_write()
+        operation.add_cleanup(self.this_tree.unlock)
         self.base_tree.lock_read()
+        operation.add_cleanup(self.base_tree.unlock)
         self.other_tree.lock_read()
+        operation.add_cleanup(self.other_tree.unlock)
+        operation.run()
+
+    def _do_merge(self, operation):
+        self.tt = transform.TreeTransform(self.this_tree, self.pb)
+        operation.add_cleanup(self.tt.finalize)
+        self.pp.next_phase()
+        self._compute_transform()
+        self.pp.next_phase()
+        results = self.tt.apply(no_conflicts=True)
+        self.write_modified(results)
         try:
+<<<<<<< TREE
             self.tt = transform.TreeTransform(self.this_tree, None)
             try:
                 self._compute_transform()
@@ -784,16 +790,34 @@
             self.other_tree.unlock()
             self.base_tree.unlock()
             self.this_tree.unlock()
+=======
+            self.this_tree.add_conflicts(self.cooked_conflicts)
+        except errors.UnsupportedOperation:
+            pass
+>>>>>>> MERGE-SOURCE
 
     def make_preview_transform(self):
+        operation = OperationWithCleanups(self._make_preview_transform)
+        operation.add_cleanup(self.pb.clear)
         self.base_tree.lock_read()
+        operation.add_cleanup(self.base_tree.unlock)
         self.other_tree.lock_read()
+        operation.add_cleanup(self.other_tree.unlock)
+        return operation.run_simple()
+
+    def _make_preview_transform(self):
         self.tt = transform.TransformPreview(self.this_tree)
+<<<<<<< TREE
         try:
             self._compute_transform()
         finally:
             self.other_tree.unlock()
             self.base_tree.unlock()
+=======
+        self.pp.next_phase()
+        self._compute_transform()
+        self.pp.next_phase()
+>>>>>>> MERGE-SOURCE
         return self.tt
 
     def _compute_transform(self):

=== modified file 'bzrlib/plugins/news_merge/__init__.py'
--- bzrlib/plugins/news_merge/__init__.py	2010-01-28 17:27:16 +0000
+++ bzrlib/plugins/news_merge/__init__.py	2010-02-12 12:16:24 +0000
@@ -26,10 +26,30 @@
 The news_merge_files config option takes a list of file paths, separated by
 commas.
 
+The basic approach is that this plugin parses the NEWS file into a simple
+series of versions, with sections of bullets in those versions.  Sections
+contain a sorted set of bullets, and sections within a version also have a
+fixed order (see the template at the bottom of NEWS).  The plugin merges
+additions and deletions to the set of bullets (and sections of bullets), then
+sorts the contents of these sets and turns them back into a series of lines of
+text.
+
 Limitations:
 
-* if there's a conflict in more than just bullet points, this doesn't yet know
-  how to resolve that, so bzr will fallback to the default line-based merge.
+* invisible whitespace in blank lines is not tracked, so is discarded.  (i.e.
+  [newline, space, newline] is collapsed to just [newline, newline])
+
+* empty sections are generally deleted, even if they were present in the
+  originals.
+
+* modified sections will typically be reordered to match the standard order (as
+  shown in the template at the bottom of NEWS).
+
+* if there's a conflict that involves more than simple sections of bullets,
+  this plugin doesn't know how to handle that.  e.g. a conflict in preamble
+  text describing a new version, or sufficiently many conflicts that the
+  matcher thinks a conflict spans a version heading.  bzr's builtin merge logic
+  will be tried instead.
 """
 
 # Since we are a built-in plugin we share the bzrlib version

=== modified file 'bzrlib/plugins/news_merge/news_merge.py'
--- bzrlib/plugins/news_merge/news_merge.py	2010-01-28 18:05:44 +0000
+++ bzrlib/plugins/news_merge/news_merge.py	2010-02-12 12:16:24 +0000
@@ -18,11 +18,23 @@
 
 
 from bzrlib.plugins.news_merge.parser import simple_parse
-from bzrlib import merge, merge3
+from bzrlib import merge, merge3, trace
 
 
 magic_marker = '|NEWS-MERGE-MAGIC-MARKER|'
 
+# The order sections are supposed to appear in.  See the template at the
+# bottom of NEWS.  None is a placeholder for an unseen section heading.
+canonical_section_order = [
+    None, 'Compatibility Breaks', 'New Features', 'Bug Fixes', 'Improvements',
+    'Documentation', 'API Changes', 'Internals', 'Testing']
+
+def section_sort_key(section):
+    try:
+        return canonical_section_order.index(section)
+    except ValueError:
+        return 99
+
 
 class NewsMerger(merge.ConfigurableFileMerger):
     """Merge bzr NEWS files."""
@@ -36,6 +48,7 @@
         points, so we can simply take a set of bullet points, determine which
         bullets to add and which to remove, sort, and reserialize.
         """
+        trace.mutter('news_merge triggered')
         # Transform the different versions of the NEWS file into a bunch of
         # text lines where each line matches one part of the overall
         # structure, e.g. a heading or bullet.
@@ -49,32 +62,90 @@
         for group in m3.merge_groups():
             if group[0] == 'conflict':
                 _, base, a, b = group
-                # Are all the conflicting lines bullets?  If so, we can merge
-                # this.
-                for line_set in [base, a, b]:
-                    for line in line_set:
-                        if not line.startswith('bullet'):
-                            # Something else :(
-                            # Maybe the default merge can cope.
-                            return 'not_applicable', None
-                # Calculate additions and deletions.
-                new_in_a = set(a).difference(base)
-                new_in_b = set(b).difference(base)
-                all_new = new_in_a.union(new_in_b)
-                deleted_in_a = set(base).difference(a)
-                deleted_in_b = set(base).difference(b)
-                # Combine into the final set of bullet points.
-                final = all_new.difference(deleted_in_a).difference(
-                    deleted_in_b)
-                # Sort, and emit.
-                final = sorted(final, key=sort_key)
-                result_lines.extend(final)
+                # Are all the conflicting lines bullets or sections?  If so, we
+                # can merge this.
+                try:
+                    base_sections = munged_lines_to_section_dict(base)
+                    a_sections = munged_lines_to_section_dict(a)
+                    b_sections = munged_lines_to_section_dict(b)
+                except MergeTooHard:
+                    # Something else :(
+                    # Maybe the default merge can cope.
+                    trace.mutter('news_merge giving up')
+                    return 'not_applicable', None
+                    
+                # Basically, for every section present in any version, call
+                # merge_bullets (passing an empty set for versions missing
+                # that section), and if the resulting set of bullets is not
+                # empty, emit the section heading and the sorted set of
+                # bullets.
+                all_sections = set(
+                    base_sections.keys() + a_sections.keys() +
+                    b_sections.keys())
+                sections_in_order = sorted(all_sections, key=section_sort_key)
+                for section in sections_in_order:
+                    bullets = merge_bullets(
+                        base_sections.get(section, set()),
+                        a_sections.get(section, set()),
+                        b_sections.get(section, set()))
+                    if bullets:
+                        # Emit section heading (if any), then sorted bullets.
+                        if section is not None:
+                            result_lines.append(
+                                'section%s%s\n%s'
+                                % (magic_marker, section, '*' * len(section)))
+                        final = sorted(bullets, key=sort_key)
+                        result_lines.extend(final)
             else:
                 result_lines.extend(group[1])
         # Transform the merged elements back into real blocks of lines.
+        trace.mutter('news_merge giving up')
         return 'success', list(fakelines_to_blocks(result_lines))
 
 
+def merge_bullets(base_bullets, a_bullets, b_bullets):
+    # Calculate additions and deletions.
+    new_in_a = a_bullets.difference(base_bullets)
+    new_in_b = b_bullets.difference(base_bullets)
+    all_new = new_in_a.union(new_in_b)
+    deleted_in_a = base_bullets.difference(a_bullets)
+    deleted_in_b = base_bullets.difference(b_bullets)
+    # Combine into the final set of bullet points.
+    final = all_new.difference(deleted_in_a).difference(deleted_in_b)
+    return final
+
+
+class MergeTooHard(Exception):
+    pass
+
+
+def munged_lines_to_section_dict(lines):
+    """Takes a sequence of munged lines, and returns a dict mapping section to
+    a set of bullets.
+
+    :param lines: a sequence of munged lines
+    :raises MergeTooHard: when lines contain anything other than sections or
+        bullets
+    :returns: a dict of section name -> set of munged bullet lines.  Any
+        bullets encounted before a section will have a name of None.
+    """
+    section = None
+    section_dict = {}
+    for line in lines:
+        if line.startswith('section'):
+            section = line[len('section') + len(magic_marker):]
+            section = section.split('\n', 1)[0]
+        elif line.startswith('bullet'):
+            try:
+                bullets = section_dict[section]
+            except KeyError:
+                bullets = section_dict[section] = set()
+            bullets.add(line)
+        else:
+            raise MergeTooHard()
+    return section_dict
+
+
 def blocks_to_fakelines(blocks):
     for kind, text in blocks:
         yield '%s%s%s' % (kind, magic_marker, text)

=== modified file 'bzrlib/plugins/news_merge/parser.py'
--- bzrlib/plugins/news_merge/parser.py	2010-01-18 07:00:11 +0000
+++ bzrlib/plugins/news_merge/parser.py	2010-02-12 12:16:23 +0000
@@ -24,6 +24,7 @@
 simple_parse's docstring).
 """
 
+import re
 
 def simple_parse(content):
     """Returns blocks, where each block is a 2-tuple (kind, text).
@@ -31,7 +32,10 @@
     :kind: one of 'heading', 'release', 'section', 'empty' or 'text'.
     :text: a str, including newlines.
     """
-    blocks = content.split('\n\n')
+    # Split on blank lines.
+    # XXX: this loses any invisible whitespace, which can cause unexpected
+    # changes to strip the whitespace out of those blank lines.
+    blocks = re.split('\n *\n', content)
     for block in blocks:
         if block.startswith('###'):
             # First line is ###...: Top heading

=== modified file 'bzrlib/push.py'
--- bzrlib/push.py	2009-12-25 13:47:23 +0000
+++ bzrlib/push.py	2010-02-12 12:16:23 +0000
@@ -110,6 +110,12 @@
                     "\nYou may supply --create-prefix to create all"
                     " leading parent directories."
                     % location)
+            # This shouldn't occur (because create_prefix is true, so
+            # create_clone_on_transport should be catching NoSuchFile and
+            # creating the missing directories) but if it does the original
+            # NoSuchFile error will be more informative than an
+            # UnboundLocalError for br_to.
+            raise
         except errors.TooManyRedirections:
             raise errors.BzrCommandError("Too many redirections trying "
                                          "to make %s." % location)

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- bzrlib/repofmt/pack_repo.py	2010-01-29 10:59:12 +0000
+++ bzrlib/repofmt/pack_repo.py	2010-02-12 12:16:23 +0000
@@ -24,6 +24,7 @@
 
 from bzrlib import (
     chk_map,
+    cleanup,
     debug,
     graph,
     osutils,
@@ -646,11 +647,10 @@
         del self.combined_index._indices[:]
         self.add_callback = None
 
-    def remove_index(self, index, pack):
+    def remove_index(self, index):
         """Remove index from the indices used to answer queries.
 
         :param index: An index from the pack parameter.
-        :param pack: A Pack instance.
         """
         del self.index_to_pack[index]
         self.combined_index._indices.remove(index)
@@ -1840,14 +1840,22 @@
         self._remove_pack_indices(pack)
         self.packs.remove(pack)
 
-    def _remove_pack_indices(self, pack):
-        """Remove the indices for pack from the aggregated indices."""
-        self.revision_index.remove_index(pack.revision_index, pack)
-        self.inventory_index.remove_index(pack.inventory_index, pack)
-        self.text_index.remove_index(pack.text_index, pack)
-        self.signature_index.remove_index(pack.signature_index, pack)
-        if self.chk_index is not None:
-            self.chk_index.remove_index(pack.chk_index, pack)
+    def _remove_pack_indices(self, pack, ignore_missing=False):
+        """Remove the indices for pack from the aggregated indices.
+        
+        :param ignore_missing: Suppress KeyErrors from calling remove_index.
+        """
+        for index_type in Pack.index_definitions.keys():
+            attr_name = index_type + '_index'
+            aggregate_index = getattr(self, attr_name)
+            if aggregate_index is not None:
+                pack_index = getattr(pack, attr_name)
+                try:
+                    aggregate_index.remove_index(pack_index)
+                except KeyError:
+                    if ignore_missing:
+                        continue
+                    raise
 
     def reset(self):
         """Clear all cached data."""
@@ -2091,24 +2099,21 @@
         # FIXME: just drop the transient index.
         # forget what names there are
         if self._new_pack is not None:
-            try:
-                self._new_pack.abort()
-            finally:
-                # XXX: If we aborted while in the middle of finishing the write
-                # group, _remove_pack_indices can fail because the indexes are
-                # already gone.  If they're not there we shouldn't fail in this
-                # case.  -- mbp 20081113
-                self._remove_pack_indices(self._new_pack)
-                self._new_pack = None
+            operation = cleanup.OperationWithCleanups(self._new_pack.abort)
+            operation.add_cleanup(setattr, self, '_new_pack', None)
+            # If we aborted while in the middle of finishing the write
+            # group, _remove_pack_indices could fail because the indexes are
+            # already gone.  But they're not there we shouldn't fail in this
+            # case, so we pass ignore_missing=True.
+            operation.add_cleanup(self._remove_pack_indices, self._new_pack,
+                ignore_missing=True)
+            operation.run_simple()
         for resumed_pack in self._resumed_packs:
-            try:
-                resumed_pack.abort()
-            finally:
-                # See comment in previous finally block.
-                try:
-                    self._remove_pack_indices(resumed_pack)
-                except KeyError:
-                    pass
+            operation = cleanup.OperationWithCleanups(resumed_pack.abort)
+            # See comment in previous finally block.
+            operation.add_cleanup(self._remove_pack_indices, resumed_pack,
+                ignore_missing=True)
+            operation.run_simple()
         del self._resumed_packs[:]
 
     def _remove_resumed_pack_indices(self):

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-02-11 09:27:55 +0000
+++ bzrlib/tests/__init__.py	2010-02-12 12:16:24 +0000
@@ -4372,6 +4372,23 @@
 CaseInsensitiveFilesystemFeature = _CaseInsensitiveFilesystemFeature()
 
 
+class _CaseSensitiveFilesystemFeature(Feature):
+
+    def _probe(self):
+        if CaseInsCasePresFilenameFeature.available():
+            return False
+        elif CaseInsensitiveFilesystemFeature.available():
+            return False
+        else:
+            return True
+
+    def feature_name(self):
+        return 'case-sensitive filesystem'
+
+# new coding style is for feature instances to be lowercase
+case_sensitive_filesystem_feature = _CaseSensitiveFilesystemFeature()
+
+
 # Kept for compatibility, use bzrlib.tests.features.subunit instead
 SubUnitFeature = _CompatabilityThunkFeature(
     deprecated_in((2,1,0)),

=== modified file 'bzrlib/tests/per_tree/test_inv.py'
--- bzrlib/tests/per_tree/test_inv.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_tree/test_inv.py	2010-02-12 12:16:23 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007, 2008 Canonical Ltd
+# Copyright (C) 2007, 2008, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -23,7 +23,10 @@
 from bzrlib import (
     tests,
     )
-from bzrlib.tests import per_tree
+from bzrlib.tests import (
+    features,
+    per_tree,
+    )
 from bzrlib.mutabletree import MutableTree
 from bzrlib.tests import SymlinkFeature, TestSkipped
 from bzrlib.transform import _PreviewTree
@@ -131,6 +134,8 @@
         work_tree.add(['dir', 'dir/file'])
         if commit:
             work_tree.commit('commit 1')
+        # XXX: this isn't actually guaranteed to return the class we want to
+        # test -- mbp 2010-02-12
         return work_tree
 
     def test_canonical_path(self):
@@ -163,3 +168,21 @@
         work_tree = self._make_canonical_test_tree()
         self.assertEqual('dir/None',
                          work_tree.get_canonical_inventory_path('Dir/None'))
+
+    def test_canonical_tree_name_mismatch(self):
+        # see <https://bugs.edge.launchpad.net/bzr/+bug/368931>
+        # some of the trees we want to use can only exist on a disk, not in
+        # memory - therefore we can only test this if the filesystem is
+        # case-sensitive.
+        self.requireFeature(tests.case_sensitive_filesystem_feature)
+        work_tree = self.make_branch_and_tree('.')
+        self.build_tree(['test/', 'test/file', 'Test'])
+        work_tree.add(['test/', 'test/file', 'Test'])
+
+        test_tree = self._convert_tree(work_tree)
+        test_tree.lock_read()
+        self.addCleanup(test_tree.unlock)
+
+        self.assertEqual(['test', 'test/file', 'Test', 'test/foo', 'Test/foo'],
+            test_tree.get_canonical_inventory_paths(
+                ['test', 'test/file', 'Test', 'test/foo', 'Test/foo']))

=== modified file 'bzrlib/tree.py'
--- bzrlib/tree.py	2010-02-10 21:36:32 +0000
+++ bzrlib/tree.py	2010-02-12 12:16:24 +0000
@@ -1,4 +1,8 @@
+<<<<<<< TREE
 # Copyright (C) 2005-2010 Canonical Ltd
+=======
+# Copyright (C) 2005, 2009, 2010 Canonical Ltd
+>>>>>>> MERGE-SOURCE
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -405,16 +409,34 @@
             bit_iter = iter(path.split("/"))
             for elt in bit_iter:
                 lelt = elt.lower()
+                new_path = None
                 for child in self.iter_children(cur_id):
                     try:
+                        # XXX: it seem like if the child is known to be in the
+                        # tree, we shouldn't need to go from its id back to
+                        # its path -- mbp 2010-02-11
+                        #
+                        # XXX: it seems like we could be more efficient
+                        # by just directly looking up the original name and
+                        # only then searching all children; also by not
+                        # chopping paths so much. -- mbp 2010-02-11
                         child_base = os.path.basename(self.id2path(child))
-                        if child_base.lower() == lelt:
+                        if (child_base == elt):
+                            # if we found an exact match, we can stop now; if
+                            # we found an approximate match we need to keep
+                            # searching because there might be an exact match
+                            # later.  
                             cur_id = child
-                            cur_path = osutils.pathjoin(cur_path, child_base)
+                            new_path = osutils.pathjoin(cur_path, child_base)
                             break
+                        elif child_base.lower() == lelt:
+                            cur_id = child
+                            new_path = osutils.pathjoin(cur_path, child_base)
                     except NoSuchId:
                         # before a change is committed we can see this error...
                         continue
+                if new_path:
+                    cur_path = new_path
                 else:
                     # got to the end of this directory and no entries matched.
                     # Return what matched so far, plus the rest as specified.