D1974: narrow: import experimental extension from narrowhg revision cb51d673e9c5

[indygreg (Gregory Szorc)]

indygreg added a comment.


  Another batch before I head off to a meeting...

INLINE COMMENTS

> narrowcopies.py:1
> +# narrowcopies.py - extensions to mercurial copies module to support narrow
> +# clones

Copy code is complex and has performance concerns. I'd like to see this moved into core sooner rather than later. Perhaps a good first step would be to add the `narrowmatch` method to `localrepository`? Or something on `localrepository` in core so we can detect presence of narrow in a one-liner.

> narrowdirstate.py:1
> +# narrowdirstate.py - extensions to mercurial dirstate to support narrow clones
> +#

I'm not an expert on dirstate. But I know enough to know that "here be dragons." I'd like to see everything dirstate moved into core ASAP so it can be better integrated with sparse, etc. People hacking on this code in core need to be aware of the implications for narrow.

> narrowmerge.py:1
> +# narrowmerge.py - extensions to mercurial merge module to support narrow clones
> +#

More code that should be on the fast track to core.

> narrowmerge.py:25
> +
> +        if not util.safehasattr(repo, 'narrowmatch'):
> +            return actions, diverge, renamedelete

The more I see this pattern, the more I think there needs to be an attribute on `localrepository` that can be used for quickly testing if narrow is in play. Actually, should we assume all repos are narrow (with the *all matcher*) and fast path the special case of the default?

> narrowspec.py:26
> +def _parsestoredpatterns(text):
> +    """Parses the narrowspec format that's stored on disk."""
> +    patlist = None

The format needs documentation somewhere inline.

> narrowspec.py:31-42
> +        if l == '[includes]':
> +            if patlist is None:
> +                patlist = includepats
> +            else:
> +                raise error.Abort(_('narrowspec includes section must appear '
> +                                    'at most once, before excludes'))
> +        elif l == '[excludes]':

Oh, hey, this looks just like sparse profiles! I sense some code conversion in our future...

> narrowspec.py:49-54
> +    """Parses the narrowspec format that's returned by the server."""
> +    includepats = set()
> +    excludepats = set()
> +
> +    # We get one entry per line, in the format "<key> <value>".
> +    # It's OK for value to contain other spaces.

Playing devil's advocate, do we really need two formats doing the same thing?

> narrowspec.py:83
> +    # it by adding a character at the end.
> +    return len((s + 'x').splitlines())
> +

Can we use `str.count()` to avoid creating objects for each line?

> narrowspec.py:93-95
> +    components = pat.split('/')
> +    if '.' in components or '..' in components:
> +        raise error.Abort(_('"." and ".." are not allowed in narrowspec paths'))

What about `\` as a path separator? Should we also ban that? I assume we're interpreting the value here and paths as bytes, so `\` will never be used as an escape character?

> narrowspec.py:110-115
> +    output = '[includes]\n'
> +    for i in sorted(includes - excludes):
> +        output += i + '\n'
> +    output += '[excludes]\n'
> +    for e in sorted(excludes):
> +        output += e + '\n'

It is better to build a list of lines and `'\n'.join()` them.

> narrowspec.py:133-138
> +    try:
> +        spec = repo.vfs.read(FILENAME)
> +    except IOError as e:
> +        # Treat "narrowspec does not exist" the same as "narrowspec file exists
> +        # and is empty".
> +        if e.errno == errno.ENOENT:

This reinvents `repo.vfs.tryread()`.

> narrowspec.py:154-162
> +    r""" Restricts the patterns according to repo settings,
> +    results in a logical AND operation
> +
> +    :param req_includes: requested includes
> +    :param req_excludes: requested excludes
> +    :param repo_includes: repo includes
> +    :param repo_excludes: repo excludes

The purpose of this function was not clear on initial reading. Could you please elaborate a bit more in the docstring?

> narrowspec.py:179
> +    """
> +    res_excludes = req_excludes.copy()
> +    res_excludes.update(repo_excludes)

We use `set(repo_includes)` below to copy a set. Let's make things consistent.

> narrowspec.py:185-192
> +        for req_include in req_includes:
> +            req_include = util.expandpath(util.normpath(req_include))
> +            if req_include in repo_includes:
> +                res_includes.append(req_include)
> +                continue
> +            valid = False
> +            for repo_include in repo_includes:

I feel like there's a more efficient mechanism lurking here. But I'm not sure if we care about the performance implications of this function.

> narrowspec.py:197
> +            if not valid and invalid_includes is not None:
> +                invalid_includes.append(req_include)
> +        if len(res_includes) == 0:

Mutating an argument feels a bit wonky. Should this be returned instead?

> narrowwirepeer.py:27
> +                ui.status(_("expanding narrowspec\n"))
> +                if not self.capable('expandnarrow'):
> +                    raise error.Abort(

There is no server-side implementation of this capability/command yet. We may want to remove the reference from core.

Also, the wire protocol capability should have `exp` or `experimental` in it until we formalize the API.

REPOSITORY
  rHG Mercurial

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

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