[PATCH] Provide better context for custom Python encode/decode filters

[Patrick Mézard]

Jesse Glick a écrit :
> # HG changeset patch
> # User Jesse Glick <jesse.glick at sun.com>
> # Date 1198297277 18000
> # Node ID dbf62abe9c0fba6559c226ded72251507e742891
> # Parent  1b4ed187319a26c2a6f01836266a8e9ae32705cc
> Provide better context for custom Python encode/decode filters.
> While some can function with just some text and an optional command name,
> others may want a repository object, a ui object, and a file path.
> Use the enhanced information to good effect in win32text.dumbdecode's warning.

I think the patch is good, but it also highlights several problems with the filter facility. Wrapping "pipefilter" and "tempfilter" in util is good, these are reusable and generic features (and the gpg extensions makes good use of them). Mixing them with extensions supplied commands looks wrong:
1- Extensions commands are not really generic and if they are, let's make a python module of them. I don't feel that calling "util.filter('foo', 'cleverencode')" explicitely will happen anytime soon. Only localrepo uses them , it should own them.
2- Registering them by updating util.filtertable is ugly. Assuming [1], I would rather register them in a localrepo.
3- [1] and [2] make implementation changes complicated, as shown in the patch. Having a registration API as suggested in [2] would allow old versions to be handled more gracefully.
4- Syntax is confusing. "pipefilter" and "tempfilter" are ways of executing commands supplied as argument, while "cleverencode" is a command. To be coherent, we would have some hook-like "python:cleverencode". But this is a minor point.


The patch below adds a simple registration mechanism to localrepo and uses it in win32text through reposetup(). It probably does not even break existing code updating util.filtertable, but I am not sure this is a good thing. If it's okay, I would be happy to rebase Jesse patch against it.


# HG changeset patch
# User Patrick Mezard <pmezard at gmail.com>
# Date 1198441173 -3600
# Node ID 843ff80607218050bf7f013c7f4d37d414fe0702
# Parent  5d14d71148b8f4d95c677846305fcfd914a3721c
Register data filters in a localrepo instead of util

- Changing data filters implementation is easier, adddatafilter() can rewrap
  filter after inspecting their prototype
- Custom data filters really belongs to localrepo, mixing them with generic
  wrapper like "pipefilter" or "tempfilter" looks wrong.
- util.filtertable should not be accessed from extensions

diff -r 5d14d71148b8 -r 843ff8060721 hgext/win32text.py
--- a/hgext/win32text.py	Fri Dec 21 14:26:20 2007 -0800
+++ b/hgext/win32text.py	Sun Dec 23 21:19:33 2007 +0100
@@ -62,12 +62,12 @@ def cleverencode(s, cmd):
         return dumbencode(s, cmd)
     return s
 
-util.filtertable.update({
+_filters = {
     'dumbdecode:': dumbdecode,
     'dumbencode:': dumbencode,
     'cleverdecode:': cleverdecode,
     'cleverencode:': cleverencode,
-    })
+    }
 
 def forbidcrlf(ui, repo, hooktype, node, **kwargs):
     halt = False
@@ -99,3 +99,10 @@ def forbidcrlf(ui, repo, hooktype, node,
                   '[decode]\n'
                   '** = cleverdecode:\n'))
     return halt
+
+def reposetup(ui, repo):
+    if not repo.local():
+        return
+    for name, fn in _filters.iteritems():
+        repo.adddatafilter(name, fn)
+
diff -r 5d14d71148b8 -r 843ff8060721 mercurial/localrepo.py
--- a/mercurial/localrepo.py	Fri Dec 21 14:26:20 2007 -0800
+++ b/mercurial/localrepo.py	Sun Dec 23 21:19:33 2007 +0100
@@ -83,6 +83,7 @@ class localrepository(repo.repository):
         self.branchcache = None
         self.nodetagscache = None
         self.filterpats = {}
+        self.datafilters = {}
         self._transref = self._lockref = self._wlockref = None
 
     def __getattr__(self, name):
@@ -485,16 +486,26 @@ class localrepository(repo.repository):
             l = []
             for pat, cmd in self.ui.configitems(filter):
                 mf = util.matcher(self.root, "", [pat], [], [])[1]
-                l.append((mf, cmd))
+                fn = None
+                for name, filterfn in self.datafilters.iteritems():
+                    if cmd.startswith(name): 
+                        fn = filterfn
+                        break
+                if not fn:
+                    fn = lambda s, c: util.filter(s, c)
+                l.append((mf, fn, cmd))
             self.filterpats[filter] = l
 
-        for mf, cmd in self.filterpats[filter]:
+        for mf, fn, cmd in self.filterpats[filter]:
             if mf(filename):
                 self.ui.debug(_("filtering %s through %s\n") % (filename, cmd))
-                data = util.filter(data, cmd)
+                data = fn(data, cmd)
                 break
 
         return data
+
+    def adddatafilter(self, name, filter):
+        self.datafilters[name] = filter
 
     def wread(self, filename):
         if self._link(filename):