Code review comment for lp:~ian-clatworthy/bzr/faster-dirstate-saving

I'd just like to make it clear that I'm "hesitant" on this patch. I'll admit I haven't looked at it closely, but it seems a bit along the lines of a band-aid for now sort of thing.

Especially given the complexity of keeping multiple inventories in sync...

221 + if self._dirblock_state == DirState.IN_MEMORY_UNMODIFIED:
222 + keys = []
223 + for directory in self._dirblocks:
224 + keys.extend([e[0] for e in directory[1]])
225 + else:

What is the use case for calling _build_line_cache when the current state is IN_MEMORY_UNMODIFIED? Certainly save() is already defined as a no-op in that case. I'd rather avoid having extra code paths that aren't really being used.
I guess maybe it is if the header changes and the blocks don't...

I'll just mention that if you look over the code, the only time _header_state = ...MODIFIED is when _dirblock_state = ... MODIFIED

The only reason it has a separate state flag, is because you can read the header without reading the rest of the dirblocks. (At least to date, certainly the knob exists for that to not be true.)
Though if you consider what is in the header:

1) num parents / parent ids (if this changes, the dirblock state certainly should)
2) ghost parents (see 1)
3) crc (Also indicative of actual content changes)
4) num_entries (if this changes, we better have dirblock changes)