D4813: narrow: the first version of narrow_widen wireprotocol command

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  > I am not sure whether we need changegroup version as an argument to the command
  >  as I *think* narrow needs changegroup3 already.
  
  Ellipses need changegroup3 (for the revision flag) and treemanifests also do (for the extra room for directory manifests), but narrow itself shouldn't need it. We can probably still assume it's cg3 because there's probably no reason to use an older version for a new command (and we can add the extra parameter later if we need to support a future cg4).

INLINE COMMENTS

> narrowcommands.py:293-295
> -            with wrappedextraprepare,\
> -                 repo.ui.configoverride(overrides, 'widen'):
> -                exchange.pull(repo, remote, heads=common)

As I said on IRC, I think we (Google) would appreciate it if this could remain here for a while to give us more time to migrate our custom server to the new wire protocol command. I was thinking we could just fall back to the getbundle command if narrow_widen isn't supported.  That probably means changing the capability to "exp-narrow-2" now and call getbundle if "exp-narrow-1" is supported but "exp-narrow-2" isn't. That shouldn't be very difficult, but I don't think we've done kind of thing before.

Actually, you can ignore all that because our server has the ellipses capability, so it will not get here anyway :) We should instead think about such a migration path when we add support for ellipses to narrow_widen.

> narrowcommands.py:302
> +                    'cgversion': '03',
> +                    'common': common,
> +                    'known': known,

Should we call this something like "heads" instead? It's the set of common heads between the server and client and "common" makes sense together with "heads" in the getbundle call, but it doesn't seem to make as much sense here.

> narrowcommands.py:307-310
> +                with repo.ui.configoverride(overrides, 'widen'):
> +                    tgetter = lambda: tr
> +                    bundle2.processbundle(repo, bundle,
> +                            transactiongetter=tgetter)

It looks like this doesn't need to be in in the commandexecutor block and the transaction probably doesn't need to span the wire protocol request. IOW, I think you can drop `repo.transaction()`from line 293 and instead put that togetgher this with this with-block.

> pulkit wrote in narrowwirepeer.py:71-78
> I am not sure, @martinvonz what do you think?

I assume the reason for that would be when pulling from a narrow clone? I think we have mostly ignored that case so far. I'm fine with just not caring about it for a while more :)

> narrowwirepeer.py:49
> +
> + at wireprotov1server.wireprotocommand('narrow_widen', '*', permission='pull')
> +def narrow_widen(repo, proto, args):

Should we just put this in core from the beginning?

> narrowwirepeer.py:79-82
> +    if args.get('ellipses') == '0':
> +        ellipses = False
> +    else:
> +        ellipses = bool(args.get('ellipses'))

I suppose we want the default to be False?  Isn't `ellipses = (args.get('ellipses') == '1')` enough? Or is the idea to be flexible with what values we accept? Is that what other wire protocol commands do?

REPOSITORY
  rHG Mercurial

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

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