Patchwork push: move handling of explicitly pushed bookmarks into localrepo.push()

login
register
mail settings
Submitter Kevin Bullock
Date Dec. 7, 2012, 6:51 p.m.
Message ID <47f4b9b1667a8af4df75.1354906273@opendoor.mincava.umn.edu>
Download mbox | patch
Permalink /patch/28/
State Not Applicable
Headers show

Comments

Kevin Bullock - Dec. 7, 2012, 6:51 p.m.
# HG changeset patch
# User Kevin Bullock <kbullock at ringworld.org>
# Date 1354905590 21600
# Node ID 47f4b9b1667a8af4df75fce5d2af8d12160f3f0b
# Parent  f3991bcf4f0ff43b43a1b1d0210925a629ef3b9c
push: move handling of explicitly pushed bookmarks into localrepo.push()

This puts the handling of bookmarks pushed with -B/--bookmark in the
same place as already-shared bookmarks.

As a side effect, the undocumented exit code 2 for push (indicating
that bookmarks explicitly listed couldn't be synched) is eliminated.
Augie Fackler - Dec. 7, 2012, 7:19 p.m.
On Dec 7, 2012, at 12:51 PM, Kevin Bullock <kbullock+mercurial at ringworld.org> wrote:

> As a side effect, the undocumented exit code 2 for push (indicating
> that bookmarks explicitly listed couldn't be synched) is eliminated.


It's undocumented, but I wonder if this would break people. Seems like that should be done as a separate change with (bc) in the summary line?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121207/95f7a916/attachment.html>
Kevin Bullock - Dec. 7, 2012, 7:22 p.m.
On Dec 7, 2012, at 1:19 PM, Augie Fackler wrote:

> On Dec 7, 2012, at 12:51 PM, Kevin Bullock <kbullock+mercurial at ringworld.org> wrote:
> 
>> As a side effect, the undocumented exit code 2 for push (indicating
>> that bookmarks explicitly listed couldn't be synched) is eliminated.
> 
> It's undocumented, but I wonder if this would break people. Seems like that should be done as a separate change with (bc) in the summary line?

You're right, should probably be changed before moving the code into localrepo. I also missed some test breakage (largefiles, test-hook), so I'll resend in a minute.

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Matt Mackall - Dec. 7, 2012, 7:39 p.m.
On Fri, 2012-12-07 at 12:51 -0600, Kevin Bullock wrote:
> # HG changeset patch
> # User Kevin Bullock <kbullock at ringworld.org>
> # Date 1354905590 21600
> # Node ID 47f4b9b1667a8af4df75fce5d2af8d12160f3f0b
> # Parent  f3991bcf4f0ff43b43a1b1d0210925a629ef3b9c
> push: move handling of explicitly pushed bookmarks into localrepo.push()
> 
> This puts the handling of bookmarks pushed with -B/--bookmark in the
> same place as already-shared bookmarks.
> 
> As a side effect, the undocumented exit code 2 for push (indicating
> that bookmarks explicitly listed couldn't be synched) is eliminated.

The docs for push say:

    Returns 0 if push was successful, 1 if nothing to push.

Most of the time, we say something like:

    Returns 0 if successful.

This implies by omission that all other exit codes are some sort of
failure. Most of these are 127/255 (abort, depending on platform), but
sometimes 1 is used. But here (and a few other places, like grep) 1 is a
non-error exit code. 2 is indeed a bit unusual here, but still falls
into the category of not-documented-as-success.

>    bookmark badname does not exist on the local or remote repository!
> -  [2]
> +  [1]

That's one failure mode. And indeed, "nothing to push" makes some sense
here. But it probably still warrants a failure, as you asked Mercurial
to push a specific thing that didn't exist.

But there's also this one:

            if not r:
                ui.warn(_('updating bookmark %s failed!\n') % b)
                if not result:
                    result = 2

This will generally be caused by divergent bookmarks or lack of remote
bookmark support. And that's definitely more of a failure than "nothing
to push".

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4766,33 +4766,9 @@  def push(ui, repo, dest=None, **opts):
                 return False
     finally:
         del repo._subtoppath
-    result = repo.push(other, opts.get('force'), revs=revs,
-                       newbranch=opts.get('new_branch'))
-
-    result = not result
-
-    if opts.get('bookmark'):
-        rb = other.listkeys('bookmarks')
-        for b in opts['bookmark']:
-            # explicit push overrides remote bookmark if any
-            if b in repo._bookmarks:
-                ui.status(_("exporting bookmark %s\n") % b)
-                new = repo[b].hex()
-            elif b in rb:
-                ui.status(_("deleting remote bookmark %s\n") % b)
-                new = '' # delete
-            else:
-                ui.warn(_('bookmark %s does not exist on the local '
-                          'or remote repository!\n') % b)
-                return 2
-            old = rb.get(b, '')
-            r = other.pushkey('bookmarks', b, old, new)
-            if not r:
-                ui.warn(_('updating bookmark %s failed!\n') % b)
-                if not result:
-                    result = 2
-
-    return result
+    return not repo.push(other, opts.get('force'), revs=revs,
+                         newbranch=opts.get('new_branch'),
+                         marks=opts.get('bookmark'))
 
 @command('recover', [])
 def recover(ui, repo):
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1866,7 +1866,7 @@  class localrepository(object):
         """
         pass
 
-    def push(self, remote, force=False, revs=None, newbranch=False):
+    def push(self, remote, force=False, revs=None, newbranch=False, marks=[]):
         '''Push outgoing changesets (limited by revs) from the current
         repository to remote. Return an integer:
           - None means nothing to push
@@ -2049,6 +2049,22 @@  class localrepository(object):
                         else:
                             self.ui.warn(_('updating bookmark %s'
                                            ' failed!\n') % k)
+        for b in marks:
+            # explicit push overrides remote bookmark if any
+            if b in unfi._bookmarks:
+                self.ui.status(_("exporting bookmark %s\n") % b)
+                new = self[b].hex()
+            elif b in rb:
+                self.ui.status(_("deleting remote bookmark %s\n") % b)
+                new = '' # delete
+            else:
+                self.ui.warn(_('bookmark %s does not exist on the local '
+                          'or remote repository!\n') % b)
+                break
+            old = rb.get(b, '')
+            r = remote.pushkey('bookmarks', b, old, new)
+            if not r:
+                self.ui.warn(_('updating bookmark %s failed!\n') % b)
 
         return ret
 
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
@@ -101,7 +101,7 @@  push/pull name that doesn't exist
   searching for changes
   no changes found
   bookmark badname does not exist on the local or remote repository!
-  [2]
+  [1]
   $ hg pull -B anotherbadname ../a
   pulling from ../a
   abort: remote bookmark anotherbadname not found!