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

[Kai <kai willadsen gmail com>]

On 4 January 2011 16:19, Peter Tyser <ptyser gmail com> wrote:
> Previously, a VcView diff of a cached version controlled directory could
> easily become out of date.  For example, if the file 'AUTHORS' was a
> version controlled file in a git repository:
>  > echo "asdf" >> AUTHORS
>  > meld ./
>  # Double click on the AUTHORS file to view its diff.
>  # Remove the "asdf" at the bottom of a file and save it.
>  # Close the AUTHORS tab.
>  # The state of the AUTHORS file will still show up as modified, even
>  # though it is now unmodified.
>
> This change implements the new update_file_state() for git.  Similar
> funcationality could be added to monotone, darcs, bzr, and arch.
>
> Signed-off-by: Peter Tyser <ptyser gmail com>
> ---
> It'd be possible to make a similar change by modifying the
> existing _get_dirsandfiles() function, but I thought this implementation
> was cleaner.  Let me know if others have a better idea.

I like this approach. It looks clean and is probably easily
implementable by other caching VCs. Usual nitpicking inline.

>  meld/vc/_vc.py |    8 ++++++++
>  meld/vc/git.py |   36 +++++++++++++++++++++++++++---------
>  meld/vcview.py |    1 +
>  3 files changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/meld/vc/_vc.py b/meld/vc/_vc.py
> index 4cd5560..45c4dc1 100644
> --- a/meld/vc/_vc.py
> +++ b/meld/vc/_vc.py
> @@ -136,6 +136,14 @@ class Vc(object):
>     def uncache_inventory(self):
>         pass
>
> +    # Update the state of a specific file.  For example after a file
> +    # has been modified and saved, its state may be out of
> +    # date and require updating.  This can be implemented for Vc
> +    # plugins that cache file states, eg 'git' an 'bzr' so that the
> +    # top-level file status is always accurate.
> +    def update_file_state(self, path):
> +        pass
> +

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

>     def get_patch_files(self, patch):
>         regex = re.compile(self.PATCH_INDEX_RE, re.M)
>         return [f.strip() for f in regex.findall(patch)]
> diff --git a/meld/vc/git.py b/meld/vc/git.py
> index 82c6c23..9e86abd 100644
> --- a/meld/vc/git.py
> +++ b/meld/vc/git.py
> @@ -77,7 +77,8 @@ class Vc(_vc.CachedVc):
>         else:
>             return ''
>
> -    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?

>         while 1:
>             try:
>                 # Update the index before getting status, otherwise we could
> @@ -87,17 +88,17 @@ class Vc(_vc.CachedVc):
>                 # Get the status of files that are different in the "index" vs
>                 # the HEAD of the git repository
>                 proc = _vc.popen([self.CMD, "diff-index", "--name-status", \
> -                    "--cached", "HEAD", "./"], cwd=self.location)
> +                    "--cached", "HEAD", path], cwd=self.location)
>                 entries = proc.read().split("\n")[:-1]
>
>                 # Get the status of files that are different in the "index" vs
>                 # the files on disk
>                 proc = _vc.popen([self.CMD, "diff-files", "--name-status", \
> -                    "-0", "./"], cwd=self.location)
> +                    "-0", path], cwd=self.location)
>                 entries += (proc.read().split("\n")[:-1])
>
>                 proc = _vc.popen([self.CMD, "ls-files", "--others", \
> -                    "--ignored", "--exclude-standard"], cwd=self.location)
> +                    "--ignored", "--exclude-standard", path], cwd=self.location)
>                 entries += ("I\t%s" % f for f in proc.read().split("\n")[:-1])
>
>                 # An unmerged file or a file that has been modified, added to
> @@ -110,15 +111,32 @@ class Vc(_vc.CachedVc):
>             except OSError, e:
>                 if e.errno != errno.EAGAIN:
>                     raise
> +
> +        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?

> +    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.

> +    def update_file_state(self, path):
> +        tree_state = self._get_tree_cache(os.path.dirname(path))
> +        self.get_path_state(path, tree_state)
> +
>     def _get_dirsandfiles(self, directory, dirs, files):
>
>         tree = self._get_tree_cache(directory)
> diff --git a/meld/vcview.py b/meld/vcview.py
> index c9d66e0..c2561b9 100644
> --- a/meld/vcview.py
> +++ b/meld/vcview.py
> @@ -583,6 +583,7 @@ class VcView(melddoc.MeldDoc, gnomeglade.Component):
>         it = self.find_iter_by_name(filename)
>         if it:
>             path = self.model.value_path(it, 0)
> +            self.vc.update_file_state(path)
>             files = self.vc.lookup_files([], [(os.path.basename(path), path)])[1]
>             for e in files:
>                 if e.path == path:
> --
> 1.7.1.13.gcfb88
>
> _______________________________________________
> meld-list mailing list
> meld-list gnome org
> http://mail.gnome.org/mailman/listinfo/meld-list
>