Patchwork [4,of,4] pull: list bookmarks before pulling changesets (issue3873)

login
register
mail settings
Submitter Siddharth Agarwal
Date March 30, 2013, 2:58 a.m.
Message ID <720475f6e382359a2057.1364612307@sid0x220>
Download mbox | patch
Permalink /patch/1229/
State Accepted
Commit a60963c02f9281efabc5d554eb2bd3deb692c6a3
Headers show

Comments

Siddharth Agarwal - March 30, 2013, 2:58 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1364612046 25200
#      Fri Mar 29 19:54:06 2013 -0700
# Node ID 720475f6e382359a20579224b1756a8c532a47a4
# Parent  16a8db1339b1dc7734fefdaca99beb3a3fa14d49
pull: list bookmarks before pulling changesets (issue3873)

Consider a bookmark B that exists both locally and remotely. If B is updated
remotely, and then a pull is performed where the pull set contains the new
location of B, the bookmark is updated locally. However, if remote B is
updated in the middle of a pull to a location not in the pull set, the
bookmark won't be updated locally at all.

To fix this, list bookmarks before pulling in changesets, not after. This
still leaves a race open if B gets moved in between listing bookmarks and
pulling in changesets, but the race window is much smaller. Fixing the race
properly would require a bundle format upgrade.

test-hook.t's output changes because we no longer do two listkeys calls during
pull, just one.

test-pull-http.t's output changes because we now search for bookmarks before
searching for changes.
Bryan O'Sullivan - April 1, 2013, 9:02 p.m.
On Fri, Mar 29, 2013 at 7:58 PM, Siddharth Agarwal <sid0@fb.com> wrote:

> pull: list bookmarks before pulling changesets (issue3873)
>

Crewed, thanks.

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -221,9 +221,8 @@  def pushbookmark(repo, key, old, new):
     finally:
         w.release()
 
-def updatefromremote(ui, repo, remote, path):
+def updatefromremote(ui, repo, remotemarks, path):
     ui.debug("checking for updated bookmarks\n")
-    remotemarks = remote.listkeys('bookmarks')
     changed = False
     localmarks = repo._bookmarks
     for k in sorted(remotemarks):
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4496,10 +4496,11 @@  def pull(ui, repo, source="default", **o
     ui.status(_('pulling from %s\n') % util.hidepassword(source))
     revs, checkout = hg.addbranchrevs(repo, other, branches, opts.get('rev'))
 
+    remotebookmarks = other.listkeys('bookmarks')
+
     if opts.get('bookmark'):
         if not revs:
             revs = []
-        remotebookmarks = other.listkeys('bookmarks')
         for b in opts['bookmark']:
             if b not in remotebookmarks:
                 raise util.Abort(_('remote bookmark %s not found!') % b)
@@ -4514,7 +4515,7 @@  def pull(ui, repo, source="default", **o
             raise util.Abort(err)
 
     modheads = repo.pull(other, heads=revs, force=opts.get('force'))
-    bookmarks.updatefromremote(ui, repo, other, source)
+    bookmarks.updatefromremote(ui, repo, remotebookmarks, source)
     if checkout:
         checkout = str(repo.changelog.rev(other.lookup(checkout)))
     repo._subtoppath = source
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -547,9 +547,10 @@  class hgsubrepo(abstractsubrepo):
             else:
                 self._repo.ui.status(_('pulling subrepo %s from %s\n')
                                      % (subrelpath(self), srcurl))
+                remotebookmarks = other.listkeys('bookmarks')
                 self._repo.pull(other)
-                bookmarks.updatefromremote(self._repo.ui, self._repo, other,
-                                           srcurl)
+                bookmarks.updatefromremote(self._repo.ui, self._repo,
+                                           remotebookmarks, srcurl)
 
     @annotatesubrepoerror
     def get(self, state, overwrite=False):
diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t
+++ b/tests/test-bookmarks-pushpull.t
@@ -204,6 +204,39 @@  update a remote bookmark from a non-head
      Y                         3:f6fc62dde3c0
      Z                         1:0d2164f0ce0d
 
+update a bookmark in the middle of a client pulling changes
+
+  $ cd ..
+  $ hg clone -q a pull-race
+  $ hg clone -q pull-race pull-race2
+  $ cd pull-race
+  $ hg up -q Y
+  $ echo c4 > f2
+  $ hg ci -Am4
+  $ echo c5 > f3
+  $ cat <<EOF > .hg/hgrc
+  > [hooks]
+  > outgoing.makecommit = hg ci -Am5; echo committed in pull-race
+  > EOF
+  $ cd ../pull-race2
+  $ hg pull
+  pulling from $TESTTMP/pull-race (glob)
+  searching for changes
+  adding changesets
+  adding f3
+  committed in pull-race
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  updating bookmark Y
+  (run 'hg update' to get a working copy)
+  $ hg book
+   * @                         1:0d2164f0ce0d
+     X                         1:0d2164f0ce0d
+     Y                         4:b0a5eff05604
+     Z                         1:0d2164f0ce0d
+  $ cd ../b
+
 diverging a remote bookmark fails
 
   $ hg up -q 4e3505fd9583
diff --git a/tests/test-hook.t b/tests/test-hook.t
--- a/tests/test-hook.t
+++ b/tests/test-hook.t
@@ -198,7 +198,6 @@  listkeys hook
   listkeys hook: HG_NAMESPACE=bookmarks HG_VALUES={'bar': '0000000000000000000000000000000000000000', 'foo': '0000000000000000000000000000000000000000'}
   no changes found
   listkeys hook: HG_NAMESPACE=phases HG_VALUES={'cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b': '1', 'publishing': 'True'}
-  listkeys hook: HG_NAMESPACE=bookmarks HG_VALUES={'bar': '0000000000000000000000000000000000000000', 'foo': '0000000000000000000000000000000000000000'}
   adding remote bookmark bar
   importing bookmark bar
   $ cd ../a
diff --git a/tests/test-pull-http.t b/tests/test-pull-http.t
--- a/tests/test-pull-http.t
+++ b/tests/test-pull-http.t
@@ -58,7 +58,6 @@  expect error, pulling not allowed
 
   $ req
   pulling from http://localhost:$HGPORT/
-  searching for changes
   abort: authorization failed
   % serve errors