Patchwork D1753: streamclone: preserve remote phases (issue5648)

login
register
mail settings
Submitter phabricator
Date Dec. 24, 2017, 6:07 p.m.
Message ID <differential-rev-PHID-DREV-i7zgbjz75l4yarzyamqe-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26427/
State New
Headers show

Comments

phabricator - Dec. 24, 2017, 6:07 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  As the tests added by the previous changeset demonstrate, stream clones
  were never taught about the existence of phases. As a result, all
  changesets applied via stream clone were made public, even if they
  were draft (or even secret) on the remote.
  
  This commit integrates phases knowledge into stream clones.
  
  As the inline comment explains, because there are scenarios where
  we can't disambiguate phases support, the resulting phase data may
  not match the server exactly. However, I believe the remaining
  areas of buggy behavior are confined to secret changesets. Secret
  changesets aren't common. And transferring them via stream clones
  either requires a buggy Mercurial server or a specially-configured
  server. I think it will require stream clone support in bundle2
  before we can properly handle secret phases for stream clones.
  
  It /might/ be acceptable to force all local changesets to secret
  and then promote to draft or public from remote phases data. However,
  I was having trouble implementing this. Existing phases code doesn't
  seem geared towards supporting this scenario and I didn't want to
  incur a lot of extra work to refactor the phases code. Perfect is
  the enemy of good: I'm inclined to tackle secret phase as part of
  supporting stream clones in bundle2.
  
  This change introduces a `roots(all())` revset, which will require
  a linear scan of the changelog during clone bundles. On a Firefox
  repo, this may slow down stream clones by ~1s. Again, support for
  stream clones in bundle2 should help here.
  
  .. fix::
  
    Draft changeset phase is now preserved during a stream clone.
    
    Before, all changesets pulled via a stream clone would have a
    public phase, even if they were draft on the remote and the remote
    was non-publishing.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/streamclone.py
  tests/test-clone-uncompressed.t
  tests/test-http-bundle1.t
  tests/test-http-proxy.t
  tests/test-http.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 26, 2017, 1:05 p.m.
yuja requested changes to this revision.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> streamclone.py:191
> +        # to draft. Then we apply the remote phases data.
> +        if remotephases:
> +            roots = [ctx.node() for ctx in repo.set('roots(all())')]

Shouldn't we test `remotephases.get('publishing', False)` for
mixed-phase + publishing server?

> streamclone.py:200
> +            publicheads, draftroots = phases.analyzeremotephases(
> +                repo, repo.heads(), {hex(n): phases.draft for n in roots})
> +

It seems odd that `remotephases` data aren't used at all, but
I'm not sure.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - Dec. 27, 2017, 6:34 p.m.
indygreg added inline comments.

INLINE COMMENTS

> yuja wrote in streamclone.py:191
> Shouldn't we test `remotephases.get('publishing', False)` for
> mixed-phase + publishing server?

Maybe.

My reasoning here was that if the remote advertises phases data, then it is phases aware and the code below will ensure phases are adjusted appropriately. We certainly could special case the mixed phase + publishing case and skip all phase adjustment.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - Dec. 28, 2017, 1:14 p.m.
yuja added inline comments.

INLINE COMMENTS

> indygreg wrote in streamclone.py:191
> Maybe.
> 
> My reasoning here was that if the remote advertises phases data, then it is phases aware and the code below will ensure phases are adjusted appropriately. We certainly could special case the mixed phase + publishing case and skip all phase adjustment.

Perhaps the remote sends back actual phases data even if it is a publishing server.
That's probably why `exchange._pullapplyphases()` checks 'publishing' flag.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, yuja
Cc: yuja, mercurial-devel

Patch

diff --git a/tests/test-http.t b/tests/test-http.t
--- a/tests/test-http.t
+++ b/tests/test-http.t
@@ -282,8 +282,9 @@ 
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=namespaces x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=capabilities HTTP/1.1" 200 -
+  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=phases x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
+  "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=phases x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=branchmap HTTP/1.1" 200 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
-  "GET /?cmd=stream_out HTTP/1.1" 401 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=stream_out HTTP/1.1" 200 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=batch HTTP/1.1" 200 - x-hgarg-1:cmds=heads+%3Bknown+nodes%3D5fed3813f7f5e1824344fdc9cf8f63bb662c292d x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=getbundle HTTP/1.1" 200 - x-hgarg-1:bookmarks=1&$USUAL_BUNDLE_CAPS$&cg=0&common=5fed3813f7f5e1824344fdc9cf8f63bb662c292d&heads=5fed3813f7f5e1824344fdc9cf8f63bb662c292d&listkeys=bookmarks&phases=1 x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
diff --git a/tests/test-http-proxy.t b/tests/test-http-proxy.t
--- a/tests/test-http-proxy.t
+++ b/tests/test-http-proxy.t
@@ -107,6 +107,7 @@ 
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ cat proxy.log
   * - - [*] "GET http://localhost:$HGPORT/?cmd=capabilities HTTP/1.1" - - (glob)
+  $LOCALIP - - [$LOGDATE$] "GET http://localhost:$HGPORT/?cmd=listkeys HTTP/1.1" - - x-hgarg-1:namespace=phases x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   $LOCALIP - - [$LOGDATE$] "GET http://localhost:$HGPORT/?cmd=branchmap HTTP/1.1" - - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ (glob)
   $LOCALIP - - [$LOGDATE$] "GET http://localhost:$HGPORT/?cmd=stream_out HTTP/1.1" - - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ (glob)
   $LOCALIP - - [$LOGDATE$] "GET http://localhost:$HGPORT/?cmd=batch HTTP/1.1" - - x-hgarg-1:cmds=heads+%3Bknown+nodes%3D83180e7845de420a1bb46896fd5fe05294f8d629 x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ (glob)
diff --git a/tests/test-http-bundle1.t b/tests/test-http-bundle1.t
--- a/tests/test-http-bundle1.t
+++ b/tests/test-http-bundle1.t
@@ -291,8 +291,9 @@ 
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=namespaces x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=capabilities HTTP/1.1" 200 -
+  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=phases x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
+  "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=phases x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=branchmap HTTP/1.1" 200 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
-  "GET /?cmd=stream_out HTTP/1.1" 401 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=stream_out HTTP/1.1" 200 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
   "GET /?cmd=batch HTTP/1.1" 200 - x-hgarg-1:cmds=heads+%3Bknown+nodes%3D5fed3813f7f5e1824344fdc9cf8f63bb662c292d x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$
diff --git a/tests/test-clone-uncompressed.t b/tests/test-clone-uncompressed.t
--- a/tests/test-clone-uncompressed.t
+++ b/tests/test-clone-uncompressed.t
@@ -39,6 +39,9 @@ 
   $ hg --debug --config worker.backgroundclose=true --config worker.backgroundcloseminfilecount=1 clone --stream -U http://localhost:$HGPORT clone-background | grep -v adding
   using http://localhost:$HGPORT/
   sending capabilities command
+  preparing listkeys for "phases"
+  sending listkeys command
+  received listkey for "phases": 58 bytes
   sending branchmap command
   streaming all changes
   sending stream_out command
@@ -94,11 +97,9 @@ 
   searching for changes
   no changes found
 
-TODO this is buggy
-
   $ hg -R all-draft log -T '{rev} {phase}\n'
-  1 public
-  0 public
+  1 draft
+  0 draft
 
 Mixed phase is preserved
 
@@ -110,10 +111,8 @@ 
   searching for changes
   no changes found
 
-TODO this is buggy
-
   $ hg -R mixed-draft log -T '{rev} {phase}\n'
-  1 public
+  1 draft
   0 public
 
 Cannot stream clone when there are secret changesets
diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py
--- a/mercurial/streamclone.py
+++ b/mercurial/streamclone.py
@@ -10,6 +10,9 @@ 
 import struct
 
 from .i18n import _
+from .node import (
+    hex,
+)
 from . import (
     branchmap,
     error,
@@ -118,6 +121,8 @@ 
     repo = pullop.repo
     remote = pullop.remote
 
+    remotephases = pullop.remote.listkeys('phases')
+
     # Save remote branchmap. We will use it later to speed up branchcache
     # creation.
     rbranchmap = None
@@ -158,6 +163,45 @@ 
         repo._applyopenerreqs()
         repo._writerequirements()
 
+        # Application of phases data is subtly non-trivial.
+        #
+        # The following have to be considered:
+        #
+        # * The remote could be running an old version of Mercurial that
+        #   isn't phases aware. Remote phases data will be empty. (This should
+        #   not be common since phases were supported since Mercurial 1.9.)
+        # * Some versions of Mercurial (before issue 5589 was fixed) can
+        #   serve secret changesets. Versions after can be configured to serve
+        #   secret changesets.
+        # * The remote could be publishing or non-publishing.
+        # * If local changesets are secret, this impacts discovery and other
+        #   mechanisms that occur during the "pull" that is performed after
+        #   this function returns.
+        #
+        # This combination of scenarios means that we can't 100% reliably
+        # reproduce the remote phases state. For example, if the remote is
+        # configured to serve secret changesets and all changesets are secret,
+        # there won't be any phases data and we won't be able to distinguish
+        # between that repo and a Mercurial that isn't phases aware.
+        #
+        # Our strategy is to look at the remote phases data. If there isn't any,
+        # we assume we are talking to an old server that doesn't support phases.
+        # We keep all changesets as public. Otherwise, we force all changesets
+        # to draft. Then we apply the remote phases data.
+        if remotephases:
+            roots = [ctx.node() for ctx in repo.set('roots(all())')]
+            tr = pullop.gettransaction()
+            phases.retractboundary(repo, tr, phases.draft, roots)
+
+            # Strictly speaking, we may not need to apply remote phases here,
+            # as the incremental pull after this function may take care of it.
+            # But since we already have the data, we might as well use it.
+            publicheads, draftroots = phases.analyzeremotephases(
+                repo, repo.heads(), {hex(n): phases.draft for n in roots})
+
+            if publicheads:
+                phases.advanceboundary(repo, tr, phases.public, publicheads)
+
         if rbranchmap:
             branchmap.replacecache(repo, rbranchmap)