Inventory.remove_recursive_id should take a list of file-ids

Bug #604950 reported by John A Meinel
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Bazaar
Confirmed
Medium
Unassigned

Bug Description

Inventory.remove_recursive_id seems to have a clumsy interface. At the moment, it is called by MutableTrees (WT, MemoryTree), to update the inventory when it finds files missing on disk during commit.

However, all callers seem to actually have many file-ids to remove (if you remove a directory, it should remove all the children of that directory). When using inv.remove_recursive_id() they then need to be careful to not remove file-ids that end up automatically removed by removing the parent directory.

The callers currently do:
WorkingTree:
    for file_id in file_ids:
        if file_id not in self._inventory:
            raise errors.NoSuchId(self, file_id)
    for file_id in file_ids:
        if self._inventory.has_id(file_id):
            self._inventory.remove_recursive_id(file_id)

So it first checks that all file ids really do exist, then removes them, checking to avoid removing something that was removed as a side effect.

MemoryTree:
    for file_id in file_ids:
        if self._inventory.has_id(file_id):
            self._inventory.remove_recursive_id(file_id)
        else:
            raise errors.NoSuchId(self, file_id)

Actually gets it wrong. It does want to check that the ids already exist, and raises an exception if they don't. However, one that used to exist could have gone missing in the meantime.

WorkingTree_4:
        if ids_to_unversion:
            raise errors.NoSuchId(self, iter(ids_to_unversion).next())
        self._make_dirty(reset_inventory=False)
        # have to change the legacy inventory too.
        if self._inventory is not None:
            for file_id in file_ids:
                self._inventory.remove_recursive_id(file_id)

Also a bit wrong. Did do the check that it has removed all the ids from the dirstate, but then removes everything directly from the inventory.

Note that the source of this bug is probably because commit (without iter_changes) only finds the top-level directory missing. It does not include file-ids for children records. That will be reported on another bug.

Tags: api inventory
Jelmer Vernooij (jelmer)
tags: added: api inventory
Jelmer Vernooij (jelmer)
tags: added: check-for-breezy
Jelmer Vernooij (jelmer)
tags: removed: check-for-breezy
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.