[MERGE] commit of removals respects filespec (fixes #46635)

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> Hi all,
> 
> Currently, commit doesn't respect the file spec for file removals.  This
> is different from deletions, which are already handled properly.
> 
> The reason is that the code that populates the new inventory iterates
> through the working tree only.  The difference between a deletion and a
> removal is that a deletion does have an entry in the working tree (just
> no contents).  Since deletions do have an entry in the working tree,
> they are handled correctly.


I think I understand what you mean.
deletion => Deleted from disk, but not removed from the working
inventory (these are currently 'auto deleted')
removal => was removed from working inventory with 'bzr rm'. may or may
not exist on disk.

> 
> This patch causes the basis tree to be scanned for unselected removals,
> which are then added to the new inventory.
> 
> Aaron


=== modified file bzrlib/commit.py
- --- bzrlib/commit.py
+++ bzrlib/commit.py
@@ -497,6 +497,10 @@
         None; inventory entries that are carried over untouched have their
         revision set to their prior value.
         """
+        # ESEPARATIONOFCONCERNS: this function is diffing and using the
diff
+        # results to create a new inventory at the same time, which results
+        # in bugs like #46635.  Any reason not to use/enhance
compare_trees?
+        # ADHB 11-07-2006
         mutter("Selecting files for commit with filter %s",
self.specific_files)
         # iter_entries does not visit the ROOT_ID node so we need to call
         # self._emit_progress_update once by hand.


I think it would be reasonable to have a function that can build up a
new inventory from two trees and a filespec.

compare_trees() isn't perfect at this point, because it goes through
'list_files()' which has no way to represent missing files, so they
currently get marked as deleted.
That is mostly a 'list_files()' api bug, though.

I think Robert and I will be working on revamping some apis to change
how to do comparisons (so we can optimize for cases where the delta is
already accessible). It might make sense to have a function that can
take a tree + a delta to build up a new tree. Though maybe it would just
be better if took two trees, and internally calls the delta function.



@@ -534,6 +538,20 @@
             else:
                 self.reporter.snapshot_change(change, path)

+        if not self.specific_files:
+            return
+
+        # ignore removals that don't match filespec
+        for path, new_ie in self.basis_inv.iter_entries():
+            if new_ie.file_id in self.work_inv:
+                continue
+            if is_inside_any(self.specific_files, path):
+                continue
+            ie = new_ie.copy()
+            ie.revision = None
+            self.builder.record_entry_contents(ie, self.parent_invs, path,
+                                               self.basis_tree)
+

Seems okay. +1 from me. I don't really like the double iteration stuff,
but for now, it fixes the bug. And we can do it more correctly as we
modify the api.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEs+gKJdeBCYSNAAMRAiKxAJ98y8jmmASbSm+VpJBGB4D5LzKKUgCglvIj
/MDYA6zSB1BuQwNZ6px8P9c=
=Q5hR
-----END PGP SIGNATURE-----