D6776: bookmarks: validate changes on push

[valentin.gatienbaron (Valentin Gatien-Baron)]

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