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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to