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