D1974: narrow: import experimental extension from narrowhg revision cb51d673e9c5

[indygreg (Gregory Szorc)]

indygreg added a comment.


  I'm still reviewing. Just thought I'd flush another batch of feedback :)

INLINE COMMENTS

> narrowrepo.py:28-33
> +def wrappostshare(orig, sourcerepo, destrepo, **kwargs):
> +    orig(sourcerepo, destrepo, **kwargs)
> +    if requirement in sourcerepo.requirements:
> +        with destrepo.wlock():
> +            with destrepo.vfs('shared', 'a') as fp:
> +                fp.write(narrowspec.FILENAME + '\n')

Is this really the best way to influence the content of the `shared` file? If it doesn't exist, a good follow-up would be a hook point in core to allow extensions to modify the content of this file so it is only written once.

> narrowrepo.py:35-43
> +def unsharenarrowspec(orig, ui, repo, repopath):
> +    if (requirement in repo.requirements
> +        and repo.path == repopath and repo.shared()):
> +        srcrepo = share._getsrcrepo(repo)
> +        with srcrepo.vfs(narrowspec.FILENAME) as f:
> +            spec = f.read()
> +        with repo.vfs(narrowspec.FILENAME, 'w') as f:

Again, my reaction is "eww." But this one is a bit special since we're dealing with a non-standard file. So unless we create a "also write these files with this content" mechanism (which doesn't feel necessary), this is probably the best we can do.

This code should live in core eventually though. So it should work itself out.

> narrowrepo.py:75
> +        @localrepo.repofilecache(narrowspec.FILENAME)
> +        def narrowpats(self):
> +            return narrowspec.load(self)

Please add a docstring, since this is a new public method.

> narrowrepo.py:85-87
> +        # TODO(martinvonz): make this property-like instead?
> +        def narrowmatch(self):
> +            return self._narrowmatch

I agree with the inline todo :)

> narrowrepo.py:89-91
> +        def setnarrowpats(self, newincludes, newexcludes):
> +            narrowspec.save(self, newincludes, newexcludes)
> +            self.invalidate(clearfilecache=True)

See comment in `narrowspec.py` about needing to hold the `wlock` somewhere.

> narrowrepo.py:93-96
> +        # I'm not sure this is the right place to do this filter.
> +        # context._manifestmatches() would probably be better, or perhaps
> +        # move it to a later place, in case some of the callers do want to know
> +        # which directories changed. This seems to work for now, though.

I agree with the comment. I'm not an expert on dirstate/status, but this does seem like the wrong place. I'm pretty sure we have code paths that call lower-level `status()` functions. I would expect this to live in a similar place to where sparse lives.

I think we can overlook this for the initial landing. This feels like a bit of work to resolve and can be deferred to the stabilization period.

> narrowrevlog.py:1
> +# narrowrevlog.py - revlog storing irrelevant nodes as "ellipsis" nodes
> +#

I'd like to see this file's content moved into core sooner rather than later. There are a lot of implications for storage that need to be in people's minds when they are touching code in core.

> narrowrevlog.py:31
> +
> +if util.safehasattr(revlog, 'addflagprocessor'):
> +    revlog.addflagprocessor(ELLIPSIS_NODE_FLAG,

This condition should always be true.

> narrowrevlog.py:40
> +
> +class excludeddir(manifest.treemanifest):
> +    def __init__(self, dir, node):

Please add docstring.

> narrowrevlog.py:57
> +
> +class excludeddirmanifestctx(manifest.treemanifestctx):
> +    def __init__(self, dir, node):

docstring

> narrowrevlog.py:66-67
> +    def write(self, *args):
> +        raise AssertionError('Attempt to write manifest from excluded dir %s' %
> +                             self._dir)
> +

I think we prefer `error.ProgrammingError` these days.

> narrowrevlog.py:69
> +
> +class excludedmanifestrevlog(manifest.manifestrevlog):
> +    def __init__(self, dir):

docstring

> narrowrevlog.py:74-87
> +        raise AssertionError('Attempt to get length of excluded dir %s' %
> +                             self._dir)
> +
> +    def rev(self, node):
> +        raise AssertionError('Attempt to get rev from excluded dir %s' %
> +                             self._dir)
> +

`error.ProgrammingError`

> narrowrevlog.py:90-94
> +        # We should never write entries in dirlogs outside the narrow clone.
> +        # However, the method still gets called from writesubtree() in
> +        # _addtree(), so we need to handle it. We should possibly make that
> +        # avoid calling add() with a clean manifest (_dirty is always False
> +        # in excludeddir instances).

Consider doing that and changing this to raise if called.

> narrowrevlog.py:105
> +        # child directories when using treemanifests.
> +        def dirlog(self, dir):
> +            if dir and not dir.endswith('/'):

Nit: `dir` is a builtin. If this matches core, fine. But I'd prefer avoiding the name collision.

> narrowrevlog.py:114-115
> +
> +    mfrevlog.__class__ = narrowmanifestrevlog
> +    mfrevlog._narrowed = True
> +

In-place mutation of low-level types. Yummy.

Please add a todo for the post-landing list to construct the proper type from the beginning. This likely requires some API changes in core. I'm thinking some function should return the type to use for new revlogs. Or we should spawn this type and call super.__init__ from its __init__.

> narrowrevlog.py:128-131
> +            m = super(narrowfilelog, self).renamed(node)
> +            if m and not narrowmatch(m[0]):
> +                return None
> +            return m

I think this wants a comment explaining why we lie about rename metadata when the destination is outside of the narrow spec. Also, we'll want to flag this for further review, since there could be some interesting implications to lying here.

> narrowrevlog.py:134-139
> +            # We take advantage of the fact that remotefilelog
> +            # lacks a node() method to just skip the
> +            # rename-checking logic when on remotefilelog. This
> +            # might be incorrect on other non-revlog-based storage
> +            # engines, but for now this seems to be fine.
> +            if util.safehasattr(self, 'node'):

This is making assumptions about code that hasn't landed yet. Not sure if we should replace this with a TODO or what.

> narrowspec.py:146-150
> +def save(repo, includepats, excludepats):
> +    spec = format(includepats, excludepats)
> +    if repo.shared():
> +        repo = share._getsrcrepo(repo)
> +    repo.vfs.write(FILENAME, spec)

This is called from `narrowrepo.setnarrowparts()` and neither of them cares about locking. Somewhere we should ensure we hold the wlock.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1974

To: durin42, #hg-reviewers
Cc: martinvonz, indygreg, mercurial-devel