| Submitter | phabricator |
|---|---|
| Date | May 22, 2019, 9:48 p.m. |
| Message ID | <differential-rev-PHID-DREV-4rvnbmxqmctedwor2csh-req@phab.mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/40200/ |
| State | Superseded |
| Headers | show |
Comments
martinvonz added a comment. This is going to break our server. Can you make it work with existing servers for a while (behave differently depending on capability, I suppose), so we can get a chance to transition? I would think it's not going to be terribly hard, but let me know if it would take more time than you have and I'll try to help. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6436 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: mercurial-devel
pulkit added a comment.
In https://phab.mercurial-scm.org/D6436#93598, @martinvonz wrote:
> This is going to break our server. Can you make it work with existing servers for a while (behave differently depending on capability, I suppose), so we can get a chance to transition? I would think it's not going to be terribly hard, but let me know if it would take more time than you have and I'll try to help.
Sure, will send an updated version soon.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D6436
To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
martinvonz added inline comments. INLINE COMMENTS > narrowcommands.py:149 > # we have all the nodes > if wireprototypes.ELLIPSESCAP in pullop.remote.capabilities(): > kwargs['known'] = [node.hex(ctx.node()) for ctx in Also need to check old capability here > narrowcommands.py:257-258 > + remotecap = remote.capabilities() > + ellipsesremote = any(True for cap in wireprototypes.SUPPORTED_ELLIPSESCAP > + if cap in remotecap) > + nit: `any(cap in remotecap for cap in wireprototypes.SUPPORTED_ELLIPSESCAP)` > narrowcommands.py:282 > with wrappedextraprepare: > - with repo.ui.configoverride(overrides, 'widen'): > exchange.pull(repo, remote, heads=common) Should keep this in the "old" case > narrowcommands.py:303-305 > + 'commonheads': common, > + 'known': known, > + 'ellipses': True, Is the `known` set needed when not using ellipses? Conversely, `commonheads` shouldn't be needed when using ellipses, but perhaps it's still useful to have it there (it's usually way smaller, so it's much less of a concern). These things can be fixed in a separate patch, of course. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6436 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: mercurial-devel
rdamazio added inline comments. INLINE COMMENTS > narrowcommands.py:306 > + 'ellipses': True, > + }).result() > + trmanager = exchange.transactionmanager(repo, 'widen', Being nosy here: does this mean that it'll download the whole bundle into memory before it starts applying it, rather than streaming it into the store? Bundles, especially in repos using narrow, can be very large, so that would not be ideal. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6436 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: rdamazio, mercurial-devel
pulkit added inline comments. INLINE COMMENTS > martinvonz wrote in narrowcommands.py:149 > Also need to check old capability here Only need to check the old capability here. Will update. > martinvonz wrote in narrowcommands.py:282 > Should keep this in the "old" case We don't need this in old cases. This is only required when widening non-ellipses cases as that will end us in a situation with an empty changegroup. > martinvonz wrote in narrowcommands.py:303-305 > Is the `known` set needed when not using ellipses? Conversely, `commonheads` shouldn't be needed when using ellipses, but perhaps it's still useful to have it there (it's usually way smaller, so it's much less of a concern). These things can be fixed in a separate patch, of course. No, the known set is not needed in non-ellipses cases. We should not compute that also as in non-ellipses repos, computing known just adding all local commits which can be in millions. The server logic completely rely on common in non-ellipses cases. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6436 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: rdamazio, mercurial-devel
pulkit added a subscriber: marmoute. pulkit added inline comments. INLINE COMMENTS > rdamazio wrote in narrowcommands.py:306 > Being nosy here: does this mean that it'll download the whole bundle into memory before it starts applying it, rather than streaming it into the store? Bundles, especially in repos using narrow, can be very large, so that would not be ideal. I believe it streams rather than downloading the whole bundle into memory. `getbundle()` uses the same function and I think we do streaming there. https://www.mercurial-scm.org/repo/hg-committed/file/127937874395/mercurial/exchange.py#l1757 I also asked @marmoute and he also think that it does streaming. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6436 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: marmoute, rdamazio, mercurial-devel
rdamazio added a comment. Hopefully not too late INLINE COMMENTS > pulkit wrote in narrowcommands.py:306 > I believe it streams rather than downloading the whole bundle into memory. `getbundle()` uses the same function and I think we do streaming there. https://www.mercurial-scm.org/repo/hg-committed/file/127937874395/mercurial/exchange.py#l1757 > > I also asked @marmoute and he also think that it does streaming. The difference from the code you're pointing too is that you're calling .result() here, which blocks until the results are fully streamed in (see httppeer.py) - so I really think this is NOT streaming. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6436 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: marmoute, rdamazio, mercurial-devel
pulkit added inline comments. INLINE COMMENTS > rdamazio wrote in narrowcommands.py:306 > The difference from the code you're pointing too is that you're calling .result() here, which blocks until the results are fully streamed in (see httppeer.py) - so I really think this is NOT streaming. 1. I see a `.result()` there too https://www.mercurial-scm.org/repo/hg-committed/file/127937874395/mercurial/exchange.py#l1751 2. I spent some time understanding whether this is streaming or not, it looks like it is not. I will try to understand more the related code path and update about my findings. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6436 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: marmoute, rdamazio, mercurial-devel
Patch
diff --git a/tests/test-narrow.t b/tests/test-narrow.t --- a/tests/test-narrow.t +++ b/tests/test-narrow.t @@ -286,13 +286,11 @@ $ hg tracked --addinclude d0 comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow-empty/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 3 changesets with 1 changes to 1 files - new changesets *:* (glob) $ hg tracked I path:d0 $ hg files diff --git a/tests/test-narrow-widen.t b/tests/test-narrow-widen.t --- a/tests/test-narrow-widen.t +++ b/tests/test-narrow-widen.t @@ -95,13 +95,11 @@ $ hg tracked --addinclude widest/f comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 3 changesets with 2 changes to 2 files - new changesets *:* (glob) $ hg tracked I path:inside I path:widest/f @@ -154,13 +152,11 @@ $ hg tracked --addinclude wider comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 8 changesets with 7 changes to 3 files - new changesets *:* (glob) $ hg tracked I path:inside I path:wider @@ -261,13 +257,11 @@ $ hg tracked --addinclude d1 comparing with ssh://user@dummy/upstream searching for changes - no changes found saved backup bundle to $TESTTMP/narrow2/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 9 changesets with 5 changes to 5 files - new changesets *:* (glob) $ hg tracked I path:d0 I path:d1 @@ -342,7 +336,6 @@ $ hg --config hooks.pretxnchangegroup.bad=false tracked --addinclude d1 comparing with ssh://user@dummy/upstream searching for changes - no changes found saved backup bundle to $TESTTMP/interrupted/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests diff --git a/tests/test-narrow-trackedcmd.t b/tests/test-narrow-trackedcmd.t --- a/tests/test-narrow-trackedcmd.t +++ b/tests/test-narrow-trackedcmd.t @@ -145,13 +145,11 @@ looking for local changes to affected paths deleting data/inside/f.i deleting meta/inside/00manifest.i (tree !) - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 2 changesets with 0 changes to 0 files - new changesets *:* (glob) $ hg tracked I path:outisde X path:inside @@ -166,13 +164,11 @@ $ hg tracked --import-rules specs --addinclude 'wider/' comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 3 changesets with 1 changes to 1 files - new changesets *:* (glob) $ hg tracked I path:outisde I path:wider @@ -211,13 +207,11 @@ $ hg tracked --import-rules ../nspecs comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 3 changesets with 0 changes to 0 files - new changesets *:* (glob) $ cd .. diff --git a/tests/test-narrow-patterns.t b/tests/test-narrow-patterns.t --- a/tests/test-narrow-patterns.t +++ b/tests/test-narrow-patterns.t @@ -135,13 +135,11 @@ $ hg tracked --removeexclude dir1/dirA comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 9 changesets with 6 changes to 6 files - new changesets *:* (glob) $ hg tracked I path:dir1 I path:dir2 @@ -195,13 +193,11 @@ deleting data/dir1/dirA/bar.i (reporevlogstore !) deleting data/dir1/dirA/bar/0eca1d0cbdaea4651d1d04d71976a6d2d9bfaae5 (reposimplestore !) deleting data/dir1/dirA/bar/index (reposimplestore !) - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 11 changesets with 7 changes to 7 files - new changesets *:* (glob) $ hg tracked I path:dir1 I path:dir2 @@ -253,13 +249,11 @@ deleting data/dir1/dirA/foo.i (reporevlogstore !) deleting data/dir1/dirA/foo/162caeb3d55dceb1fee793aa631ac8c73fcb8b5e (reposimplestore !) deleting data/dir1/dirA/foo/index (reposimplestore !) - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 13 changesets with 8 changes to 8 files - new changesets *:* (glob) $ hg tracked I path:dir1 I path:dir2 @@ -312,13 +306,11 @@ $ hg tracked --removeexclude dir1/dirA comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 13 changesets with 9 changes to 9 files - new changesets *:* (glob) $ hg tracked I path:dir1 I path:dir2 @@ -389,13 +381,11 @@ $ hg tracked --addinclude dir1 comparing with ssh://user@dummy/master searching for changes - no changes found saved backup bundle to $TESTTMP/narrow2/.hg/strip-backup/*-widen.hg (glob) adding changesets adding manifests adding file changes added 10 changesets with 6 changes to 6 files - new changesets *:* (glob) $ find * | sort dir1 dir1/bar diff --git a/tests/test-narrow-clone-non-narrow-server.t b/tests/test-narrow-clone-non-narrow-server.t --- a/tests/test-narrow-clone-non-narrow-server.t +++ b/tests/test-narrow-clone-non-narrow-server.t @@ -32,7 +32,7 @@ > EOF $ echo hello | hg -R . serve --stdio | \ > "$PYTHON" unquote.py | tr ' ' '\n' | grep narrow - exp-narrow-1 + exp-narrow-2 $ cd .. diff --git a/mercurial/wireprototypes.py b/mercurial/wireprototypes.py --- a/mercurial/wireprototypes.py +++ b/mercurial/wireprototypes.py @@ -29,8 +29,8 @@ SSHV2 = 'exp-ssh-v2-0003' HTTP_WIREPROTO_V2 = 'exp-http-v2-0003' -NARROWCAP = 'exp-narrow-1' -ELLIPSESCAP = 'exp-ellipses-1' +NARROWCAP = 'exp-narrow-2' +ELLIPSESCAP = 'exp-ellipses-2' # All available wire protocol transports. TRANSPORTS = { diff --git a/hgext/narrow/narrowwirepeer.py b/hgext/narrow/narrowwirepeer.py --- a/hgext/narrow/narrowwirepeer.py +++ b/hgext/narrow/narrowwirepeer.py @@ -19,6 +19,8 @@ wireprotov1server, ) +from . import narrowbundle2 + def uisetup(): wireprotov1peer.wirepeer.narrow_widen = peernarrowwiden @@ -69,21 +71,25 @@ narrowspec.validatepatterns(set(newexcludes)) common = wireprototypes.decodelist(commonheads) - known = None - if known: - known = wireprototypes.decodelist(known) + known = wireprototypes.decodelist(known) if ellipses == '0': ellipses = False else: ellipses = bool(ellipses) cgversion = cgversion - newmatch = narrowspec.match(repo.root, include=newincludes, - exclude=newexcludes) - oldmatch = narrowspec.match(repo.root, include=oldincludes, - exclude=oldexcludes) - bundler = bundle2.widen_bundle(repo, oldmatch, newmatch, common, known, - cgversion, ellipses) + if not ellipses: + newmatch = narrowspec.match(repo.root, include=newincludes, + exclude=newexcludes) + oldmatch = narrowspec.match(repo.root, include=oldincludes, + exclude=oldexcludes) + bundler = bundle2.widen_bundle(repo, oldmatch, newmatch, common, + known, cgversion, ellipses) + else: + bundler = bundle2.bundle20(repo.ui) + narrowbundle2.generateellipsesbundle2(bundler, repo, oldincludes, + oldexcludes, newincludes, newexcludes, cgversion, common, + known, None, list(common)) except error.Abort as exc: bundler = bundle2.bundle20(repo.ui) manargs = [('message', pycompat.bytestr(exc))] diff --git a/hgext/narrow/narrowcommands.py b/hgext/narrow/narrowcommands.py --- a/hgext/narrow/narrowcommands.py +++ b/hgext/narrow/narrowcommands.py @@ -279,8 +279,27 @@ with ds.parentchange(): ds.setparents(node.nullid, node.nullid) with wrappedextraprepare: - with repo.ui.configoverride(overrides, 'widen'): - exchange.pull(repo, remote, heads=common) + known = [node.hex(ctx.node()) for ctx in + repo.set('::%ln', common) + if ctx.node() != node.nullid] + + with remote.commandexecutor() as e: + bundle = e.callcommand('narrow_widen', { + 'oldincludes': oldincludes, + 'oldexcludes': oldexcludes, + 'newincludes': newincludes, + 'newexcludes': newexcludes, + 'cgversion': '03', + 'commonheads': common, + 'known': known, + 'ellipses': True, + }).result() + trmanager = exchange.transactionmanager(repo, 'widen', remote.url()) + with trmanager: + op = bundle2.bundleoperation(repo, + trmanager.transaction, source='widen') + bundle2.processbundle(repo, bundle, op=op) + with ds.parentchange(): ds.setparents(p1, p2) else: