D1974: narrow: import experimental extension from narrowhg revision cb51d673e9c5

[indygreg (Gregory Szorc)]

indygreg added a comment.


  I'm still only partly through the changegroup and bundle code. Haven't looked at commands or tests yet. I think I'm going to take a break for a bit because this is a lot of code to absorb!

INLINE COMMENTS

> narrowbundle2.py:37
> +
> +narrowcap = 'narrow'
> +narrowacl_section = 'narrowhgacl'

This wants an `exp` or `experimental` in its name. Once we start advertising it, there's no going back. So we shouldn't squat on final names until we're sure.

> narrowbundle2.py:126
> +    relevant_nodes = set()
> +    visitnodes = map(cl.node, missing)
> +    required = set(headsrevs) | known

`map()` returns an iterator on Python 3.

> narrowbundle2.py:140-142
> +                # We choose to not trust the changed files list in
> +                # changesets because it's not always correct. TODO: could
> +                # we trust it for the non-merge case?

No, we can't. There were bugs in e.g. rebase that caused changed files to be dropped from the files list in the changeset. Walking manifests is the only way to be sure you are getting all changes.

> narrowbundle2.py:260
> +        if include or exclude:
> +            narrowspecpart = bundler.newpart(specpart)
> +            if include:

Would it make sense to send the narrow spec before the changegroup part?

> narrowbundle2.py:262-266
> +                narrowspecpart.addparam(
> +                    specpart_include, '\n'.join(include), mandatory=True)
> +            if exclude:
> +                narrowspecpart.addparam(
> +                    specpart_exclude, '\n'.join(exclude), mandatory=True)

Part parameters are effectively header data for bundle2 parts. As such, they are read and written as atomic units. And they are URL quoted for transport safety.

Since narrow specs are unbound in size, I think they should be transferred as part of the part payload, not in parameters. This allows for streaming and doesn't add overhead for encoding/decoding their content.

This is obviously a BC change to the wire protocol and bundle storage.

> narrowchangegroup.py:47
> +
> +    extensions.wrapfunction(changegroup.cg1packer, 'prune', prune)
> +

Might want a comment here that the wrapped method will get inherited by later versions of the changegroup packer. It feels weird to see this definition on `cg1packer` since code above removes support for changegroup versions `01` and `02`.

> narrowchangegroup.py:60-81
> +            # In a shallow clone, the linknodes callback needs to also include
> +            # those file nodes that are in the manifests we sent but weren't
> +            # introduced by those manifests.
> +            commonctxs = [self._repo[c] for c in commonrevs]
> +            oldlinknodes = linknodes
> +            clrev = self._repo.changelog.rev
> +            def linknodes(flog, fname):

I don't fully understand linknodes. This should be scrutinized by someone who does, especially since there are likely performance concerns here.

> narrowchangegroup.py:102-105
> +        getattr(self, 'clrev_to_localrev', {}).clear()
> +        if getattr(self, 'next_clrev_to_localrev', {}):
> +            self.clrev_to_localrev = self.next_clrev_to_localrev
> +            del self.next_clrev_to_localrev

I'm not a fan of `getattr()` and `del` here. Is there no way to always set these attributes on instance creation?

(One solution is to move this code into `changegroup.py`.)

> narrowwirepeer.py:45-48
> +            if cmd == 'unbundle':
> +                include, exclude = repo.narrowpats
> +                kwargs["includepats"] = ','.join(include)
> +                kwargs["excludepats"] = ','.join(exclude)

This will blindly add `includepats` and `excludepats` as wire protocol arguments to the `unbundle` command. In the Python server, wire protocol arguments have to be defined as part of the command handler or the server will refuse to serve the request.

So, this code is missing the server-side declaration of these arguments.

And, the client/peer code here is buggy for not conditionally adding these arguments based on the presence of a server capability advertising support for receiving these arguments.

REPOSITORY
  rHG Mercurial

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

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