This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7311

utils: move the `dirs` definition in pathutil (API)
ClosedPublic

Authored by marmoute on Nov 8 2019, 4:28 AM.

Details

Reviewers
durin42
martinvonz
Alphare
indygreg
Group Reviewers
hg-reviewers
Commits
rHGc21aca51b392: utils: move the `dirs` definition in pathutil (API)
rHGe170e425c88d: utils: move the `dirs` definition in pathutil (API)
Summary

Before this change, the dirs class was accessible through the mercurial.util
module. That module is expected to stay free of scm specific content.

The pathutil destination has been selection by Martin von Zweigbergk.

This work is part of a refactoring to unify the revlog index and the nodemap.
This unification prepare the use of a persistent nodemap.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

marmoute created this revision.Nov 8 2019, 4:28 AM
Alphare accepted this revision.Nov 8 2019, 5:21 AM
durin42 added a comment.Nov 8 2019, 8:11 AM

Can you fix up the fuzzer in contrib/fuzz/dirstate.cc?

Alphare added a comment.Nov 8 2019, 10:08 AM

Note that there is a comment in match.py about util.dirs. It's not that important, but it should be updated.

marmoute added a comment.Nov 8 2019, 10:18 AM
In D7311#107111, @durin42 wrote:

Can you fix up the fuzzer in contrib/fuzz/dirstate.cc?

I do not understand what need fixing there. Are you talking about D7314 ?

durin42 added a comment.Nov 8 2019, 10:21 AM
In D7311#107177, @marmoute wrote:
In D7311#107111, @durin42 wrote:

Can you fix up the fuzzer in contrib/fuzz/dirstate.cc?

I do not understand what need fixing there. Are you talking about D7314 ?

I was talking about this patch. https://www.mercurial-scm.org/repo/hg/file/tip/contrib/fuzz/dirs.cc#l18 is what I was talking about: some of our fuzzers actually contain a tiny Python script that gets evaluated to drive the fuzzing.

It turns out in this case it's not a problem. :)

Alphare added inline comments.Nov 8 2019, 10:22 AM
mercurial/match.py
631–632

Should be dirstateutil.dirs

marmoute updated this revision to Diff 17750.Nov 8 2019, 10:34 AM
martinvonz added inline comments.Nov 8 2019, 2:05 PM
mercurial/utils/dirstateutil.py
14

Is pathutil.py an appropriate home for this instead of creating a new file?

indygreg requested changes to this revision.Nov 8 2019, 2:23 PM
indygreg added a subscriber: indygreg.

I agree with Martin that we should put this in pathutils.

This revision now requires changes to proceed.Nov 8 2019, 2:23 PM
marmoute added inline comments.Nov 8 2019, 7:19 PM
mercurial/utils/dirstateutil.py
14

I feel like pathutil.py is mostly scm agnostic and I would rather keep it this way.

martinvonz added inline comments.Nov 8 2019, 7:22 PM
mercurial/utils/dirstateutil.py
14

But how is dirs not scm-agnostic? Oh, you mean the ugly skip thing? Can we see if we can just remove that instead (move it outside of the class)?

marmoute added inline comments.Nov 8 2019, 7:39 PM
mercurial/utils/dirstateutil.py
14

It explicitly mention dirstate and manifest. Both are scm related context.

martinvonz added inline comments.Nov 8 2019, 7:41 PM
mercurial/utils/dirstateutil.py
14

That's easy to fix, though. Just replace "dirstate or manifest" by "set of file paths".

marmoute retitled this revision from utils: move the `dirs` definition in a dedicated module (API) to utils: move the `dirs` definition in pathutil (API).Nov 8 2019, 8:09 PM
marmoute edited the summary of this revision. (Show Details)
marmoute updated this revision to Diff 17821.
martinvonz added inline comments.Nov 8 2019, 8:32 PM
mercurial/util.py
3494–3499

Could you follow up with a patch to move this one as well?

martinvonz added a comment.Nov 8 2019, 8:36 PM

FYI, I had to resolve some conflicts with Augie's recent patches for removing r'' prefixes and for consecutive slashes. I'm pretty sure I did it right, but take a look if you want.

marmoute added a commit: rHGe170e425c88d: utils: move the `dirs` definition in pathutil (API).Nov 8 2019, 8:53 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGe170e425c88d: utils: move the `dirs` definition in pathutil (API) (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 17750

hgext/narrow/narrowcommands.py

registrar, registrar,
repair, repair,
repoview, repoview,
sparse, sparse,
util, util,
wireprototypes, wireprototypes,
) )
from mercurial.interfaces import repository from mercurial.interfaces import repository
from mercurial.utils import dirstateutil
table = {} table = {}
command = registrar.command(table) command = registrar.command(table)
def setup(): def setup():
"""Wraps user-facing mercurial commands with narrow-aware versions.""" """Wraps user-facing mercurial commands with narrow-aware versions."""
todelete = [] todelete = []
for f, f2, size in repo.store.datafiles(): for f, f2, size in repo.store.datafiles():
if f.startswith(b'data/'): if f.startswith(b'data/'):
file = f[5:-2] file = f[5:-2]
if not newmatch(file): if not newmatch(file):
todelete.append(f) todelete.append(f)
elif f.startswith(b'meta/'): elif f.startswith(b'meta/'):
dir = f[5:-13] dir = f[5:-13]
dirs = sorted(util.dirs({dir})) + [dir] dirs = sorted(dirstateutil.dirs({dir})) + [dir]
include = True include = True
for d in dirs: for d in dirs:
visit = newmatch.visitdir(d) visit = newmatch.visitdir(d)
if not visit: if not visit:
include = False include = False
break break
if visit == b'all': if visit == b'all':
break break

hgext/uncommit.py

copies as copiesmod, copies as copiesmod,
error, error,
node, node,
obsutil, obsutil,
pycompat, pycompat,
registrar, registrar,
rewriteutil, rewriteutil,
scmutil, scmutil,
util,
) )
from mercurial.utils import dirstateutil
cmdtable = {} cmdtable = {}
command = registrar.command(cmdtable) command = registrar.command(cmdtable)
configtable = {} configtable = {}
configitem = registrar.configitem(configtable) configitem = registrar.configitem(configtable)
configitem( configitem(
eligible = set(s.added) | set(s.modified) | set(s.removed) eligible = set(s.added) | set(s.modified) | set(s.removed)
badfiles = set(match.files()) - eligible badfiles = set(match.files()) - eligible
# Naming a parent directory of an eligible file is OK, even # Naming a parent directory of an eligible file is OK, even
# if not everything tracked in that directory can be # if not everything tracked in that directory can be
# uncommitted. # uncommitted.
if badfiles: if badfiles:
badfiles -= {f for f in util.dirs(eligible)} badfiles -= {f for f in dirstateutil.dirs(eligible)}
for f in sorted(badfiles): for f in sorted(badfiles):
if f in s.clean: if f in s.clean:
hint = _( hint = _(
b"file was not changed in working directory parent" b"file was not changed in working directory parent"
) )
elif repo.wvfs.exists(f): elif repo.wvfs.exists(f):
hint = _(b"file was untracked in working directory parent") hint = _(b"file was untracked in working directory parent")

mercurial/cmdutil.py

templatekw, templatekw,
templater, templater,
util, util,
vfs as vfsmod, vfs as vfsmod,
) )
from .utils import ( from .utils import (
dateutil, dateutil,
dirstateutil,
stringutil, stringutil,
) )
stringio = util.stringio stringio = util.stringio
# templates of common command options # templates of common command options
dryrunopts = [ dryrunopts = [
except error.LookupError: except error.LookupError:
warnings.append( warnings.append(
_(b"skipping missing subrepository: %s\n") _(b"skipping missing subrepository: %s\n")
% uipathfn(subpath) % uipathfn(subpath)
) )
progress.complete() progress.complete()
# warn about failure to delete explicit files/dirs # warn about failure to delete explicit files/dirs
deleteddirs = util.dirs(deleted) deleteddirs = dirstateutil.dirs(deleted)
files = m.files() files = m.files()
progress = ui.makeprogress( progress = ui.makeprogress(
_(b'deleting'), total=len(files), unit=_(b'files') _(b'deleting'), total=len(files), unit=_(b'files')
) )
for f in files: for f in files:
def insubrepo(): def insubrepo():
for subpath in wctx.substate: for subpath in wctx.substate:

mercurial/dirstate.py

util, util,
) )
from .interfaces import ( from .interfaces import (
dirstate as intdirstate, dirstate as intdirstate,
util as interfaceutil, util as interfaceutil,
) )
from .utils import dirstateutil
parsers = policy.importmod(r'parsers') parsers = policy.importmod(r'parsers')
rustmod = policy.importrust(r'dirstate') rustmod = policy.importrust(r'dirstate')
propertycache = util.propertycache propertycache = util.propertycache
filecache = scmutil.filecache filecache = scmutil.filecache
_rangemask = 0x7FFFFFFF _rangemask = 0x7FFFFFFF
dirstatetuple = parsers.dirstatetuple dirstatetuple = parsers.dirstatetuple
""" """
Returns True if the dirstate contains a file (tracked or removed) Returns True if the dirstate contains a file (tracked or removed)
in this directory. in this directory.
""" """
return d in self._alldirs return d in self._alldirs
@propertycache @propertycache
def _dirs(self): def _dirs(self):
return util.dirs(self._map, b'r') return dirstateutil.dirs(self._map, b'r')
@propertycache @propertycache
def _alldirs(self): def _alldirs(self):
return util.dirs(self._map) return dirstateutil.dirs(self._map)
def _opendirstatefile(self): def _opendirstatefile(self):
fp, mode = txnutil.trypending(self._root, self._opener, self._filename) fp, mode = txnutil.trypending(self._root, self._opener, self._filename)
if self._pendingmode is not None and self._pendingmode != mode: if self._pendingmode is not None and self._pendingmode != mode:
fp.close() fp.close()
raise error.Abort( raise error.Abort(
_(b'working directory state may be changed parallelly') _(b'working directory state may be changed parallelly')
) )

mercurial/manifest.py

pycompat, pycompat,
revlog, revlog,
util, util,
) )
from .interfaces import ( from .interfaces import (
repository, repository,
util as interfaceutil, util as interfaceutil,
) )
from .utils import dirstateutil
parsers = policy.importmod(r'parsers') parsers = policy.importmod(r'parsers')
propertycache = util.propertycache propertycache = util.propertycache
# Allow tests to more easily test the alternate path in manifestdict.fastdelta() # Allow tests to more easily test the alternate path in manifestdict.fastdelta()
FASTDELTA_TEXTDIFF_THRESHOLD = 1000 FASTDELTA_TEXTDIFF_THRESHOLD = 1000
filepath filepath
for filepath, hashflags in pycompat.iteritems(diff) for filepath, hashflags in pycompat.iteritems(diff)
if hashflags[1][0] is None if hashflags[1][0] is None
) )
return files return files
@propertycache @propertycache
def _dirs(self): def _dirs(self):
return util.dirs(self) return dirstateutil.dirs(self)
def dirs(self): def dirs(self):
return self._dirs return self._dirs
def hasdir(self, dir): def hasdir(self, dir):
return dir in self._dirs return dir in self._dirs
def _filesfastpath(self, match): def _filesfastpath(self, match):
if fn not in t2._files: if fn not in t2._files:
files.add(t1._subpath(fn)) files.add(t1._subpath(fn))
_filesnotin(self, m2) _filesnotin(self, m2)
return files return files
@propertycache @propertycache
def _alldirs(self): def _alldirs(self):
return util.dirs(self) return dirstateutil.dirs(self)
def dirs(self): def dirs(self):
return self._alldirs return self._alldirs
def hasdir(self, dir): def hasdir(self, dir):
self._load() self._load()
topdir, subdir = _splittopdir(dir) topdir, subdir = _splittopdir(dir)
if topdir: if topdir:

mercurial/match.py

from . import ( from . import (
encoding, encoding,
error, error,
pathutil, pathutil,
policy, policy,
pycompat, pycompat,
util, util,
) )
from .utils import stringutil from .utils import (
dirstateutil,
stringutil,
)
rustmod = policy.importrust(r'filepatterns') rustmod = policy.importrust(r'filepatterns')
allpatternkinds = ( allpatternkinds = (
b're', b're',
b'glob', b'glob',
b'path', b'path',
b'relglob', b'relglob',
super(patternmatcher, self).__init__(badfn) super(patternmatcher, self).__init__(badfn)
self._files = _explicitfiles(kindpats) self._files = _explicitfiles(kindpats)
self._prefix = _prefix(kindpats) self._prefix = _prefix(kindpats)
self._pats, self.matchfn = _buildmatch(kindpats, b'$', root) self._pats, self.matchfn = _buildmatch(kindpats, b'$', root)
@propertycache @propertycache
def _dirs(self): def _dirs(self):
return set(util.dirs(self._fileset)) return set(dirstateutil.dirs(self._fileset))
def visitdir(self, dir): def visitdir(self, dir):
dir = normalizerootdir(dir, b'visitdir') dir = normalizerootdir(dir, b'visitdir')
if self._prefix and dir in self._fileset: if self._prefix and dir in self._fileset:
return b'all' return b'all'
return ( return (
dir in self._fileset dir in self._fileset
or dir in self._dirs or dir in self._dirs
def prefix(self): def prefix(self):
return self._prefix return self._prefix
@encoding.strmethod @encoding.strmethod
def __repr__(self): def __repr__(self):
return b'<patternmatcher patterns=%r>' % pycompat.bytestr(self._pats) return b'<patternmatcher patterns=%r>' % pycompat.bytestr(self._pats)
# This is basically a reimplementation of util.dirs that stores the children # This is basically a reimplementation of dirstateutil.dirs that stores the
AlphareUnsubmitted
Not Done

Should be dirstateutil.dirs

Alphare: Should be `dirstateutil.dirs`
# instead of just a count of them, plus a small optional optimization to avoid # children instead of just a count of them, plus a small optional optimization
# some directories we don't need. # to avoid some directories we don't need.
class _dirchildren(object): class _dirchildren(object):
def __init__(self, paths, onlyinclude=None): def __init__(self, paths, onlyinclude=None):
self._dirs = {} self._dirs = {}
self._onlyinclude = onlyinclude or [] self._onlyinclude = onlyinclude or []
addpath = self.addpath addpath = self.addpath
for f in paths: for f in paths:
addpath(f) addpath(f)
self._files = files self._files = files
else: else:
self._files = list(files) self._files = list(files)
matchfn = basematcher.exact matchfn = basematcher.exact
@propertycache @propertycache
def _dirs(self): def _dirs(self):
return set(util.dirs(self._fileset)) return set(dirstateutil.dirs(self._fileset))
def visitdir(self, dir): def visitdir(self, dir):
dir = normalizerootdir(dir, b'visitdir') dir = normalizerootdir(dir, b'visitdir')
return dir in self._dirs return dir in self._dirs
def visitchildrenset(self, dir): def visitchildrenset(self, dir):
dir = normalizerootdir(dir, b'visitchildrenset') dir = normalizerootdir(dir, b'visitchildrenset')
>>> print(r[0:2], sorted(r[2])) # the set has an unstable output >>> print(r[0:2], sorted(r[2])) # the set has an unstable output
(['', '', ''], []) [''] (['', '', ''], []) ['']
''' '''
r, d = _patternrootsanddirs(kindpats) r, d = _patternrootsanddirs(kindpats)
p = set() p = set()
# Add the parents as non-recursive/exact directories, since they must be # Add the parents as non-recursive/exact directories, since they must be
# scanned to get to either the roots or the other exact directories. # scanned to get to either the roots or the other exact directories.
p.update(util.dirs(d)) p.update(dirstateutil.dirs(d))
p.update(util.dirs(r)) p.update(dirstateutil.dirs(r))
# FIXME: all uses of this function convert these to sets, do so before # FIXME: all uses of this function convert these to sets, do so before
# returning. # returning.
# FIXME: all uses of this function do not need anything in 'roots' and # FIXME: all uses of this function do not need anything in 'roots' and
# 'dirs' to also be in 'parents', consider removing them before returning. # 'dirs' to also be in 'parents', consider removing them before returning.
return r, d, p return r, d, p

mercurial/repair.py

error, error,
exchange, exchange,
obsolete, obsolete,
obsutil, obsutil,
phases, phases,
pycompat, pycompat,
util, util,
) )
from .utils import stringutil from .utils import (
dirstateutil,
stringutil,
)
def backupbundle( def backupbundle(
repo, bases, heads, node, suffix, compress=True, obsolescence=True repo, bases, heads, node, suffix, compress=True, obsolescence=True
): ):
"""create a bundle with the specified revisions as a backup""" """create a bundle with the specified revisions as a backup"""
backupdir = b"strip-backup" backupdir = b"strip-backup"
if repo.store._exists(d): if repo.store._exists(d):
newentries.add(d) newentries.add(d)
progress.complete() progress.complete()
if b'treemanifest' in repo.requirements: if b'treemanifest' in repo.requirements:
# This logic is safe if treemanifest isn't enabled, but also # This logic is safe if treemanifest isn't enabled, but also
# pointless, so we skip it if treemanifest isn't enabled. # pointless, so we skip it if treemanifest isn't enabled.
for dir in util.dirs(seenfiles): for dir in dirstateutil.dirs(seenfiles):
i = b'meta/%s/00manifest.i' % dir i = b'meta/%s/00manifest.i' % dir
d = b'meta/%s/00manifest.d' % dir d = b'meta/%s/00manifest.d' % dir
if repo.store._exists(i): if repo.store._exists(i):
newentries.add(i) newentries.add(i)
if repo.store._exists(d): if repo.store._exists(d):
newentries.add(d) newentries.add(d)

mercurial/util.py

urllibcompat, urllibcompat,
) )
from .utils import ( from .utils import (
compression, compression,
procutil, procutil,
stringutil, stringutil,
) )
rustdirs = policy.importrust(r'dirstate', r'Dirs')
base85 = policy.importmod(r'base85') base85 = policy.importmod(r'base85')
osutil = policy.importmod(r'osutil') osutil = policy.importmod(r'osutil')
parsers = policy.importmod(r'parsers')
b85decode = base85.b85decode b85decode = base85.b85decode
b85encode = base85.b85encode b85encode = base85.b85encode
cookielib = pycompat.cookielib cookielib = pycompat.cookielib
httplib = pycompat.httplib httplib = pycompat.httplib
pickle = pycompat.pickle pickle = pycompat.pickle
safehasattr = pycompat.safehasattr safehasattr = pycompat.safehasattr
if otherf: if otherf:
otherf.flush() otherf.flush()
f.write(b'%s at:\n' % msg.rstrip()) f.write(b'%s at:\n' % msg.rstrip())
for line in getstackframes(skip + 1, depth=depth): for line in getstackframes(skip + 1, depth=depth):
f.write(line) f.write(line)
f.flush() f.flush()
class dirs(object):
'''a multiset of directory names from a dirstate or manifest'''
def __init__(self, map, skip=None):
self._dirs = {}
addpath = self.addpath
if isinstance(map, dict) and skip is not None:
for f, s in pycompat.iteritems(map):
if s[0] != skip:
addpath(f)
elif skip is not None:
raise error.ProgrammingError(
b"skip character is only supported with a dict source"
)
else:
for f in map:
addpath(f)
def addpath(self, path):
dirs = self._dirs
for base in finddirs(path):
if base in dirs:
dirs[base] += 1
return
dirs[base] = 1
def delpath(self, path):
dirs = self._dirs
for base in finddirs(path):
if dirs[base] > 1:
dirs[base] -= 1
return
del dirs[base]
def __iter__(self):
return iter(self._dirs)
def __contains__(self, d):
return d in self._dirs
if safehasattr(parsers, 'dirs'):
dirs = parsers.dirs
if rustdirs is not None:
dirs = rustdirs
def finddirs(path): def finddirs(path):
pos = path.rfind(b'/') pos = path.rfind(b'/')
while pos != -1: while pos != -1:
yield path[:pos] yield path[:pos]
pos = path.rfind(b'/', 0, pos) pos = path.rfind(b'/', 0, pos)
yield b'' yield b''
martinvonzUnsubmitted
Not Done

Could you follow up with a patch to move this one as well?

martinvonz: Could you follow up with a patch to move this one as well?
# convenient shortcut # convenient shortcut
dst = debugstacktrace dst = debugstacktrace
def safename(f, tag, ctx, others=None): def safename(f, tag, ctx, others=None):
""" """

mercurial/utils/dirstateutil.py

  • This file was added.
from __future__ import absolute_import
from .. import (
error,
policy,
pycompat,
util,
)
rustdirs = policy.importrust(r'dirstate', r'Dirs')
parsers = policy.importmod(r'parsers')
class dirs(object):
martinvonzUnsubmitted
Not Done

Is pathutil.py an appropriate home for this instead of creating a new file?

martinvonz: Is `pathutil.py` an appropriate home for this instead of creating a new file?
marmouteAuthorUnsubmitted
Done

I feel like pathutil.py is mostly scm agnostic and I would rather keep it this way.

marmoute: I feel like `pathutil.py` is mostly scm agnostic and I would rather keep it this way.
martinvonzUnsubmitted
Not Done

But how is dirs not scm-agnostic? Oh, you mean the ugly skip thing? Can we see if we can just remove that instead (move it outside of the class)?

martinvonz: But how is `dirs` not scm-agnostic? Oh, you mean the ugly `skip` thing? Can we see if we can…
marmouteAuthorUnsubmitted
Not Done

It explicitly mention dirstate and manifest. Both are scm related context.

marmoute: It explicitly mention dirstate and manifest. Both are scm related context.
martinvonzUnsubmitted
Not Done

That's easy to fix, though. Just replace "dirstate or manifest" by "set of file paths".

martinvonz: That's easy to fix, though. Just replace "dirstate or manifest" by "set of file paths".
'''a multiset of directory names from a dirstate or manifest'''
def __init__(self, map, skip=None):
self._dirs = {}
addpath = self.addpath
if isinstance(map, dict) and skip is not None:
for f, s in pycompat.iteritems(map):
if s[0] != skip:
addpath(f)
elif skip is not None:
raise error.ProgrammingError(
b"skip character is only supported with a dict source"
)
else:
for f in map:
addpath(f)
def addpath(self, path):
dirs = self._dirs
for base in util.finddirs(path):
if base in dirs:
dirs[base] += 1
return
dirs[base] = 1
def delpath(self, path):
dirs = self._dirs
for base in util.finddirs(path):
if dirs[base] > 1:
dirs[base] -= 1
return
del dirs[base]
def __iter__(self):
return iter(self._dirs)
def __contains__(self, d):
return d in self._dirs
if util.safehasattr(parsers, 'dirs'):
dirs = parsers.dirs
if rustdirs is not None:
dirs = rustdirs