Patchwork [5,of,6] push: add a way to allow concurrent pushes on unrelated heads

login
register
mail settings
Submitter Pierre-Yves David
Date June 4, 2017, 2:49 p.m.
Message ID <ae88951457de93c7f628.1496587773@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/21172/
State Accepted
Headers show

Comments

Pierre-Yves David - June 4, 2017, 2:49 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1496030038 -7200
#      Mon May 29 05:53:58 2017 +0200
# Node ID ae88951457de93c7f6286d449672b0b9d20c57f1
# Parent  33062379c342992425c1ba8083bafb46420ba7d0
# EXP-Topic pushrace
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r ae88951457de
push: add a way to allow concurrent pushes on unrelated heads

Client has a mechanism for the server to check that nothing changed server side
since the client prepared a push. That check is wide and any head changed on
the server will lead to an aborted push. We introduces a way for the client to
send a less strict checking. That logic will checks that no heads impacted by
the push have been affected. If other unrelated heads (including named branches
heads) have been affected, the push will proceed.

This is very helpful for repositories with high developers traffic on different
heads, a common setup.

That behavior is currently controlled by an experimental option. The config
should live in the "server" section but bike-shedding of the name will happen
in the next changesets. Servers advertise this capability through a new bundle2
capability 'checkeads', using the value 'related'.

The 'test-push-race.t' is updated to check that new capabilities on the
documented cases.
Yuya Nishihara - June 7, 2017, 2:25 p.m.
On Sun, 04 Jun 2017 15:49:33 +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1496030038 -7200
> #      Mon May 29 05:53:58 2017 +0200
> # Node ID ae88951457de93c7f6286d449672b0b9d20c57f1
> # Parent  33062379c342992425c1ba8083bafb46420ba7d0
> # EXP-Topic pushrace
> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r ae88951457de
> push: add a way to allow concurrent pushes on unrelated heads

I don't fully understand the bundle2 caps and parts, but the change looks good.
Queued up to here, thanks.

> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -323,8 +323,21 @@ class pushoperation(object):
>          self.bkresult = None
>          # discover.outgoing object (contains common and outgoing data)
>          self.outgoing = None
> -        # all remote heads before the push
> +        # all remote topological heads before the push
>          self.remoteheads = None
> +        # Details of the remote branch pre and post push
> +        #
> +        # mapping: {'branch': ([remoteheads],
> +        #                      [newheads],
> +        #                      [unsyncedheads],
> +        #                      [discardedheads])}
> +        # - branch: the branch name
> +        # - remoteheads: the list of remote heads known locally
> +        #                None if the branch is new
> +        # - newheads: the new remote heads (known locally) with outgoing pushed
> +        # - unsyncedheads: the list of remote heads unknown locally.
> +        # - discardedheads: the list of remote heads made obsolete by the push
> +        self.pushbranchmap = None
>          # testable as a boolean indicating if any nodes are missing locally.
>          self.incoming = None
>          # phases changes that must be pushed along side the changesets
> @@ -712,8 +725,23 @@ def _pushb2ctxcheckheads(pushop, bundler
>  
>      Exists as an independent function to aid extensions
>      """
> -    if not pushop.force:
> -        bundler.newpart('check:heads', data=iter(pushop.remoteheads))
> +    # * 'force' do not check for push race,
> +    # * if we don't push anything, there are nothing to check.
> +    if not pushop.force and pushop.outgoing.missingheads:
> +        allowunrelated = 'related' in bundler.capabilities.get('checkheads', ())
> +        if not allowunrelated:
> +            bundler.newpart('check:heads', data=iter(pushop.remoteheads))
> +        else:
> +            affected = set()
> +            for branch, heads in pushop.pushbranchmap.iteritems():
> +                remoteheads, newheads, unsyncedheads, discardedheads = heads
> +                if remoteheads is not None:
> +                    remote = set(remoteheads)
> +                    affected |= set(discardedheads) & remote
> +                    affected |= remote - set(newheads)

I'm a little confused with the name "newheads" even with the comment above,
but I don't have any better name.

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1323,6 +1323,8 @@  def getrepocaps(repo, allowpushback=Fals
         caps['obsmarkers'] = supportedformat
     if allowpushback:
         caps['pushback'] = ()
+    if not repo.ui.configbool('experimental', 'checkheads-strict', True):
+        caps['checkheads'] = ('related',)
     return caps
 
 def bundle2caps(remote):
@@ -1603,6 +1605,35 @@  def handlecheckheads(op, inpart):
         raise error.PushRaced('repository changed while pushing - '
                               'please try again')
 
+@parthandler('check:updated-heads')
+def handlecheckupdatedheads(op, inpart):
+    """check for race on the heads touched by a push
+
+    This is similar to 'check:heads' but focus on the heads actually updated
+    during the push. If other activities happen on unrelated heads, it is
+    ignored.
+
+    This allow server with high traffic to avoid push contention as long as
+    unrelated parts of the graph are involved."""
+    h = inpart.read(20)
+    heads = []
+    while len(h) == 20:
+        heads.append(h)
+        h = inpart.read(20)
+    assert not h
+    # trigger a transaction so that we are guaranteed to have the lock now.
+    if op.ui.configbool('experimental', 'bundle2lazylocking'):
+        op.gettransaction()
+
+    currentheads = set()
+    for ls in op.repo.branchmap().itervalues():
+        currentheads.update(ls)
+
+    for h in heads:
+        if h not in currentheads:
+            raise error.PushRaced('repository changed while pushing - '
+                                  'please try again')
+
 @parthandler('output')
 def handleoutput(op, inpart):
     """forward output captured on the server to the client"""
diff --git a/mercurial/discovery.py b/mercurial/discovery.py
--- a/mercurial/discovery.py
+++ b/mercurial/discovery.py
@@ -332,6 +332,7 @@  def checkheads(pushop):
         headssum = _headssummary(pushop)
     else:
         headssum = _oldheadssummary(repo, remoteheads, outgoing, inc)
+    pushop.pushbranchmap = headssum
     newbranches = [branch for branch, heads in headssum.iteritems()
                    if heads[0] is None]
     # 1. Check for new branches on the remote.
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -323,8 +323,21 @@  class pushoperation(object):
         self.bkresult = None
         # discover.outgoing object (contains common and outgoing data)
         self.outgoing = None
-        # all remote heads before the push
+        # all remote topological heads before the push
         self.remoteheads = None
+        # Details of the remote branch pre and post push
+        #
+        # mapping: {'branch': ([remoteheads],
+        #                      [newheads],
+        #                      [unsyncedheads],
+        #                      [discardedheads])}
+        # - branch: the branch name
+        # - remoteheads: the list of remote heads known locally
+        #                None if the branch is new
+        # - newheads: the new remote heads (known locally) with outgoing pushed
+        # - unsyncedheads: the list of remote heads unknown locally.
+        # - discardedheads: the list of remote heads made obsolete by the push
+        self.pushbranchmap = None
         # testable as a boolean indicating if any nodes are missing locally.
         self.incoming = None
         # phases changes that must be pushed along side the changesets
@@ -712,8 +725,23 @@  def _pushb2ctxcheckheads(pushop, bundler
 
     Exists as an independent function to aid extensions
     """
-    if not pushop.force:
-        bundler.newpart('check:heads', data=iter(pushop.remoteheads))
+    # * 'force' do not check for push race,
+    # * if we don't push anything, there are nothing to check.
+    if not pushop.force and pushop.outgoing.missingheads:
+        allowunrelated = 'related' in bundler.capabilities.get('checkheads', ())
+        if not allowunrelated:
+            bundler.newpart('check:heads', data=iter(pushop.remoteheads))
+        else:
+            affected = set()
+            for branch, heads in pushop.pushbranchmap.iteritems():
+                remoteheads, newheads, unsyncedheads, discardedheads = heads
+                if remoteheads is not None:
+                    remote = set(remoteheads)
+                    affected |= set(discardedheads) & remote
+                    affected |= remote - set(newheads)
+            if affected:
+                data = iter(sorted(affected))
+                bundler.newpart('check:updated-heads', data=data)
 
 @b2partsgenerator('changeset')
 def _pushb2ctx(pushop, bundler):
diff --git a/tests/test-push-race.t b/tests/test-push-race.t
--- a/tests/test-push-race.t
+++ b/tests/test-push-race.t
@@ -102,6 +102,21 @@  A set of extension and shell functions e
   > graph = log -G --rev 'sort(all(), "topo")'
   > EOF
 
+We tests multiple cases:
+* strict: no race detected,
+* unrelated: race on unrelated heads are allowed.
+
+#testcases strict unrelated
+
+#if unrelated
+
+  $ cat >> $HGRCPATH << EOF
+  > [experimental]
+  > checkheads-strict = no
+  > EOF
+
+#endif
+
 Setup
 -----
 
@@ -265,6 +280,7 @@  Pushing
 
 Check the result of the push
 
+#if strict
   $ cat ./push-log
   pushing to ssh://user@dummy/server
   searching for changes
@@ -282,6 +298,34 @@  Check the result of the push
   |/
   @  842e2fac6304 C-ROOT (default)
   
+#endif
+#if unrelated
+
+(The two heads are unrelated, push should be allowed)
+
+  $ cat ./push-log
+  pushing to ssh://user@dummy/server
+  searching for changes
+  wrote ready: $TESTTMP/readyfile
+  waiting on: $TESTTMP/watchfile
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files
+
+  $ hg -R server graph
+  o  59e76faf78bd C-D (default)
+  |
+  o  a9149a1428e2 C-B (default)
+  |
+  | o  51c544a58128 C-C (default)
+  | |
+  | o  98217d5a1659 C-A (default)
+  |/
+  @  842e2fac6304 C-ROOT (default)
+  
+#endif
+
 Pushing while someone creates a new head
 -----------------------------------------
 
@@ -295,6 +339,8 @@  Pushing a new changeset while someone cr
 
 (resync-all)
 
+#if strict
+
   $ hg -R ./server pull ./client-racy
   pulling from ./client-racy
   searching for changes
@@ -303,6 +349,17 @@  Pushing a new changeset while someone cr
   adding file changes
   added 1 changesets with 1 changes to 1 files
   (run 'hg update' to get a working copy)
+
+#endif
+#if unrelated
+
+  $ hg -R ./server pull ./client-racy
+  pulling from ./client-racy
+  searching for changes
+  no changes found
+
+#endif
+
   $ hg -R ./client-other pull
   pulling from ssh://user@dummy/server
   searching for changes
@@ -367,6 +424,8 @@  Pushing
 
 Check the result of the push
 
+#if strict
+
   $ cat ./push-log
   pushing to ssh://user@dummy/server
   searching for changes
@@ -389,6 +448,39 @@  Check the result of the push
   @  842e2fac6304 C-ROOT (default)
   
 
+#endif
+
+#if unrelated
+
+(The racing new head do not affect existing heads, push should go through)
+
+  $ cat ./push-log
+  pushing to ssh://user@dummy/server
+  searching for changes
+  wrote ready: $TESTTMP/readyfile
+  waiting on: $TESTTMP/watchfile
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files
+
+  $ hg -R server graph
+  o  d9e379a8c432 C-F (default)
+  |
+  o  51c544a58128 C-C (default)
+  |
+  | o  d603e2c0cdd7 C-E (default)
+  |/
+  o  98217d5a1659 C-A (default)
+  |
+  | o  59e76faf78bd C-D (default)
+  | |
+  | o  a9149a1428e2 C-B (default)
+  |/
+  @  842e2fac6304 C-ROOT (default)
+  
+#endif
+
 Pushing touching different named branch (same topo): new branch raced
 ---------------------------------------------------------------------
 
@@ -402,6 +494,8 @@  Pushing two children on the same head, o
 
 (resync-all)
 
+#if strict
+
   $ hg -R ./server pull ./client-racy
   pulling from ./client-racy
   searching for changes
@@ -410,6 +504,17 @@  Pushing two children on the same head, o
   adding file changes
   added 1 changesets with 1 changes to 1 files
   (run 'hg update' to get a working copy)
+
+#endif
+#if unrelated
+
+  $ hg -R ./server pull ./client-racy
+  pulling from ./client-racy
+  searching for changes
+  no changes found
+
+#endif
+
   $ hg -R ./client-other pull
   pulling from ssh://user@dummy/server
   searching for changes
@@ -480,6 +585,7 @@  Pushing
 
 Check the result of the push
 
+#if strict
   $ cat ./push-log
   pushing to ssh://user@dummy/server
   searching for changes
@@ -505,6 +611,43 @@  Check the result of the push
   |/
   @  842e2fac6304 C-ROOT (default)
   
+#endif
+#if unrelated
+
+(unrelated named branches are unrelated)
+
+  $ cat ./push-log
+  pushing to ssh://user@dummy/server
+  searching for changes
+  wrote ready: $TESTTMP/readyfile
+  waiting on: $TESTTMP/watchfile
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
+
+  $ hg -R server graph
+  o  833be552cfe6 C-H (my-first-test-branch)
+  |
+  | o  75d69cba5402 C-G (default)
+  |/
+  o  d9e379a8c432 C-F (default)
+  |
+  o  51c544a58128 C-C (default)
+  |
+  | o  d603e2c0cdd7 C-E (default)
+  |/
+  o  98217d5a1659 C-A (default)
+  |
+  | o  59e76faf78bd C-D (default)
+  | |
+  | o  a9149a1428e2 C-B (default)
+  |/
+  @  842e2fac6304 C-ROOT (default)
+  
+#endif
+
+The racing new head do not affect existing heads, push should go through
 
 pushing touching different named branch (same topo): old branch raced
 ---------------------------------------------------------------------
@@ -519,6 +662,8 @@  Pushing two children on the same head, o
 
 (resync-all)
 
+#if strict
+
   $ hg -R ./server pull ./client-racy
   pulling from ./client-racy
   searching for changes
@@ -527,6 +672,17 @@  Pushing two children on the same head, o
   adding file changes
   added 1 changesets with 1 changes to 1 files (+1 heads)
   (run 'hg heads .' to see heads, 'hg merge' to merge)
+
+#endif
+#if unrelated
+
+  $ hg -R ./server pull ./client-racy
+  pulling from ./client-racy
+  searching for changes
+  no changes found
+
+#endif
+
   $ hg -R ./client-other pull
   pulling from ssh://user@dummy/server
   searching for changes
@@ -600,6 +756,8 @@  Pushing
 
 Check the result of the push
 
+#if strict
+
   $ cat ./push-log
   pushing to ssh://user@dummy/server
   searching for changes
@@ -630,6 +788,48 @@  Check the result of the push
   @  842e2fac6304 C-ROOT (default)
   
 
+#endif
+
+#if unrelated
+
+(unrelated named branches are unrelated)
+
+  $ cat ./push-log
+  pushing to ssh://user@dummy/server
+  searching for changes
+  wrote ready: $TESTTMP/readyfile
+  waiting on: $TESTTMP/watchfile
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
+
+  $ hg -R server graph
+  o  89420bf00fae C-J (default)
+  |
+  | o  b35ed749f288 C-I (my-second-test-branch)
+  |/
+  o  75d69cba5402 C-G (default)
+  |
+  | o  833be552cfe6 C-H (my-first-test-branch)
+  |/
+  o  d9e379a8c432 C-F (default)
+  |
+  o  51c544a58128 C-C (default)
+  |
+  | o  d603e2c0cdd7 C-E (default)
+  |/
+  o  98217d5a1659 C-A (default)
+  |
+  | o  59e76faf78bd C-D (default)
+  | |
+  | o  a9149a1428e2 C-B (default)
+  |/
+  @  842e2fac6304 C-ROOT (default)
+  
+
+#endif
+
 pushing racing push touch multiple heads
 ----------------------------------------
 
@@ -644,6 +844,8 @@  There are multiple heads, but the racing
 
 (resync-all)
 
+#if strict
+
   $ hg -R ./server pull ./client-racy
   pulling from ./client-racy
   searching for changes
@@ -652,6 +854,18 @@  There are multiple heads, but the racing
   adding file changes
   added 1 changesets with 1 changes to 1 files (+1 heads)
   (run 'hg heads .' to see heads, 'hg merge' to merge)
+
+#endif
+
+#if unrelated
+
+  $ hg -R ./server pull ./client-racy
+  pulling from ./client-racy
+  searching for changes
+  no changes found
+
+#endif
+
   $ hg -R ./client-other pull
   pulling from ssh://user@dummy/server
   searching for changes