Patchwork D6436: narrow: use narrow_widen wireproto command to widen in case of ellipses

login
register
mail settings
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

phabricator - May 22, 2019, 9:48 p.m.
pulkit created this revision.
Herald added a reviewer: durin42.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Few releases ago, we introduce narrow_widen wireproto command to be used to widen
  narrow repositories. Before this patch, that was used in non-ellipses cases
  only. In ellipses cases, we still do exchange.pull() which can pull more data
  than required.
  
  This patch makes ellipses case also use narrow_widen command. I updated both the
  NARROW_CAP and ELLIPSES_CAP wireprotocol capabilities.
  
  Due to change to wireproto command, the code looks a bit dirty, next patches
  will clean that up.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/narrow/narrowcommands.py
  hgext/narrow/narrowwirepeer.py
  mercurial/wireprototypes.py
  tests/test-narrow-clone-non-narrow-server.t
  tests/test-narrow-patterns.t
  tests/test-narrow-trackedcmd.t
  tests/test-narrow-widen.t
  tests/test-narrow.t

CHANGE DETAILS




To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - May 23, 2019, 4:45 p.m.
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
phabricator - May 24, 2019, 1:07 p.m.
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
phabricator - June 3, 2019, 11:47 p.m.
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
phabricator - June 3, 2019, 11:51 p.m.
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
phabricator - June 4, 2019, 3:12 p.m.
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
phabricator - June 4, 2019, 3:22 p.m.
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
phabricator - June 4, 2019, 11:27 p.m.
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
phabricator - June 5, 2019, 7:05 p.m.
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: