Patchwork D6776: bookmarks: validate changes on push

login
register
mail settings
Submitter phabricator
Date Aug. 30, 2019, 5:55 p.m.
Message ID <differential-rev-PHID-DREV-ov2pme7jbr3ea5o3l6bs-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41443/
State New
Headers show

Comments

phabricator - Aug. 30, 2019, 5:55 p.m.
idlsoft created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/bundle2.py
  mercurial/exchange.py
  mercurial/localrepo.py
  tests/test-bookmarksconflict.t

CHANGE DETAILS




To: idlsoft, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 30, 2019, 6:02 p.m.
idlsoft added a comment.


  This is a potential fix for https://bz.mercurial-scm.org/show_bug.cgi?id=6193
  The actual validation is in `bundle2.handlebookmark`.
  The rest of the changes are there to pass the `force` parameter.
  I've also made `force` available to hooks. That way one can validate who can and cannot use `--force` on a shared repo.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers
Cc: mercurial-devel
phabricator - Sept. 1, 2019, 9:54 p.m.
valentin.gatienbaron added a comment.


  (just interested in this change, I don't have any power to actually accept it)
  
  Interesting. I've thought of `hg push -B` as forcing the move of the bookmark, and `hg push -r` as not forcing a bookmark move, as that's how they behave (well, mostly).
  I don't know if this is a bug: the intended behavior of `push -B` is not explicitly documented, but it at least appears to be consistent with `hg pull -B`.
  But if it wasn't for compatibility, the behavior you're trying to implement looks superior: it gives the ability to ensure that a push either moves a bookmark forward or fails, which is clearly useful. We have an extension that makes `hg push -r BOOKMARK` have such a behavior, but the fewer extensions the better.
  
  But given compatibility constraints, this behavior probably needs to be gated by a config option.
  
  About more technical aspects, I have a couple of remarks:
  
  - the change of behavior would be clearer in the diff if you added the test in a first change, and updated the code in a second change
  - the check should probably live in some combination of `exchange.{_processcompared,_pushb2checkbookmarks}` instead (that's where the "move forward" logic for push is right now)
  - using `hg push -f` to overwrite the remote bookmark is not great, because -f forces several completely unrelated things (new heads, removes race checks about bookmarks, something about obsolescence markers and something about phases). No one wants to bypass all these at once. Dedicated force flags seems better, perhaps `hg push --force-bookmark BOOKMARK`, to replicate the current behavior of `hg push -B BOOKMARK`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers
Cc: valentin.gatienbaron, mercurial-devel
phabricator - Sept. 3, 2019, 3:10 p.m.
idlsoft added a comment.


  I see `--force` as a general "I know what I'm doing, forget your checks"  instruction.
  If it's not, then all the variations would need their own `--force-heads`, `--force-bookmarks`, `--force-*`.
  May be a bit much.
  
  As for this being configurable?
  I, personally, don't see a lot of value in the current behavior.
  There may be a use case, but the default behavior, useful for most users, would be to check it, I think.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers
Cc: valentin.gatienbaron, mercurial-devel
phabricator - Sept. 9, 2019, 3:42 p.m.
durin42 added a comment.


  Clarifying I understand: this causes pushing a bookmark to fail in what cases that it currently succeeds? I think it's just the case of:
  
    o 2
    |  o 1 @
    o/ 0
  
  when I last pulled, `@` was on 0, and now I'm doing `hg push -B @`, but since my last pull the server advanced `@` to 2, meaning it's a non-linear update. Right?
  
  If so, I wholeheartedly approve, as this behavior has previously burned me and I think it's well past time we started breaking some compatibility edges on bookmarks to make them actually useful to share.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers
Cc: durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 9, 2019, 6:32 p.m.
idlsoft added a comment.


  Yes, server changed since your last pull, and you override the bookmark without so much as a warning.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers
Cc: durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 11, 2019, 2:16 p.m.
This revision is now accepted and ready to land.
durin42 added a comment.
durin42 accepted this revision.


  Queued this with lots of content added to the commit message. Thanks!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 11, 2019, 2:31 p.m.
durin42 added a comment.


  I get a fair number of test failures with this, eg `test-bookmarks-corner-case.t`:
  
    --- /usr/local/google/home/augie/hgtest/tests/test-bookmarks-corner-case.t
    +++ /usr/local/google/home/augie/hgtest/tests/test-bookmarks-corner-case.t.err
    @@ -178,11 +178,48 @@
       $ hg push -R client-B -r book-B
       pushing to ssh://user@dummy/bookrace-server
       searching for changes
    -  remote: adding changesets
    -  remote: adding manifests
    -  remote: adding file changes
    -  remote: added 1 changesets with 1 changes to 1 files
    -  updating bookmark book-B
    +  ** unknown exception encountered, please report by visiting
    +  ** https://mercurial-scm.org/wiki/BugTracker
    +  ** Python 2.7.16 (default, Apr  6 2019, 01:42:57) [GCC 7.3.0]
    +  ** Mercurial Distributed SCM (version 5.1.1+253-85a0f0de90e5)
    +  ** Extensions loaded:
    +  Traceback (most recent call last):
    +    File "/tmp/hgtests.lYOPXh/install/bin/hg", line 43, in <module>
    +      dispatch.run()
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 99, in run
    +      status = dispatch(req)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 225, in dispatch
    +      ret = _runcatch(req) or 0
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 376, in _runcatch
    +      return _callcatch(ui, _runcatchfunc)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 384, in _callcatch
    +      return scmutil.callcatch(ui, func)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/scmutil.py", line 167, in callcatch
    +      return func()
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 367, in _runcatchfunc
    +      return _dispatch(req)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 1021, in _dispatch
    +      cmdpats, cmdoptions)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 756, in runcommand
    +      ret = _runcommand(ui, options, cmd, d)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 1030, in _runcommand
    +      return cmdfunc()
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/dispatch.py", line 1018, in <lambda>
    +      d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/util.py", line 1682, in check
    +      return func(*args, **kwargs)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/commands.py", line 4667, in push
    +      opargs=opargs)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/exchange.py", line 570, in push
    +      _pushbundle2(pushop)
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/exchange.py", line 1164, in _pushbundle2
    +      'force': pushop.force,
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/thirdparty/concurrent/futures/_base.py", line 457, in result
    +      return self.__get_result()
    +    File "/tmp/hgtests.lYOPXh/install/lib/python/mercurial/wireprotov1peer.py", line 211, in sendcommands
    +      result = fn(**pycompat.strkwargs(args))
    +  TypeError: unbundle() got an unexpected keyword argument 'force'
    +  [1]
  
  Could I get you to take a look? I like where this is going, but I don't have the time at the moment to hunt down the issues in the tests.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 11, 2019, 2:32 p.m.
This revision now requires changes to proceed.
durin42 added a comment.
durin42 requested changes to this revision.


  I've uploaded my revised version (with the more complete commit message - no other changes) in case that helps.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 11, 2019, 6:07 p.m.
durin42 added a comment.


  We're really close. I've uploaded my rebase of this to the latest dev hg (along with some minor test fixes). There's now only one failure:
  
    --- /Users/augie/Programming/hg/crew/tests/test-bookmarks-pushpull.t
    +++ /Users/augie/Programming/hg/crew/tests/test-bookmarks-pushpull.t#b2-binary.err
    @@ -820,15 +820,17 @@
       pushing to http://localhost:$HGPORT/
       searching for changes
       no changes found
    -  updating bookmark Z
    -  [1]
    +  remote: push rejected: bookmark "Z" has changed
    +  remote: (run 'hg pull', resolve conflicts, and push again)
    +  abort: push failed on remote
    +  [255]
       $ hg book -d Z
       $ hg in -B http://localhost:$HGPORT/
       comparing with http://localhost:$HGPORT/
       searching for changed bookmarks
          @                         9b140be10808
          X                         9b140be10808
    -     Z                         0d2164f0ce0d
    +     Z                         9b140be10808
          foo                       000000000000
          foobar                    9b140be10808
       $ hg pull -B Z http://localhost:$HGPORT/
    @@ -853,7 +855,7 @@
        * @                         1:9b140be10808
          X                         1:9b140be10808
          Y                         4:c922c0139ca0
    -     Z                         2:0d2164f0ce0d
    +     Z                         1:9b140be10808
          foo                       -1:000000000000
          foobar                    1:9b140be10808
     
    
    ERROR: test-bookmarks-pushpull.t#b2-binary output changed
  
  My gut at this point is that we should:
  
  1. Document that legacy bookmark push behavior over pushkey is different
  2. Add a config to reject bookmark pushes over pushkey
  
  But if there's a way to fix the pushkey behavior too, I'd be interested in that. I'm just not sure it's worth blocking on when that should only matter with very old servers. Obviously if others have stronger opinions they should speak up.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 12, 2019, 6:06 a.m.
pulkit added a comment.


  I agree with @valentin.gatienbaron that `--force` is quite generic :(.  It may create new heads on the server even if I don't want to.

INLINE COMMENTS

> bundle2.py:2149
> +                        hint=
> +                        _("run 'hg pull', resolve conflicts, and push again"))
> +            hookargs = tr.hookargs.copy()

Hm, I think `conflicts` can be confusing but I don't have better suggestions here.

> repository.py:194
>  
> -    def unbundle(bundle, heads, url):
> +    def unbundle(bundle, heads, url, force=None):
>          """Transfer repository data to the peer.

It will be nice to have documentation about what `force` does in the interface.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: pulkit, durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 13, 2019, 4:33 a.m.
valentin.gatienbaron added a comment.


  I showed in D6847 <https://phab.mercurial-scm.org/D6847> the same change but implemented in exchange._processcompared. Tests pass.
  I think it'd make for a simpler final state: with the current change, the client sees that the bookmark is going to move sideways, decides this is fine, requests that the server validates that the bookmark is indeed moving sideways (which it does), but in the end the server rejects the move. In the suggested change, the client sees that the bookmark is going sideways and rejects it. This should be consistent with the way new heads or new branches or diverging rewrites are prevented.

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: pulkit, durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 13, 2019, 1:40 p.m.
durin42 added a comment.


  In D6776#100487 <https://phab.mercurial-scm.org/D6776#100487>, @valentin.gatienbaron wrote:
  
  > I showed in D6847 <https://phab.mercurial-scm.org/D6847> the same change but implemented in exchange._processcompared. Tests pass.
  > I think it'd make for a simpler final state: with the current change, the client sees that the bookmark is going to move sideways, decides this is fine, requests that the server validates that the bookmark is indeed moving sideways (which it does), but in the end the server rejects the move. In the suggested change, the client sees that the bookmark is going sideways and rejects it. This should be consistent with the way new heads or new branches or diverging rewrites are prevented.
  
  Isn't that a client-side change only though, so we still need functionality on the server to reject bad pushes? (I could be missing something.)

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: pulkit, durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 13, 2019, 9:39 p.m.
idlsoft added a comment.


  > Isn't that a client-side change only though, so we still need functionality on the server to reject bad pushes? (I could be missing something.)
  
  I'm actually not sure how to make this work for http, it doesn't seem to propagate `--force` to `unbundle`.
  Which is why the line in **test-bookmarks-pushpull.t** is `--config devel.legacy.exchange=bookmarks` instead of `--force`

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: pulkit, durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 14, 2019, 5:56 a.m.
valentin.gatienbaron added a comment.


  In D6776#100506 <https://phab.mercurial-scm.org/D6776#100506>, @durin42 wrote:
  
  > In D6776#100487 <https://phab.mercurial-scm.org/D6776#100487>, @valentin.gatienbaron wrote:
  >
  >> I showed in D6847 <https://phab.mercurial-scm.org/D6847> the same change but implemented in exchange._processcompared. Tests pass.
  >> I think it'd make for a simpler final state: with the current change, the client sees that the bookmark is going to move sideways, decides this is fine, requests that the server validates that the bookmark is indeed moving sideways (which it does), but in the end the server rejects the move. In the suggested change, the client sees that the bookmark is going sideways and rejects it. This should be consistent with the way new heads or new branches or diverging rewrites are prevented.
  >
  > Isn't that a client-side change only though, so we still need functionality on the server to reject bad pushes? (I could be missing something.)
  
  If you mean "reject bad pushes as long as people don't push -f", both versions implement this (the client-side check is not racy thanks to these check:bookmark bundle parts).
  If you mean "reject bad pushes, push -f or not", a server-side hook is necessary for that, and hook.pretxnclose-bookmark should be able to do the job without any server code change. Server code change can certainly simplify writing such checks, but passing the force parameter on the procol is not needed.

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: pulkit, durin42, valentin.gatienbaron, mercurial-devel
phabricator - Sept. 14, 2019, 6:15 a.m.
idlsoft added a comment.


  > If you mean "reject bad pushes as long as people don't push -f", both versions implement this (the client-side check is not racy thanks to these check:bookmark bundle parts).
  
  That was the idea. This implementation is a bit more flexible I think.
  Since it’s server side, you don’t have to upgrade your clients.
  It’s true old clients won’t be able to use —force, but new ones can.
  And you can add a trigger if you want to limit who can use —force.

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6776/new/

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

To: idlsoft, #hg-reviewers, durin42
Cc: pulkit, durin42, valentin.gatienbaron, mercurial-devel

Patch

diff --git a/tests/test-bookmarksconflict.t b/tests/test-bookmarksconflict.t
new file mode 100644
--- /dev/null
+++ b/tests/test-bookmarksconflict.t
@@ -0,0 +1,84 @@ 
+initialize
+  $ make_changes() {
+  >     d=`pwd`
+  >     [ ! -z $1 ] && cd $1
+  >     echo "test `basename \`pwd\``" >> test
+  >     hg commit -Am"${2:-test}"
+  >     r=$?
+  >     cd $d
+  >     return $r
+  > }
+  $ ls -1a
+  .
+  ..
+  $ hg init a
+  $ cd a
+  $ echo 'test' > test; hg commit -Am'test'
+  adding test
+  $ hg book @
+
+clone to b
+
+  $ mkdir ../b
+  $ cd ../b
+  $ hg clone ../a .
+  updating to bookmark @
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ make_changes
+  $ hg book bk_b
+
+clone to c
+  $ mkdir ../c
+  $ cd ../c
+  $ hg clone ../a .
+  updating to bookmark @
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ make_changes
+  $ hg book bk_c
+
+push from b
+  $ cd ../b
+  $ hg push -B .
+  pushing to $TESTTMP/a
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  updating bookmark @
+  exporting bookmark bk_b
+  $ hg -R ../a id -r @
+  e11a942451be tip @/bk_b
+
+push from c
+  $ cd ../c
+  $ hg push -B .
+  pushing to $TESTTMP/a
+  searching for changes
+  remote has heads on branch 'default' that are not known locally: e11a942451be
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files (+1 heads)
+  exporting bookmark bk_c
+  $ hg push -B @
+  pushing to $TESTTMP/a
+  searching for changes
+  no changes found
+  abort: push rejected: bookmark "@" has changed
+  (run 'hg pull', resolve conflicts, and push again)
+  [255]
+  $ hg -R ../a book
+   * @                         1:e11a942451be
+     bk_b                      1:e11a942451be
+     bk_c                      2:110743c8a16c
+  $ hg push -B @ --force
+  pushing to $TESTTMP/a
+  searching for changes
+  no changes found
+  updating bookmark @
+  [1]
+  $ hg -R ../a book
+   * @                         2:110743c8a16c
+     bk_b                      1:e11a942451be
+     bk_c                      2:110743c8a16c
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -307,14 +307,14 @@ 
         raise error.Abort(_('cannot perform stream clone against local '
                             'peer'))
 
-    def unbundle(self, bundle, heads, url):
+    def unbundle(self, bundle, heads, url, force=None):
         """apply a bundle on a repo
 
         This function handles the repo locking itself."""
         try:
             try:
                 bundle = exchange.readbundle(self.ui, bundle, None)
-                ret = exchange.unbundle(self._repo, bundle, heads, 'push', url)
+                ret = exchange.unbundle(self._repo, bundle, heads, 'push', url, force=force)
                 if util.safehasattr(ret, 'getchunks'):
                     # This is a bundle20 object, turn it into an unbundler.
                     # This little dance should be dropped eventually when the
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1149,6 +1149,7 @@ 
                     'bundle': stream,
                     'heads': ['force'],
                     'url': pushop.remote.url(),
+                    'force': pushop.force,
                 }).result()
         except error.BundleValueError as exc:
             raise error.Abort(_('missing support for %s') % exc)
@@ -2362,7 +2363,7 @@ 
         raise error.PushRaced('repository changed while %s - '
                               'please try again' % context)
 
-def unbundle(repo, cg, heads, source, url):
+def unbundle(repo, cg, heads, source, url, force=None):
     """Apply a bundle to a repo.
 
     this function makes sure the repo is locked during the application and have
@@ -2410,7 +2411,8 @@ 
 
                 op = bundle2.bundleoperation(repo, gettransaction,
                                              captureoutput=captureoutput,
-                                             source='push')
+                                             source='push',
+                                             force=force)
                 try:
                     op = bundle2.processbundle(repo, cg, op=op)
                 finally:
diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -299,7 +299,7 @@ 
     * a way to construct a bundle response when applicable.
     """
 
-    def __init__(self, repo, transactiongetter, captureoutput=True, source=''):
+    def __init__(self, repo, transactiongetter, captureoutput=True, source='', force=False):
         self.repo = repo
         self.ui = repo.ui
         self.records = unbundlerecords()
@@ -310,6 +310,7 @@ 
         # carries value that can modify part behavior
         self.modes = {}
         self.source = source
+        self.force = force
 
     def gettransaction(self):
         transaction = self._gettransaction()
@@ -2135,17 +2136,23 @@ 
     if bookmarksmode == 'apply':
         tr = op.gettransaction()
         bookstore = op.repo._bookmarks
+        allhooks = []
+        for book, node in changes:
+            old_node = bookstore.get(book, '')
+            if not op.force and old_node != '' and node is not None:
+                if not bookmarks.validdest(op.repo, op.repo[old_node], op.repo[node]):
+                    raise error.Abort(_('push rejected: bookmark "%s" has changed') % book,
+                                      hint=_("run 'hg pull', resolve conflicts, and push again"))
+            hookargs = tr.hookargs.copy()
+            hookargs['pushkeycompat'] = '1'
+            hookargs['namespace'] = 'bookmarks'
+            hookargs['key'] = book
+            hookargs['old'] = nodemod.hex(old_node)
+            hookargs['new'] = nodemod.hex(node if node is not None else '')
+            hookargs['force'] = op.force
+            allhooks.append(hookargs)
+
         if pushkeycompat:
-            allhooks = []
-            for book, node in changes:
-                hookargs = tr.hookargs.copy()
-                hookargs['pushkeycompat'] = '1'
-                hookargs['namespace'] = 'bookmarks'
-                hookargs['key'] = book
-                hookargs['old'] = nodemod.hex(bookstore.get(book, ''))
-                hookargs['new'] = nodemod.hex(node if node is not None else '')
-                allhooks.append(hookargs)
-
             for hookargs in allhooks:
                 op.repo.hook('prepushkey', throw=True,
                              **pycompat.strkwargs(hookargs))