Re: [PATCH] CachedVC: Add ability to update file status when its modified

[Peter Tyser <ptyser gmail com>]

Thanks for the review.

<snip>

> Could you rewrite this comment as a docstring? I know the existing
> code isn't big on them, but it probably should be.

Will do.

>> -    def _lookup_tree_cache(self, rootdir):
>> +    # Inspect the state of the file or directory at 'path'
>> +    def get_path_state(self, path, tree_state):
>
> I think get_path_state is misleading here; update_path_state would be
> better, though that's perilously close to update_file_state below,
> while being completely different. Maybe something overly verbose like
> _update_tree_state_cache?

Overly verbose sounds good to me.

<snip>

>> +
>> +        if len(entries) == 0 and os.path.isfile(path):
>> +            # The file must be version-controlled and in a non-modified state
>> +            tree_state[path] = _vc.STATE_NORMAL
>> +        else:
>> +            # There are 1 or more modified files
>> +            for entry in entries:
>> +                statekey, name = entry.split("\t", 2)
>> +                path = os.path.join(self.root, name.strip())
>> +                state = self.state_map.get(statekey.strip(), _vc.STATE_NONE)
>> +                tree_state[path] = state
>> +
>
> So this looks like: if we don't get any results, we assume that the
> path is normal, but if we get results, anything not mentioned gets the
> NONE state instead. The second part of this sounds right to me, but
> I'm wondering why the first part is needed; it wasn't part of the
> previous logic, so what's different?

The first part of the conditional is needed now that this function is
used to update the tree state, specifically when a file changes from
modified to non-modified.  Previously it always was called to create a
fresh tree so this wasn't necessary.

For example, if 'AUTHORS' is modified when the tree state is first
populated, then its modifications are manually reverted so that it is
no longer modified, we need to update 'AUTHORS' state to STATE_NORMAL
(STATE_NONE would also work, but I like STATE_NORMAL personally).
Without that first check the get_path_state() wouldn't know that
'AUTHORS' needs to be updated so it'd remain in its "modified" state.
There are some other ways to accomplish the same thing, but the logic
is generally pretty similar.  I'll beef up the comment in the next rev
and feel free to chime in if someone has a cleaner way.

>> +    def _lookup_tree_cache(self, rootdir):
>> +        # Get a list of all files in rootdir, as well as their status
>>         tree_state = {}
>> -        for entry in entries:
>> -            statekey, name = entry.split("\t", 2)
>> -            path = os.path.join(self.root, name.strip())
>> -            state = self.state_map.get(statekey.strip(), _vc.STATE_NONE)
>> -            tree_state[path] = state
>> +        self.get_path_state("./", tree_state)
>>
>>         return tree_state
>>
>> +    # Call to update the state of a specific file.  For example after a
>> +    # file has been modified and saved in meld, its state may be out of
>> +    # date and require updating.
>
> This is basically a duplicate comment from the overridden method.

Yeah, it was intentional.  I'm not too familiar with the meld codebase
at this point so I probably over-comment to keep myself straight.
I'll remove if preferred.

Best,
Peter