This is an archive of the discontinued Mercurial Phabricator instance.

bookmarks: validate changes on push (issue6193) (BC)
Needs RevisionPublic

Authored by idlsoft on Aug 30 2019, 1:54 PM.

Details

Reviewers
durin42
baymax
Group Reviewers
hg-reviewers
Summary

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.

In other words, in 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 of the bookmark. This used to be allowed, and is now
prohibited without --force.

.. bc::

`hg push --bookmark` and `hg push -B` no longer move a bookmark
during push if the new bookmark position isn't a descendant of the
current bookmark position.

.. feature::

Hooks are now able to tell if --force was specified when a bookmark was pushed.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

idlsoft created this revision.Aug 30 2019, 1:54 PM

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.

(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.

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.

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.

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

durin42 accepted this revision.Sep 11 2019, 10:16 AM

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

This revision is now accepted and ready to land.Sep 11 2019, 10:16 AM

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.

durin42 retitled this revision from bookmarks: validate changes on push to bookmarks: validate changes on push (issue6193) (BC).Sep 11 2019, 10:31 AM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 16515.
durin42 requested changes to this revision.Sep 11 2019, 10:32 AM

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

This revision now requires changes to proceed.Sep 11 2019, 10:32 AM
idlsoft updated this revision to Diff 16518.Sep 11 2019, 11:52 AM
durin42 updated this revision to Diff 16519.Sep 11 2019, 2:05 PM

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.

pulkit added a subscriber: pulkit.Sep 12 2019, 2:06 AM

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.

mercurial/bundle2.py
2148

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

mercurial/interfaces/repository.py
194 ↗(On Diff #16519)

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

idlsoft updated this revision to Diff 16524.Sep 12 2019, 1:46 PM

I showed in 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.

I showed in 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.)

idlsoft added a comment.EditedSep 13 2019, 5:39 PM

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 updated the diff to propagate --force properly

idlsoft updated this revision to Diff 16531.Sep 13 2019, 8:22 PM
idlsoft updated this revision to Diff 16533.Sep 13 2019, 10:34 PM
idlsoft updated this revision to Diff 16534.Sep 14 2019, 12:52 AM

I showed in 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.

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.

baymax requested changes to this revision.Jan 24 2020, 12:31 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:31 PM

As @valentin.gatienbaron pointed out, the now avoid adding more semantic to bare --force with an associated --force-bookmark especially because we want to be able to select the bookmarks that get force pushed here.

As I

As @valentin.gatienbaron pointed out, the now avoid adding more semantic to bare --force with an associated --force-bookmark especially because we want to be able to select the bookmarks that get force pushed here.

I've expressed my concerns about that earlier, but ultimately it's a debate for core maintainers, we'll use whatever they decide.
Without a clear decision it didn't make much sense to modify the PR.
I'm not sure if I'll have the bandwith to resume working on this, but glad to see it getting renewed attention.

mjacob added a subscriber: mjacob.Aug 6 2020, 5:00 PM

I agree (with some previous answers) that adding more stuff to --force should be avoided.

I planned to send a proposal (and patches) after the 5.5 release for adding more detailed options that can be used instead of --force. However, due to unexpected reasons, I don’t know whether I’ll have the time for doing that.