Patchwork [1,of,2,RFC,RESEND] bookmarks: allow push to create a new remote head if it is about to be bookmarked via -B

login
register
mail settings
Submitter Stephen Lee
Date Sept. 8, 2013, 10:45 a.m.
Message ID <4f3b94d13ecd9d35b1c7.1378637108@slee-desktop>
Download mbox | patch
Permalink /patch/2412/
State Superseded, archived
Headers show

Comments

Stephen Lee - Sept. 8, 2013, 10:45 a.m.
# HG changeset patch
# User Stephen Lee <sphen.lee@gmail.com>
# Date 1378636805 -36000
# Node ID 4f3b94d13ecd9d35b1c7bd65e57f8700322d2816
# Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
bookmarks: allow push to create a new remote head if it is about to be bookmarked via -B

Push is currently allowed to create a new head if there is a remote bookmark that will
be updated to point to the new head.  If the bookmark is not known remotely then
push aborts, even if a -B argument is about to push the bookmark. This change allows
push to continue in this case.  This does not require a wireproto force.

This is an RFC change.  The list of new bookmarks is stored in config to avoid changing
the signature of localrepo.push which is wrapped by many extensions (both bundled and external).
A real solution would need to find a cleaner way to do this.
Katsunori FUJIWARA - Sept. 9, 2013, 10:39 a.m.
This comment is not for patch itself, but the policy of pushing
revisions in subrepositories should be clarified before discussing
about this patch: at least for me :-)


AFAIK, current implementations of each subrepo classes don't support
partial pushing. "--new-branch" option is passed to
"localreposiory.push()" in "hgsubrepo.push()", but none of information
specified by "--rev" (or by "--branch"/"--bookmark" indirectly) is.

So, pushing without "--force" in parent repo fails, if revisions in
subrepos cause new remote heads, even if "--rev" or such options for
partial pushing are specified to limit revisions to be pushed on
parent repo.

(1) Should subrepo support partial pushing ?

  - pros:

    - users can push revisions in subrepos partially in easy way from
      parent repo

      users shouldn't specify "--force", if revisions in subrepos
      implied by ones to be pushed in parent don't cause new remote
      heads, even if there are any other revisions causing new remote
      heads in subrepos.

      "picking revisions in subrepos referred from ones to be pushed
      in parent up" is not for human work but for computer's one.

  - cons:

    - cost of parent side

      picking revisions in subrepos referred from ones to be pushed in
      parent up may cost much.

      all filelog entries for ".hgsubstate" referred from revisions to
      be pushed in parent should be scanned, to get referred revisions
      in subrepo.

      (is it enough for efficiency to avoid such scanning, when none
      of the "--rev" like options are specified ?)

    - cost of subrepo side

      pushing own revisions itself may not cost so much (maybe), if
      "onlyheads" like information is passed from parent.

      but "picking revisions in subrepos referred ..." is needed for
      recursive pushing.

      should gitsubrepo support partial pushing ? is it easy to
      implement ? (sorry, I'm not familiar with git)

(2) in typical usecase of pushing new remote head by "-B foo" without
    "--force", is it reasonable to assume that revisions to be pushed
    in subrepos doesn't cause new remote heads, or not ?

  - if it is reasonable:

    we can allow "-B foo" causing new remote head to push revisions
    without "--force"

  - otherwise:

    we should support partial pushing for subrepos, if we allow such
    pushing


At Sun, 08 Sep 2013 20:45:08 +1000,
Stephen Lee wrote:
> 
> # HG changeset patch
> # User Stephen Lee <sphen.lee@gmail.com>
> # Date 1378636805 -36000
> # Node ID 4f3b94d13ecd9d35b1c7bd65e57f8700322d2816
> # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> bookmarks: allow push to create a new remote head if it is about to be bookmarked via -B
> 
> Push is currently allowed to create a new head if there is a remote bookmark that will
> be updated to point to the new head.  If the bookmark is not known remotely then
> push aborts, even if a -B argument is about to push the bookmark. This change allows
> push to continue in this case.  This does not require a wireproto force.
> 
> This is an RFC change.  The list of new bookmarks is stored in config to avoid changing
> the signature of localrepo.push which is wrapped by many extensions (both bundled and external).
> A real solution would need to find a cleaner way to do this.
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4664,6 +4664,7 @@
>      """
>  
>      if opts.get('bookmark'):
> +        ui.setconfig('bookmarks', 'pushing', opts['bookmark'])
>          for b in opts['bookmark']:
>              # translate -B options to -r so changesets get pushed
>              if b in repo._bookmarks:
> diff --git a/mercurial/discovery.py b/mercurial/discovery.py
> --- a/mercurial/discovery.py
> +++ b/mercurial/discovery.py
> @@ -219,7 +219,8 @@
>      unsynced = inc and set([None]) or set()
>      return {None: (oldheads, newheads, unsynced)}
>  
> -def checkheads(repo, remote, outgoing, remoteheads, newbranch=False, inc=False):
> +def checkheads(repo, remote, outgoing, remoteheads, newbranch=False, inc=False,
> +               newbookmarks=[]):
>      """Check that a push won't add any outgoing head
>  
>      raise Abort error and display ui message as needed.
> @@ -259,6 +260,9 @@
>              lctx, rctx = repo[bm], repo[rnode]
>              if bookmarks.validdest(repo, rctx, lctx):
>                  bookmarkedheads.add(lctx.node())
> +        else:
> +            if bm in newbookmarks:
> +                bookmarkedheads.add(repo[bm].node())
>  
>      # 3. Check for new heads.
>      # If there are more heads after the push than before, a suitable
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1830,9 +1830,10 @@
>                                      raise util.Abort(_(mst)
>                                                       % (ctx.troubles()[0],
>                                                          ctx))
> +                        newbm = self.ui.configlist('bookmarks', 'pushing')
>                          discovery.checkheads(unfi, remote, outgoing,
>                                               remoteheads, newbranch,
> -                                             bool(inc))
> +                                             bool(inc), newbm)
>  
>                      # TODO: get bundlecaps from remote
>                      bundlecaps = None
> diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
> --- a/tests/test-bookmarks-pushpull.t
> +++ b/tests/test-bookmarks-pushpull.t
> @@ -424,4 +424,23 @@
>    remote: added 1 changesets with 1 changes to 1 files
>    exporting bookmark add-foo
>  
> +pushing a new bookmark on a new head does not require -f if -B is specified
> +
> +  $ hg up -q X
> +  $ hg book W
> +  $ echo c5 > f2
> +  $ hg ci -Am5
> +  created new head
> +  $ hg push -B W
> +  pushing to http://localhost:$HGPORT/
> +  searching for changes
> +  remote: adding changesets
> +  remote: adding manifests
> +  remote: adding file changes
> +  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
> +  updating bookmark @ failed!
> +  exporting bookmark W
> +  $ hg -R ../b id -r W
> +  cc978a373a53 tip W
> +
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - Sept. 9, 2013, 10:39 a.m.
This comment is not for patch itself, but the policy of pushing
revisions in subrepositories should be clarified before discussing
about this patch: at least for me :-)


AFAIK, current implementations of each subrepo classes don't support
partial pushing. "--new-branch" option is passed to
"localreposiory.push()" in "hgsubrepo.push()", but none of information
specified by "--rev" (or by "--branch"/"--bookmark" indirectly) is.

So, pushing without "--force" in parent repo fails, if revisions in
subrepos cause new remote heads, even if "--rev" or such options for
partial pushing are specified to limit revisions to be pushed on
parent repo.

(1) Should subrepo support partial pushing ?

  - pros:

    - users can push revisions in subrepos partially in easy way from
      parent repo

      users shouldn't specify "--force", if revisions in subrepos
      implied by ones to be pushed in parent don't cause new remote
      heads, even if there are any other revisions causing new remote
      heads in subrepos.

      "picking revisions in subrepos referred from ones to be pushed
      in parent up" is not for human work but for computer's one.

  - cons:

    - cost of parent side

      picking revisions in subrepos referred from ones to be pushed in
      parent up may cost much.

      all filelog entries for ".hgsubstate" referred from revisions to
      be pushed in parent should be scanned, to get referred revisions
      in subrepo.

      (is it enough for efficiency to avoid such scanning, when none
      of the "--rev" like options are specified ?)

    - cost of subrepo side

      pushing own revisions itself may not cost so much (maybe), if
      "onlyheads" like information is passed from parent.

      but "picking revisions in subrepos referred ..." is needed for
      recursive pushing.

      should gitsubrepo support partial pushing ? is it easy to
      implement ? (sorry, I'm not familiar with git)

(2) in typical usecase of pushing new remote head by "-B foo" without
    "--force", is it reasonable to assume that revisions to be pushed
    in subrepos doesn't cause new remote heads, or not ?

  - if it is reasonable:

    we can allow "-B foo" causing new remote head to push revisions
    without "--force"

  - otherwise:

    we should support partial pushing for subrepos, if we allow such
    pushing


At Sun, 08 Sep 2013 20:45:08 +1000,
Stephen Lee wrote:
> 
> # HG changeset patch
> # User Stephen Lee <sphen.lee@gmail.com>
> # Date 1378636805 -36000
> # Node ID 4f3b94d13ecd9d35b1c7bd65e57f8700322d2816
> # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> bookmarks: allow push to create a new remote head if it is about to be bookmarked via -B
> 
> Push is currently allowed to create a new head if there is a remote bookmark that will
> be updated to point to the new head.  If the bookmark is not known remotely then
> push aborts, even if a -B argument is about to push the bookmark. This change allows
> push to continue in this case.  This does not require a wireproto force.
> 
> This is an RFC change.  The list of new bookmarks is stored in config to avoid changing
> the signature of localrepo.push which is wrapped by many extensions (both bundled and external).
> A real solution would need to find a cleaner way to do this.
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4664,6 +4664,7 @@
>      """
>  
>      if opts.get('bookmark'):
> +        ui.setconfig('bookmarks', 'pushing', opts['bookmark'])
>          for b in opts['bookmark']:
>              # translate -B options to -r so changesets get pushed
>              if b in repo._bookmarks:
> diff --git a/mercurial/discovery.py b/mercurial/discovery.py
> --- a/mercurial/discovery.py
> +++ b/mercurial/discovery.py
> @@ -219,7 +219,8 @@
>      unsynced = inc and set([None]) or set()
>      return {None: (oldheads, newheads, unsynced)}
>  
> -def checkheads(repo, remote, outgoing, remoteheads, newbranch=False, inc=False):
> +def checkheads(repo, remote, outgoing, remoteheads, newbranch=False, inc=False,
> +               newbookmarks=[]):
>      """Check that a push won't add any outgoing head
>  
>      raise Abort error and display ui message as needed.
> @@ -259,6 +260,9 @@
>              lctx, rctx = repo[bm], repo[rnode]
>              if bookmarks.validdest(repo, rctx, lctx):
>                  bookmarkedheads.add(lctx.node())
> +        else:
> +            if bm in newbookmarks:
> +                bookmarkedheads.add(repo[bm].node())
>  
>      # 3. Check for new heads.
>      # If there are more heads after the push than before, a suitable
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1830,9 +1830,10 @@
>                                      raise util.Abort(_(mst)
>                                                       % (ctx.troubles()[0],
>                                                          ctx))
> +                        newbm = self.ui.configlist('bookmarks', 'pushing')
>                          discovery.checkheads(unfi, remote, outgoing,
>                                               remoteheads, newbranch,
> -                                             bool(inc))
> +                                             bool(inc), newbm)
>  
>                      # TODO: get bundlecaps from remote
>                      bundlecaps = None
> diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
> --- a/tests/test-bookmarks-pushpull.t
> +++ b/tests/test-bookmarks-pushpull.t
> @@ -424,4 +424,23 @@
>    remote: added 1 changesets with 1 changes to 1 files
>    exporting bookmark add-foo
>  
> +pushing a new bookmark on a new head does not require -f if -B is specified
> +
> +  $ hg up -q X
> +  $ hg book W
> +  $ echo c5 > f2
> +  $ hg ci -Am5
> +  created new head
> +  $ hg push -B W
> +  pushing to http://localhost:$HGPORT/
> +  searching for changes
> +  remote: adding changesets
> +  remote: adding manifests
> +  remote: adding file changes
> +  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
> +  updating bookmark @ failed!
> +  exporting bookmark W
> +  $ hg -R ../b id -r W
> +  cc978a373a53 tip W
> +
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Martin Geisler - Sept. 10, 2013, 7:15 a.m.
FUJIWARA Katsunori <foozy@lares.dti.ne.jp> writes:

> This comment is not for patch itself, but the policy of pushing
> revisions in subrepositories should be clarified before discussing
> about this patch: at least for me :-)
>
> AFAIK, current implementations of each subrepo classes don't support
> partial pushing. "--new-branch" option is passed to
> "localreposiory.push()" in "hgsubrepo.push()", but none of information
> specified by "--rev" (or by "--branch"/"--bookmark" indirectly) is.
>
> So, pushing without "--force" in parent repo fails, if revisions in
> subrepos cause new remote heads, even if "--rev" or such options for
> partial pushing are specified to limit revisions to be pushed on
> parent repo.

I would prefer to delay this subrepo discussion until after the patch is
in since I feel the patch is important and will help the 95% of our
users who don't use subrepositories.

I think that is okay to do since the patch is simply enabling more
pushes to go through without --force. Enabling the remaining cases
(where it makes sense) can be done later.

In table form, I believe the situation today is:

  push   | new heads in | push
  flags  | parent | sub | result
  ==============================
  -B foo | no     | no  | ok
  -B foo | yes    | no  | abort
  -B foo | no     | yes | abort
  -B foo | yes    | yes | abort

That is, new heads in either parent or sub will abort the push. One can
then add --force to make the aborted push go through.

Changing -B foo to allow creating the a new head with bookmark foo would
result in:

  push   | new heads in | push
  flags  | parent | sub | result
  ==============================
  -B foo | no     | no  | ok
  -B foo | yes    | no  | ok
  -B foo | no     | yes | abort
  -B foo | yes    | yes | abort

One can still add --force to turn the remaining two aborts into ok.
Matt Mackall - Oct. 19, 2013, 10:43 p.m.
On Sat, 2013-09-21 at 22:51 -0500, Kevin Bullock wrote:
> On 8 Sep 2013, at 5:45 AM, Stephen Lee wrote:
> 
> > # HG changeset patch
> > # User Stephen Lee <sphen.lee@gmail.com>
> > # Date 1378636805 -36000
> > # Node ID 4f3b94d13ecd9d35b1c7bd65e57f8700322d2816
> > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > bookmarks: allow push to create a new remote head if it is about to be bookmarked via -B
> > 
> > Push is currently allowed to create a new head if there is a remote bookmark that will
> > be updated to point to the new head.  If the bookmark is not known remotely then
> > push aborts, even if a -B argument is about to push the bookmark. This change allows
> > push to continue in this case.  This does not require a wireproto force.
> > 
> > This is an RFC change.  The list of new bookmarks is stored in config to avoid changing
> > the signature of localrepo.push which is wrapped by many extensions (both bundled and external).
> > A real solution would need to find a cleaner way to do this.
> 
> Changing the signature of localrepo.push is fine across major releases. I'd like to see us do this the right way. I'm now convinced that it can be done while minimizing the chance for clobberage.
> 
> But! I think to do anything without driving ourselves completely mad in code, we have to get to that bookmarks-exchange refactoring that we've so badly needed. We already have a problem where we can push a bookmark once in localrepo.push and again in commands.py (if you push a bookmark with -B that already exists remotely, IIRC).
> 
> Thanks for your patience and perseverence on this. Maybe some
> combination of you, Augie, Sean, and I can work a bit more
> synchronously on a plan to attack this soon?

This probably collides with Fujiwara's series, which has been in flight
for a while as well. Looks like post-2.8 material.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4664,6 +4664,7 @@ 
     """
 
     if opts.get('bookmark'):
+        ui.setconfig('bookmarks', 'pushing', opts['bookmark'])
         for b in opts['bookmark']:
             # translate -B options to -r so changesets get pushed
             if b in repo._bookmarks:
diff --git a/mercurial/discovery.py b/mercurial/discovery.py
--- a/mercurial/discovery.py
+++ b/mercurial/discovery.py
@@ -219,7 +219,8 @@ 
     unsynced = inc and set([None]) or set()
     return {None: (oldheads, newheads, unsynced)}
 
-def checkheads(repo, remote, outgoing, remoteheads, newbranch=False, inc=False):
+def checkheads(repo, remote, outgoing, remoteheads, newbranch=False, inc=False,
+               newbookmarks=[]):
     """Check that a push won't add any outgoing head
 
     raise Abort error and display ui message as needed.
@@ -259,6 +260,9 @@ 
             lctx, rctx = repo[bm], repo[rnode]
             if bookmarks.validdest(repo, rctx, lctx):
                 bookmarkedheads.add(lctx.node())
+        else:
+            if bm in newbookmarks:
+                bookmarkedheads.add(repo[bm].node())
 
     # 3. Check for new heads.
     # If there are more heads after the push than before, a suitable
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1830,9 +1830,10 @@ 
                                     raise util.Abort(_(mst)
                                                      % (ctx.troubles()[0],
                                                         ctx))
+                        newbm = self.ui.configlist('bookmarks', 'pushing')
                         discovery.checkheads(unfi, remote, outgoing,
                                              remoteheads, newbranch,
-                                             bool(inc))
+                                             bool(inc), newbm)
 
                     # TODO: get bundlecaps from remote
                     bundlecaps = None
diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t
+++ b/tests/test-bookmarks-pushpull.t
@@ -424,4 +424,23 @@ 
   remote: added 1 changesets with 1 changes to 1 files
   exporting bookmark add-foo
 
+pushing a new bookmark on a new head does not require -f if -B is specified
+
+  $ hg up -q X
+  $ hg book W
+  $ echo c5 > f2
+  $ hg ci -Am5
+  created new head
+  $ hg push -B W
+  pushing to http://localhost:$HGPORT/
+  searching for changes
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
+  updating bookmark @ failed!
+  exporting bookmark W
+  $ hg -R ../b id -r W
+  cc978a373a53 tip W
+
   $ cd ..