Code review comment for lp:~jameinel/bzr/2.4-dirstate-invalid-delta-855155

I tried to add this test, but it did not explode:

in test_dirstate.py TestUpdateBasisByDelta:

    def test_add_file_in_empty_dir_not_matching_active_state(self):
        state = self.assertUpdate(
                active=[],
                basis=[('dir/', 'dir-id')],
                target=[('dir/', 'dir-id'), ('dir/file', 'file-id')],
                )

The reason it does not explode is because the inventory entry for dir/ in basis has a last-modified revision of 'basis', while the inventory entry in target has a last-modified revision of 'target'.
So the delta produced still includes 'dir/' even though it isn't supposed to change in this test. And we have this bit of code in _update_basis_apply_adds:
            new_kind = new_details[0]
            if new_kind == 'd':
                self._ensure_block(block_index, entry_index, new_path)

So if we see the parent directory in the delta, we will call _ensure_block.

The case we are seeing here is:

1) The directory already exists and is unchanged
2) The file is just being added into that directory
3) The directory is empty, so it does not already have a dirblock existing for it

I'm pretty sure the delta of:
[(None, 'A/B', 'file-id-B',
  InventoryFile('file-id-B', 'B', parent_id='dir-id-A',
  sha1='11111111111111111111111111111111', len=24, revision=new-parent))]

Is accurate for the case of just adding a file to a directory that is otherwise unchanged.

I did push up a tweaked version of a direct Dirstate.update_basis_by_delta test, which requires tweaking the test infrastructure to allow you to specify the exact last-modified-revision for path entries. I think it is reasonably tasteful and does give you a test at the layer the bug exists. (It really is a bug in Dirstate itself, and wouldn't inherently apply to all other tree formats that might exist, it only triggers because we don't force blocks to exist for empty directories.)