[PATCH 1 of 2 RFC V2] largefiles: changed overridelog to work with graphlog
Mads Kiilerich
mads at kiilerich.com
Thu Mar 6 12:26:09 CST 2014
On 03/06/2014 01:10 AM, Lucas Moscovicz wrote:
> # HG changeset patch
> # User Lucas Moscovicz <lmoscovicz at fb.com>
> # Date 1394063709 28800
> # Wed Mar 05 15:55:09 2014 -0800
> # Node ID 2367ddb8b07acef2ea7f72936c0ba46f47908205
> # Parent ef75710021e1c1dd2ef192c21b274313698186d4
> largefiles: changed overridelog to work with graphlog
You have my sincere respect and sympathy for looking into this ...
> Log for largefiles was failing for graphlog since it was overriding match
> instead of matchandpats.
Did it already not work and this is a bugfix? Or is it a change of
strategy that will make it work with the refactoring in the next changeset?
> Tests for graphlog to be added in future patches.
Why not introduce the tests in a patch before this or in this patch so
we can see that it improves (or at least doesn't break)?
> 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,15 @@
> scmutil.match = f
> return oldmatch
>
> +def installmatchandpatsfn(f):
> + oldmatchandpats = scmutil.matchandpats
> + setattr(f, 'oldmatchandpats', oldmatchandpats)
> + scmutil.matchandpats = f
> + return oldmatchandpats
> +
> +def installfilelogfn(f):
> + revset.symbols['filelog'] = f
"usually" we store the old function and make sure we restore the right
one when we are done. Shouldn't we do the same here?
> +
> def restorematchfn():
> '''restores scmutil.match to what it was before installnormalfilesmatchfn
> was called. no-op if scmutil.match is its original function.
> @@ -55,6 +64,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():
> + '''restores revset.filelog to what it was before.'''
> + revset.symbols['filelog'] = revset.filelog
> +
> def addlargefiles(ui, repo, *pats, **opts):
> large = opts.pop('large', None)
> lfsize = lfutil.getminsize(
> @@ -241,15 +264,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 +287,81 @@
> r = origmatchfn(f)
> return r
> m.matchfn = lfmatchfn
> - return m
> - oldmatch = installmatchfn(overridematch)
> +
> + def expandpats(pats):
> + ret = []
> + for p in pats:
> + kind, name = match_._patsplit(p, None)
> + if kind is None:
> + try:
> + globbed = glob.glob(name)
> + except re.error:
> + globbed = [name]
> + if globbed:
> + ret.extend(globbed)
> + for g in globbed:
> + if not lfutil.isstandin(g):
> + ret.append(lfutil.standin(g))
I would expect that it only should use the standin if the matched file
itself not is tracked directly but is a largefile - which probably
approximately means that it is in lfdirstate or the standin exists in
the filesystem or revision. I also wonder if the standins should be used
instead of the matched file ...
BUT this is for log and across multi revisions so I guess it is less
wrong to include both and see what they match? That might deserve a comment.
> + continue
> + ret.append(p)
> + return ret
This expandpats function seems to be an almost exact copy of
scmutil.expandpats, only with some post processing? It is only used once
- a few lines below? Why not just call scmutil.expandpats and
postprocess the result?
> +
> + if pats == ("",):
> + pats = []
> + else:
> + pats = expandpats(pats or [])
> +
> + for f in m.files():
> + if f not in pats:
pats is a list. Could we use sets instead?
> + pats.append(f)
> + if not lfutil.isstandin(f):
> + slf = lfutil.standin(f)
> + else:
> + slf = lfutil.splitstandin(f)
(minor it: un-negate the expression and swap the true and false branches
to get more readable code?)
> + if not slf in pats:
> + pats.append(slf)
> + return m, pats
> +
> + def overridefilelog(repo, subset, x):
In what way is this different from the real filelog function and why?
That might deserve a comment.
> + """``filelog(pattern)``
> + Changesets connected to the specified filelog.
> +
> + For performance reasons, ``filelog()`` does not show every changeset
> + that affects the requested file(s). See :hg:`help log` for details. For
> + a slower, more accurate result, use ``file()``.
> +
> + The pattern without explicit kind like ``glob:`` is expected to be
> + relative to the current directory and match against a file exactly
> + for efficiency.
> + """
> +
> + # 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)
> + installfilelogfn(overridefilelog)
> try:
> repo.lfstatus = True
> return orig(ui, repo, *pats, **opts)
> finally:
> repo.lfstatus = False
> - restorematchfn()
> + restorematchandpatsfn()
> + restorefilelogfn()
>
> def overrideverify(orig, ui, repo, *pats, **opts):
> large = opts.pop('large', False)
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -428,7 +428,7 @@
> '''Return a list of revisions matching the given revset'''
> expr = revset.formatspec(expr, *args)
> m = revset.match(None, expr)
> - return revset.baseset([r for r in m(self, revset.baseset(self))])
> + return m(self, revset.spanset(self))
Unrelated change?
/Mads
More information about the Mercurial-devel
mailing list