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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
632–633

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 ↗(On Diff #17750)

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 ↗(On Diff #17750)

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 ↗(On Diff #17750)

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 ↗(On Diff #17750)

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 ↗(On Diff #17750)

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 17824

hgext/narrow/narrowcommands.py

discovery, discovery,
encoding, encoding,
error, error,
exchange, exchange,
extensions, extensions,
hg, hg,
narrowspec, narrowspec,
node, node,
pathutil,
pycompat, pycompat,
registrar, registrar,
repair, repair,
repoview, repoview,
sparse, sparse,
util, util,
wireprototypes, wireprototypes,
) )
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(pathutil.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

from mercurial import ( from mercurial import (
cmdutil, cmdutil,
commands, commands,
context, context,
copies as copiesmod, copies as copiesmod,
error, error,
node, node,
obsutil, obsutil,
pathutil,
pycompat, pycompat,
registrar, registrar,
rewriteutil, rewriteutil,
scmutil, scmutil,
util,
) )
cmdtable = {} cmdtable = {}
command = registrar.command(cmdtable) command = registrar.command(cmdtable)
configtable = {} configtable = {}
configitem = registrar.configitem(configtable) configitem = registrar.configitem(configtable)
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 pathutil.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

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 = pathutil.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

""" """
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 pathutil.dirs(self._map, b'r')
@propertycache @propertycache
def _alldirs(self): def _alldirs(self):
return util.dirs(self._map) return pathutil.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

hex, hex,
nullid, nullid,
nullrev, nullrev,
) )
from .pycompat import getattr from .pycompat import getattr
from . import ( from . import (
error, error,
mdiff, mdiff,
pathutil,
policy, policy,
pycompat, pycompat,
revlog, revlog,
util, util,
) )
from .interfaces import ( from .interfaces import (
repository, repository,
util as interfaceutil, util as interfaceutil,
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 pathutil.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 pathutil.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

import re import re
from .i18n import _ from .i18n import _
from .pycompat import open from .pycompat import open
from . import ( from . import (
encoding, encoding,
error, error,
pathutil, pathutil,
pathutil,
policy, policy,
pycompat, pycompat,
util, util,
) )
from .utils import stringutil from .utils import stringutil
rustmod = policy.importrust('filepatterns') rustmod = policy.importrust('filepatterns')
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(pathutil.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 pathutil.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(pathutil.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(pathutil.dirs(d))
p.update(util.dirs(r)) p.update(pathutil.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/pathutil.py

from __future__ import absolute_import from __future__ import absolute_import
import errno import errno
import os import os
import posixpath import posixpath
import stat import stat
from .i18n import _ from .i18n import _
from . import ( from . import (
encoding, encoding,
error, error,
policy,
pycompat, pycompat,
util, util,
) )
rustdirs = policy.importrust('dirstate', 'Dirs')
parsers = policy.importmod('parsers')
def _lowerclean(s): def _lowerclean(s):
return encoding.hfsignoreclean(s.lower()) return encoding.hfsignoreclean(s.lower())
class pathauditor(object): class pathauditor(object):
'''ensure that a filesystem path contains no banned components. '''ensure that a filesystem path contains no banned components.
the following properties of a path are checked: the following properties of a path are checked:
''' '''
d, p = os.path.splitdrive(path) d, p = os.path.splitdrive(path)
if len(p) != len(pycompat.ossep): if len(p) != len(pycompat.ossep):
return path + pycompat.ossep return path + pycompat.ossep
else: else:
return path return path
class dirs(object):
'''a multiset of directory names from a set of file paths'''
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.endswith(b'/'):
raise ValueError(
"found invalid consecutive slashes in path: %r" % base
)
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
# forward two methods from posixpath that do what we need, but we'd # forward two methods from posixpath that do what we need, but we'd
# rather not let our internals know that we're thinking in posix terms # rather not let our internals know that we're thinking in posix terms
# - instead we'll let them be oblivious. # - instead we'll let them be oblivious.
join = posixpath.join join = posixpath.join
dirname = posixpath.dirname dirname = posixpath.dirname

mercurial/repair.py

from . import ( from . import (
bundle2, bundle2,
changegroup, changegroup,
discovery, discovery,
error, error,
exchange, exchange,
obsolete, obsolete,
obsutil, obsutil,
pathutil,
phases, phases,
pycompat, pycompat,
util, util,
) )
from .utils import stringutil from .utils import stringutil
def backupbundle( def backupbundle(
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 pathutil.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('dirstate', 'Dirs')
base85 = policy.importmod('base85') base85 = policy.importmod('base85')
osutil = policy.importmod('osutil') osutil = policy.importmod('osutil')
parsers = policy.importmod('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.endswith(b'/'):
raise ValueError(
"found invalid consecutive slashes in path: %r" % base
)
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):
""" """

tests/test-dirs.py

from __future__ import absolute_import from __future__ import absolute_import
import unittest import unittest
import silenttestrunner import silenttestrunner
from mercurial import util from mercurial import pathutil
class dirstests(unittest.TestCase): class dirstests(unittest.TestCase):
def testdirs(self): def testdirs(self):
for case, want in [ for case, want in [
(b'a/a/a', [b'a', b'a/a', b'']), (b'a/a/a', [b'a', b'a/a', b'']),
(b'alpha/beta/gamma', [b'', b'alpha', b'alpha/beta']), (b'alpha/beta/gamma', [b'', b'alpha', b'alpha/beta']),
]: ]:
d = util.dirs({}) d = pathutil.dirs({})
d.addpath(case) d.addpath(case)
self.assertEqual(sorted(d), sorted(want)) self.assertEqual(sorted(d), sorted(want))
def testinvalid(self): def testinvalid(self):
with self.assertRaises(ValueError): with self.assertRaises(ValueError):
d = util.dirs({}) d = pathutil.dirs({})
d.addpath(b'a//b') d.addpath(b'a//b')
if __name__ == '__main__': if __name__ == '__main__':
silenttestrunner.main(__name__) silenttestrunner.main(__name__)