Patchwork [1,of,3,RFC,V3] largefiles: changed overridelog to work with graphlog

login
register
mail settings
Submitter Lucas Moscovicz
Date March 10, 2014, 6:27 p.m.
Message ID <93739ecdb6bbb600b77e.1394476050@dev1037.prn2.facebook.com>
Download mbox | patch
Permalink /patch/3903/
State Superseded
Commit 49e13e76ec5a34fa6aab2c86b15c77ed0c6b394a
Headers show

Comments

Lucas Moscovicz - March 10, 2014, 6:27 p.m.
# HG changeset patch
# User Lucas Moscovicz <lmoscovicz@fb.com>
# Date 1394063709 28800
#      Wed Mar 05 15:55:09 2014 -0800
# Node ID 93739ecdb6bbb600b77e902d33843b56b913315f
# Parent  e0c93e315d4ae98d83f69fd7ca8cceabc06c7b61
largefiles: changed overridelog to work with graphlog

Log for largefiles was failing for graph log since it was overriding match
instead of matchandpats. This was not actually covered by the tests, which
only cover log code.

Tests for graphlog to be added in future patches, not added before this one
since they would fail in most cases.
Mads Kiilerich - March 26, 2014, 7:07 p.m.
Commenting on this old RFC:

On 03/10/2014 07:27 PM, Lucas Moscovicz wrote:
> # HG changeset patch
> # User Lucas Moscovicz <lmoscovicz@fb.com>
> # Date 1394063709 28800
> #      Wed Mar 05 15:55:09 2014 -0800
> # Node ID 93739ecdb6bbb600b77e902d33843b56b913315f
> # Parent  e0c93e315d4ae98d83f69fd7ca8cceabc06c7b61
> largefiles: changed overridelog to work with graphlog
>
> Log for largefiles was failing for graph log since it was overriding match
> instead of matchandpats. This was not actually covered by the tests, which
> only cover log code.
>
> Tests for graphlog to be added in future patches, not added before this one
> since they would fail in most cases.
>
> diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
> --- a/hgext/largefiles/overrides.py
> +++ b/hgext/largefiles/overrides.py
> @@ -8,7 +8,7 @@
>   
>   '''Overridden Mercurial commands and functions for the largefiles extension'''
>   
> -import os
> +import os, re, glob
>   import copy
>   
>   from mercurial import hg, commands, util, cmdutil, scmutil, match as match_, \
> @@ -47,6 +47,17 @@
>       scmutil.match = f
>       return oldmatch
>   
> +def installmatchandpatsfn(f):
> +    oldmatchandpats = scmutil.matchandpats
> +    setattr(f, 'oldmatchandpats', oldmatchandpats)
> +    scmutil.matchandpats = f
> +    return oldmatchandpats
> +
> +def installfilelogfn(f):

There is only one obvious interpretation of what installmatchandpatsfn 
do, but it is very hard to guess from the name what installfilelogfn 
really do. I think a long and ugly name like 
'installrevsetfilelogsymbol' would be better (and similar for restore).

> +    oldsymbol = revset.symbols['filelog']
> +    revset.symbols['filelog'] = f
> +    return oldsymbol
> +
>   def restorematchfn():
>       '''restores scmutil.match to what it was before installnormalfilesmatchfn
>       was called.  no-op if scmutil.match is its original function.
> @@ -55,6 +66,20 @@
>       restore matchfn to reverse'''
>       scmutil.match = getattr(scmutil.match, 'oldmatch', scmutil.match)
>   
> +def restorematchandpatsfn():
> +    '''restores scmutil.matchandpats to what it was before
> +    installnormalfilesmatchandpatsfn was called.  no-op if scmutil.matchandpats
> +    is its original function.
> +
> +    Note that n calls to installnormalfilesmatchandpatsfn will require n calls
> +    to restore matchfn to reverse'''
> +    scmutil.matchandpats = getattr(scmutil.matchandpats, 'oldmatchandpats',
> +            scmutil.matchandpats)
> +
> +def restorefilelogfn(oldsymbol=revset.filelog):
> +    '''restores revset.filelog to what it was before.'''

This is more like '''restores revset.symbols['filelog'] to 
revset.filelog'''.

It hardcodes the assumption that the original value of the 'filelog' 
symbol is the import time revset.filelog. It is probably not a real 
problem, but why not just be consistent with the other code and save the 
old value when as an attribute on f in installfilelogfn.

> +    revset.symbols['filelog'] = oldsymbol
> +
>   def addlargefiles(ui, repo, *pats, **opts):
>       large = opts.pop('large', None)
>       lfsize = lfutil.getminsize(
> @@ -241,15 +266,15 @@
>           repo._repo.lfstatus = False
>   
>   def overridelog(orig, ui, repo, *pats, **opts):
> -    def overridematch(ctx, pats=[], opts={}, globbed=False,
> +    def overridematchandpats(ctx, pats=[], opts={}, globbed=False,
>               default='relpath'):
>           """Matcher that merges root directory with .hglf, suitable for log.
>           It is still possible to match .hglf directly.
>           For any listed files run log on the standin too.
>           matchfn tries both the given filename and with .hglf stripped.
>           """
> -        match = oldmatch(ctx, pats, opts, globbed, default)
> -        m = copy.copy(match)
> +        matchandpats = oldmatchandpats(ctx, pats, opts, globbed, default)
> +        m, p = copy.copy(matchandpats)
>           for i in range(0, len(m._files)):
>               standin = lfutil.standin(m._files[i])
>               if standin in repo[ctx.node()]:
> @@ -264,14 +289,74 @@
>               r = origmatchfn(f)
>               return r
>           m.matchfn = lfmatchfn
> -        return m
> -    oldmatch = installmatchfn(overridematch)
> +
> +        def expandpats(pats):
> +            ret = set()
> +            for p in pats:
> +                kind, name = match_._patsplit(p, None)
> +                if kind is None:
> +                    try:
> +                        globbed = glob.glob(name)
> +                    except re.error:
> +                        globbed = set([name])
> +                    if globbed:
> +                        ret.update(globbed)
> +                        for g in globbed:
> +                            if not lfutil.isstandin(g):
> +                                ret.add(lfutil.standin(g))
> +                        continue
> +                ret.add(p)
> +            return ret

We talked about this before. It is mostly a copy from somewhere else ... 
and here it is as a function that only is called once without any 
documentation of what it is or why it is that way. This leaves the code 
more weird than it used to be.

It also deserves a comment to describe why we add standin for globbed 
values when kind is None but don't do anything for other kind of 
patterns. Is it handled elsewhere (where?) or is it a known limitation 
that someone who cares can try to fix later?

> +
> +        if pats == ("",):
> +            pats = set()
> +        else:
> +            pats = expandpats(pats or [])
> +
> +        # Add the new matched files to the pats
> +        for f in m.files():
> +            if f not in pats:
> +                pats.add(f)
> +                if lfutil.isstandin(f):
> +                    slf = lfutil.splitstandin(f)
> +                else:
> +                    slf = lfutil.standin(f)
> +                if slf not in pats:
> +                    pats.add(slf)

I don't know what slf means (is it just a "f2"?) ... but why not just do

+                if lfutil.isstandin(f):
+                    pats.add(lfutil.splitstandin(f))
+                else:
+                    pats.add(lfutil.standin(f))

> +
> +        return m, pats
> +
> +    def overridefilelog(repo, subset, x):
> +        '''override filelog revset, use relative paths instead of
> +        pathutil.canonpath to look for a pat's filelog
> +        '''
> +        # i18n: "filelog" is a keyword
> +        pat = revset.getstring(x, _("filelog requires a pattern"))
> +        s = set()
> +
> +        if not match_.patkind(pat):
> +            fl = repo.file(pat)
> +            for fr in fl:
> +                s.add(fl.linkrev(fr))
> +        else:
> +            m = match_.match(repo.root, repo.getcwd(), [pat], ctx=repo[None])
> +            for f in repo[None]:
> +                if m(f):
> +                    fl = repo.file(f)
> +                    for fr in fl:
> +                        s.add(fl.linkrev(fr))
> +
> +        return subset.filter(lambda r: r in s)
> +
> +    oldmatchandpats = installmatchandpatsfn(overridematchandpats)
> +    oldsymbol = installfilelogfn(overridefilelog)
>       try:
>           repo.lfstatus = True
>           return orig(ui, repo, *pats, **opts)
>       finally:
>           repo.lfstatus = False
> -        restorematchfn()
> +        restorematchandpatsfn()
> +        restorefilelogfn(oldsymbol)
>   
>   def overrideverify(orig, ui, repo, *pats, **opts):
>       large = opts.pop('large', False)

It would be very nice if the comments here could be addressed ... but 
even as it is here it leaves the code in a better state than before 
overall. Thanks!

/Mads

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -8,7 +8,7 @@ 
 
 '''Overridden Mercurial commands and functions for the largefiles extension'''
 
-import os
+import os, re, glob
 import copy
 
 from mercurial import hg, commands, util, cmdutil, scmutil, match as match_, \
@@ -47,6 +47,17 @@ 
     scmutil.match = f
     return oldmatch
 
+def installmatchandpatsfn(f):
+    oldmatchandpats = scmutil.matchandpats
+    setattr(f, 'oldmatchandpats', oldmatchandpats)
+    scmutil.matchandpats = f
+    return oldmatchandpats
+
+def installfilelogfn(f):
+    oldsymbol = revset.symbols['filelog']
+    revset.symbols['filelog'] = f
+    return oldsymbol
+
 def restorematchfn():
     '''restores scmutil.match to what it was before installnormalfilesmatchfn
     was called.  no-op if scmutil.match is its original function.
@@ -55,6 +66,20 @@ 
     restore matchfn to reverse'''
     scmutil.match = getattr(scmutil.match, 'oldmatch', scmutil.match)
 
+def restorematchandpatsfn():
+    '''restores scmutil.matchandpats to what it was before
+    installnormalfilesmatchandpatsfn was called.  no-op if scmutil.matchandpats
+    is its original function.
+
+    Note that n calls to installnormalfilesmatchandpatsfn will require n calls
+    to restore matchfn to reverse'''
+    scmutil.matchandpats = getattr(scmutil.matchandpats, 'oldmatchandpats',
+            scmutil.matchandpats)
+
+def restorefilelogfn(oldsymbol=revset.filelog):
+    '''restores revset.filelog to what it was before.'''
+    revset.symbols['filelog'] = oldsymbol
+
 def addlargefiles(ui, repo, *pats, **opts):
     large = opts.pop('large', None)
     lfsize = lfutil.getminsize(
@@ -241,15 +266,15 @@ 
         repo._repo.lfstatus = False
 
 def overridelog(orig, ui, repo, *pats, **opts):
-    def overridematch(ctx, pats=[], opts={}, globbed=False,
+    def overridematchandpats(ctx, pats=[], opts={}, globbed=False,
             default='relpath'):
         """Matcher that merges root directory with .hglf, suitable for log.
         It is still possible to match .hglf directly.
         For any listed files run log on the standin too.
         matchfn tries both the given filename and with .hglf stripped.
         """
-        match = oldmatch(ctx, pats, opts, globbed, default)
-        m = copy.copy(match)
+        matchandpats = oldmatchandpats(ctx, pats, opts, globbed, default)
+        m, p = copy.copy(matchandpats)
         for i in range(0, len(m._files)):
             standin = lfutil.standin(m._files[i])
             if standin in repo[ctx.node()]:
@@ -264,14 +289,74 @@ 
             r = origmatchfn(f)
             return r
         m.matchfn = lfmatchfn
-        return m
-    oldmatch = installmatchfn(overridematch)
+
+        def expandpats(pats):
+            ret = set()
+            for p in pats:
+                kind, name = match_._patsplit(p, None)
+                if kind is None:
+                    try:
+                        globbed = glob.glob(name)
+                    except re.error:
+                        globbed = set([name])
+                    if globbed:
+                        ret.update(globbed)
+                        for g in globbed:
+                            if not lfutil.isstandin(g):
+                                ret.add(lfutil.standin(g))
+                        continue
+                ret.add(p)
+            return ret
+
+        if pats == ("",):
+            pats = set()
+        else:
+            pats = expandpats(pats or [])
+
+        # Add the new matched files to the pats
+        for f in m.files():
+            if f not in pats:
+                pats.add(f)
+                if lfutil.isstandin(f):
+                    slf = lfutil.splitstandin(f)
+                else:
+                    slf = lfutil.standin(f)
+                if slf not in pats:
+                    pats.add(slf)
+
+        return m, pats
+
+    def overridefilelog(repo, subset, x):
+        '''override filelog revset, use relative paths instead of
+        pathutil.canonpath to look for a pat's filelog
+        '''
+        # i18n: "filelog" is a keyword
+        pat = revset.getstring(x, _("filelog requires a pattern"))
+        s = set()
+
+        if not match_.patkind(pat):
+            fl = repo.file(pat)
+            for fr in fl:
+                s.add(fl.linkrev(fr))
+        else:
+            m = match_.match(repo.root, repo.getcwd(), [pat], ctx=repo[None])
+            for f in repo[None]:
+                if m(f):
+                    fl = repo.file(f)
+                    for fr in fl:
+                        s.add(fl.linkrev(fr))
+
+        return subset.filter(lambda r: r in s)
+
+    oldmatchandpats = installmatchandpatsfn(overridematchandpats)
+    oldsymbol = installfilelogfn(overridefilelog)
     try:
         repo.lfstatus = True
         return orig(ui, repo, *pats, **opts)
     finally:
         repo.lfstatus = False
-        restorematchfn()
+        restorematchandpatsfn()
+        restorefilelogfn(oldsymbol)
 
 def overrideverify(orig, ui, repo, *pats, **opts):
     large = opts.pop('large', False)