Patchwork D2873: remotenames: add functionality to override -B flag of push

login
register
mail settings
Submitter phabricator
Date March 15, 2018, 1:49 p.m.
Message ID <differential-rev-PHID-DREV-lh4opb6zgb7nuqouor7t-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29532/
State New
Headers show

Comments

phabricator - March 15, 2018, 1:49 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch adds a new config option which can be used to override the `-B` flag
  of push command. If config is set to true, changesets will be pushed to that
  bookmark on the server. This is equivalent to `--to` flag of remotenames and
  infinitepush.
  
  After this patch if config is set to true, the behavior will be:
  
  - Normal push without -B flag: no behavior change
  - specifying one bookmark using -B flag: changesets will be pushed to that bookmark on the server, as explained above
  - specifying multiple bookmarks using -B flag: no behavior change, the same behavior as it's today without this config option
  
  There needs some extra logic to update the remotenames state locally after the
  push since we know that the bookmark will now be at a known updated location.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/remotenames.py
  tests/test-logexchange.t

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - March 16, 2018, 3 p.m.
pulkit added a comment.


  I am not sure the functionality and config names should go to core or not, so I have introduced them in this extension. Also suggestions for better config names are welcome.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - March 23, 2018, 6:56 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  I'm happy with this feature.
  
  But the code needs work around multiple revisions/heads before it can be queued.

INLINE COMMENTS

> remotenames.py:99
> +    bookmark = pushop.bookmarks[0]
> +    rev = pushop.revs[0]
> +

What happens if we're pushing multiple heads? I suspect this will choose a head/revision arbitrarily - maybe depending on the `-r` arguments to `hg push`.

I think we need to validate that the outgoing revs are in a single DAG head and we should then pick the rev that is the DAG head.

Please add test coverage for multiple `-r` arguments and `-r` arguments that resolve to multiple heads.

> remotenames.py:101
> +
> +    # allow new bookmark only if --create is specified
> +    old = ''

Nit: what is `--create`?

> remotenames.py:287
>      extensions.wrapfunction(bookmarks, '_printbookmarks', wrapprintbookmarks)
> +    exchange.pushdiscoverymapping['bookmarks'] = expushdiscoverybookmarks
>  

Strictly speaking, we should probably wrap `exchange._pushdiscoverybookmarks` so other extensions can get involved. But I think this is fine.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - April 1, 2018, 4:51 p.m.
pulkit added a comment.


  A gentle reminder for reviewing this series. If this config option gets in, we can use it in infinitepush extension too where for now we have overridden `-B` flag in a hacky way.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - April 11, 2018, 5:29 p.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


  Looks good!

INLINE COMMENTS

> remotenames.py:113
> +    else:
> +        msg = _("bookmark '%s' does not exists on remote")
> +        raise error.Abort(msg % bookmark)

Nit: "exist". This can be fixed in flight.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - April 11, 2018, 5:32 p.m.
smf added a comment.


  In https://phab.mercurial-scm.org/D2873#52021, @indygreg wrote:
  
  > Looks good!
  
  
  I'm very heavily against this direction. Changing the behavior of push (even in this extension) is something I've always considered outside the scope of remotenames. Having another extension that changes push behavior (e.g. bookmark-push) is where I think this should go so that remotenames is just that: keeping track of remote names.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: smf, indygreg, mercurial-devel
phabricator - April 11, 2018, 8:55 p.m.
indygreg added a comment.


  In https://phab.mercurial-scm.org/D2873#52025, @smf wrote:
  
  > In https://phab.mercurial-scm.org/D2873#52021, @indygreg wrote:
  >
  > > Looks good!
  >
  >
  > I'm very heavily against this direction. Changing the behavior of push (even in this extension) is something I've always considered outside the scope of remotenames. Having another extension that changes push behavior (e.g. bookmark-push) is where I think this should go so that remotenames is just that: keeping track of remote names.
  
  
  I think there's room for this feature to live outside of remotenames. But currently I think it is the best place for it, since remotenames is the closest thing we have to... tracking remote names. We can always alias the old config option in the future if we move this functionality elsewhere.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: smf, indygreg, mercurial-devel
phabricator - April 11, 2018, 9:39 p.m.
smf added a comment.


  In https://phab.mercurial-scm.org/D2873#52100, @indygreg wrote:
  
  > In https://phab.mercurial-scm.org/D2873#52025, @smf wrote:
  >
  > > In https://phab.mercurial-scm.org/D2873#52021, @indygreg wrote:
  > >
  > > > Looks good!
  > >
  > >
  > > I'm very heavily against this direction. Changing the behavior of push (even in this extension) is something I've always considered outside the scope of remotenames. Having another extension that changes push behavior (e.g. bookmark-push) is where I think this should go so that remotenames is just that: keeping track of remote names.
  >
  >
  > I think there's room for this feature to live outside of remotenames. But currently I think it is the best place for it, since remotenames is the closest thing we have to... tracking remote names. We can always alias the old config option in the future if we move this functionality elsewhere.
  
  
  One of the biggest regrets I have about the original remotenames, is the modifying of push (and some default) behavior. The `--to` is small enough (and I guess fine enough) for now but I absolutely and strongly believe that all the other behavior modifications should stay in an out-of-core repo for now. My goal is to split my remotenames repo to be based off of core's remotenames (basically only having the behavior changing patches there).

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: smf, indygreg, mercurial-devel

Patch

diff --git a/tests/test-logexchange.t b/tests/test-logexchange.t
--- a/tests/test-logexchange.t
+++ b/tests/test-logexchange.t
@@ -259,3 +259,186 @@ 
   o  3:62615734edd5 $TESTTMP/server2/foo default/foo
   |
   ~
+
+Testing the remotenames.pushtobookmark config option to push to bookmark
+
+  $ echo foo > foobar
+  $ hg add foobar
+  $ hg ci -m "added foobar"
+
+  $ hg log -G
+  @  changeset:   9:aa6a885086c0
+  |  branch:      wat
+  |  tag:         tip
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     added foobar
+  |
+  o  changeset:   8:3e1487808078
+  |  branch:      wat
+  |  remote branch:  $TESTTMP/server2/wat
+  |  remote branch:  default/wat
+  |  parent:      4:aa98ab95a928
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     added bar
+  |
+  | o  changeset:   7:ec2426147f0e
+  | |  remote branch:  $TESTTMP/server2/default
+  | |  remote branch:  default/default
+  | |  user:        test
+  | |  date:        Thu Jan 01 00:00:00 1970 +0000
+  | |  summary:     Added h
+  | |
+  | o  changeset:   6:87d6d6676308
+  | |  bookmark:    bar
+  | |  remote bookmark:  $TESTTMP/server2/bar
+  | |  remote bookmark:  default/bar
+  | |  user:        test
+  | |  date:        Thu Jan 01 00:00:00 1970 +0000
+  | |  summary:     Added g
+  | |
+  | o  changeset:   5:825660c69f0c
+  |/   user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    summary:     Added f
+  |
+  o  changeset:   4:aa98ab95a928
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added e
+  |
+  o  changeset:   3:62615734edd5
+  |  bookmark:    foo
+  |  remote bookmark:  $TESTTMP/server2/foo
+  |  remote bookmark:  default/foo
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added d
+  |
+  o  changeset:   2:28ad74487de9
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added c
+  |
+  o  changeset:   1:29becc82797a
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added b
+  |
+  o  changeset:   0:18d04c59bb5d
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     Added a
+  
+  $ echo '[remotenames]' >> .hg/hgrc
+  $ echo 'pushtobookmark = True' >> .hg/hgrc
+
+Trying to push a non-existing bookmark on the server
+
+  $ hg push ../server/ -B nonexistentbm
+  pushing to ../server/
+  searching for changes
+  abort: bookmark 'nonexistentbm' does not exists on remote
+  [255]
+
+  $ hg push ../server/ -B nonexistinentbm -f
+  pushing to ../server/
+  searching for changes
+  abort: bookmark 'nonexistinentbm' does not exists on remote
+  [255]
+
+Pushing changesets to a bookmark on the remote
+
+  $ hg bookmarks -R ../server/
+     bar                       6:87d6d6676308
+     foo                       3:62615734edd5
+
+  $ hg push ../server/ -r . -B foo
+  pushing to ../server/
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  updating bookmark foo
+
+  $ hg log -G -R ../server/ -r tip
+  o  changeset:   9:aa6a885086c0
+  |  branch:      wat
+  ~  bookmark:    foo
+     tag:         tip
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     added foobar
+  
+  $ hg bookmarks -R ../server/
+     bar                       6:87d6d6676308
+     foo                       9:aa6a885086c0
+
+XXX: remotebookmarks should have been updated after this push
+  $ hg log -G
+  @  changeset:   9:aa6a885086c0
+  |  branch:      wat
+  |  tag:         tip
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     added foobar
+  |
+  o  changeset:   8:3e1487808078
+  |  branch:      wat
+  |  remote branch:  $TESTTMP/server2/wat
+  |  remote branch:  default/wat
+  |  parent:      4:aa98ab95a928
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     added bar
+  |
+  | o  changeset:   7:ec2426147f0e
+  | |  remote branch:  $TESTTMP/server2/default
+  | |  remote branch:  default/default
+  | |  user:        test
+  | |  date:        Thu Jan 01 00:00:00 1970 +0000
+  | |  summary:     Added h
+  | |
+  | o  changeset:   6:87d6d6676308
+  | |  bookmark:    bar
+  | |  remote bookmark:  $TESTTMP/server2/bar
+  | |  remote bookmark:  default/bar
+  | |  user:        test
+  | |  date:        Thu Jan 01 00:00:00 1970 +0000
+  | |  summary:     Added g
+  | |
+  | o  changeset:   5:825660c69f0c
+  |/   user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    summary:     Added f
+  |
+  o  changeset:   4:aa98ab95a928
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added e
+  |
+  o  changeset:   3:62615734edd5
+  |  bookmark:    foo
+  |  remote bookmark:  $TESTTMP/server2/foo
+  |  remote bookmark:  default/foo
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added d
+  |
+  o  changeset:   2:28ad74487de9
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added c
+  |
+  o  changeset:   1:29becc82797a
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     Added b
+  |
+  o  changeset:   0:18d04c59bb5d
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     Added a
+  
diff --git a/hgext/remotenames.py b/hgext/remotenames.py
--- a/hgext/remotenames.py
+++ b/hgext/remotenames.py
@@ -18,6 +18,13 @@ 
 
 remotenames.branches
   Boolean value to enable or disable showing of remotebranches
+
+remotenames.pushtobookmark
+  Boolean value to change the behavior of bookmark passed to push command using
+  `-B` flag. If set the true, the changesets will be pushed to that bookmark on
+  the server. Errors if bookmark does not exists on the server. If multiple
+  bookmarks are specified using `-B` flag, fallbacks to default behavior.
+  (default: False)
 """
 
 from __future__ import absolute_import
@@ -28,8 +35,11 @@ 
 
 from mercurial.node import (
     bin,
+    hex,
 )
 from mercurial import (
+    error,
+    exchange,
     logexchange,
     namespaces,
     registrar,
@@ -55,6 +65,36 @@ 
 configitem('remotenames', 'branches',
     default=True,
 )
+configitem('remotenames', 'pushtobookmark',
+    default=False,
+)
+
+def extsetup(ui):
+    exchange.pushdiscoverymapping['bookmarks'] = expushdiscoverybookmarks
+
+def expushdiscoverybookmarks(pushop):
+    # config not set, fallback to normal push behavior
+    if not pushop.repo.ui.configbool('remotenames', 'pushtobookmark'):
+        return exchange._pushdiscoverybookmarks(pushop)
+
+    # either zero or more than one bookmarks specified, fallback to normal
+    # push behavior, maybe we should error out in case of multiple bookmarks
+    if len(pushop.bookmarks) != 1:
+        return exchange._pushdiscoverybookmarks(pushop)
+
+    remotemarks = pushop.remote.listkeys('bookmarks')
+    bookmark = pushop.bookmarks[0]
+    rev = pushop.revs[0]
+
+    # allow new bookmark only if --create is specified
+    old = ''
+    if bookmark in remotemarks:
+        old = remotemarks[bookmark]
+    else:
+        msg = _("bookmark '%s' does not exists on remote")
+        raise error.Abort(msg % bookmark)
+
+    pushop.outbookmarks.append((bookmark, old, hex(rev)))
 
 class lazyremotenamedict(collections.MutableMapping):
     """