Patchwork Backed out changeset 5c153c69fdb2

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 2, 2014, 7:12 p.m.
Message ID <8450cb5eddbd7396f3dc.1409685159@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5681/
State Changes Requested
Headers show

Comments

Pierre-Yves David - Sept. 2, 2014, 7:12 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1409683301 -7200
#      Tue Sep 02 20:41:41 2014 +0200
# Node ID 8450cb5eddbd7396f3dc7b50abdcdeab109b9b2d
# Parent  5c153c69fdb28ff5b1bf6578e5f07c50bf25833c
Backed out changeset 5c153c69fdb2

I still strongly believe that the special case that can trigger that warning is
not important enough to break the generic behavior of Mercurial regarding race
detection during push.

There is two situations were this may happens:

1) Three is actually two people pushing on the repo at the same time and
   something is getting raced (since bundle1 push are not atomic)

2) A 3.2 client push to an old server using bundle1 and this server as a hook
   change bookmark location in addchangegroup.

   This happen because as future 3.2 is able to use bundle2, it has to computes
   bookmark movement before he push the changesets (instead of right before
   pushing the bookmarks). So if the server moves it in another step of the
   push, the client old value used in pushkey by the client is outdated.

In my opinion, printing a warning for (1) is valid. There is a race going on we should warn.

I think that (2) is not a good reason for breaking the general case.
Transactionally with bundle1 is a mess. Writing hook to move bookmark in bundle1
setup is brave and fragile.  If there is people out there actually using hook
for that, They -should- upgrade to 3.2 to benefit from the transactionally of
bundle2. Having new clients warn when touching old server will be valuable to
help server admin to detect that an upgrade is due. Old clients are not affect
since they do the discovery right before pushing bookmarks. New client talking
to new server is not affected either since they will (hopefully) use bundle2.
Matt Mackall - Oct. 2, 2014, 6:07 p.m.
On Tue, 2014-09-02 at 21:12 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1409683301 -7200
> #      Tue Sep 02 20:41:41 2014 +0200
> # Node ID 8450cb5eddbd7396f3dc7b50abdcdeab109b9b2d
> # Parent  5c153c69fdb28ff5b1bf6578e5f07c50bf25833c
> Backed out changeset 5c153c69fdb2

After in-person discussion, we're going to leave this be for now.
Durham Goode - Oct. 2, 2014, 10:32 p.m.
On 9/2/14, 12:12 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1409683301 -7200
> #      Tue Sep 02 20:41:41 2014 +0200
> # Node ID 8450cb5eddbd7396f3dc7b50abdcdeab109b9b2d
> # Parent  5c153c69fdb28ff5b1bf6578e5f07c50bf25833c
> Backed out changeset 5c153c69fdb2
>
Please cc the person being backed out as well.

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -226,12 +226,11 @@  def listbookmarks(repo):
 
 def pushbookmark(repo, key, old, new):
     w = repo.wlock()
     try:
         marks = repo._bookmarks
-        existing = hex(marks.get(key, ''))
-        if existing != old and existing != new:
+        if hex(marks.get(key, '')) != old:
             return False
         if new == '':
             del marks[key]
         else:
             if new not in repo: