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

Reply via email to