Patchwork [STABLE] strip: set current bookmark to None if stripped

login
register
mail settings
Submitter Sean Farley
Date Sept. 17, 2013, 8:18 p.m.
Message ID <f33dec43e9afbdf966ab.1379449097@laptop.local>
Download mbox | patch
Permalink /patch/2512/
State Accepted
Headers show

Comments

Sean Farley - Sept. 17, 2013, 8:18 p.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1378584430 18000
#      Sat Sep 07 15:07:10 2013 -0500
# Branch stable
# Node ID f33dec43e9afbdf966abbb1a290fdc38efb09d59
# Parent  fd4f612f7cb6413940d4cf2052334cd23f60e042
strip: set current bookmark to None if stripped
Matt Mackall - Sept. 18, 2013, 8:02 p.m.
On Tue, 2013-09-17 at 15:18 -0500, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1378584430 18000
> #      Sat Sep 07 15:07:10 2013 -0500
> # Branch stable
> # Node ID f33dec43e9afbdf966abbb1a290fdc38efb09d59
> # Parent  fd4f612f7cb6413940d4cf2052334cd23f60e042
> strip: set current bookmark to None if stripped

Looks good, queued for stable.
Augie Fackler - Sept. 19, 2013, 2:33 p.m.
On Tue, Sep 17, 2013 at 03:18:17PM -0500, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1378584430 18000
> #      Sat Sep 07 15:07:10 2013 -0500
> # Branch stable
> # Node ID f33dec43e9afbdf966abbb1a290fdc38efb09d59
> # Parent  fd4f612f7cb6413940d4cf2052334cd23f60e042
> strip: set current bookmark to None if stripped

I know this is already queued, but I'm a bit confused. The bookmark rewinds its position, why would we deactivate it? Eg, if I had this graph:


@  20007[tip][ssl-stable-rollbacks]   6d315e354a06   2013-09-18 14:45 -0400   raf
|    httpclient: apply upstream revision da7579b034a4 to fix SSL problems (issue4038)
|
o  20006:19977   df135218860d   2013-09-18 14:40 -0400   raf
|    sslutil: backed out changeset 074bd02352c0 (issue4038)
|
o  19977[crew/stable,firefly/stable,mpm/stable,queue/stable]:19975   fd4f612f7cb6   2013-09-07 21:20 +0200   solipsis
|    bundle: fix performance regression when bundling file changes (issue4031)
|
o  19975:19918   26ddce1a2a55   2013-08-06 00:52 +0400   alexander
|    revset: fix wrong keyword() behaviour for strings with spaces
|

I'm on 20007, and if I do 'hg strip .', I expect to end up with 20006
as my wc parent with the bookmark pointing there as well and *still
active*. I've been doing this for a while and it's behavior I rely on
occasionally.

Can you convince me this new behavior is correct?

>
> diff --git a/hgext/mq.py b/hgext/mq.py
> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -61,11 +61,11 @@
>
>  from mercurial.i18n import _
>  from mercurial.node import bin, hex, short, nullid, nullrev
>  from mercurial.lock import release
>  from mercurial import commands, cmdutil, hg, scmutil, util, revset
> -from mercurial import repair, extensions, error, phases
> +from mercurial import repair, extensions, error, phases, bookmarks
>  from mercurial import patch as patchmod
>  from mercurial import localrepo
>  import os, re, errno, shutil
>
>  commands.norepo += " qclone"
> @@ -3059,10 +3059,12 @@
>              update = False
>          finally:
>              wlock.release()
>
>      if opts.get('bookmark'):
> +        if mark == repo._bookmarkcurrent:
> +            bookmarks.setcurrent(repo, None)
>          del marks[mark]
>          marks.write()
>          ui.write(_("bookmark '%s' deleted\n") % mark)
>
>      repo.mq.strip(repo, revs, backup=backup, update=update,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Sean Farley - Sept. 19, 2013, 5:24 p.m.
raf@durin42.com writes:

> On Tue, Sep 17, 2013 at 03:18:17PM -0500, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1378584430 18000
>> #      Sat Sep 07 15:07:10 2013 -0500
>> # Branch stable
>> # Node ID f33dec43e9afbdf966abbb1a290fdc38efb09d59
>> # Parent  fd4f612f7cb6413940d4cf2052334cd23f60e042
>> strip: set current bookmark to None if stripped
>
> I know this is already queued, but I'm a bit confused. The bookmark rewinds its position, why would we deactivate it? Eg, if I had this graph:
>
>
> @  20007[tip][ssl-stable-rollbacks]   6d315e354a06   2013-09-18 14:45 -0400   raf
> |    httpclient: apply upstream revision da7579b034a4 to fix SSL problems (issue4038)
> |
> o  20006:19977   df135218860d   2013-09-18 14:40 -0400   raf
> |    sslutil: backed out changeset 074bd02352c0 (issue4038)
> |
> o  19977[crew/stable,firefly/stable,mpm/stable,queue/stable]:19975   fd4f612f7cb6   2013-09-07 21:20 +0200   solipsis
> |    bundle: fix performance regression when bundling file changes (issue4031)
> |
> o  19975:19918   26ddce1a2a55   2013-08-06 00:52 +0400   alexander
> |    revset: fix wrong keyword() behaviour for strings with spaces
> |
>
> I'm on 20007, and if I do 'hg strip .', I expect to end up with 20006
> as my wc parent with the bookmark pointing there as well and *still
> active*. I've been doing this for a while and it's behavior I rely on
> occasionally.
>
> Can you convince me this new behavior is correct?

If you look at the code path around the patch, you'll see that it is
only setting the bookmark to None when we're already deleting the
bookmark. See line mq.py:3084: 'del marks[mark]'. This would be a great
case for unifying all these bookmark operations into one object so we're
not left with stragglers strewn about.

Your use case above is what I also expect and what should still happen.

Patch

diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -61,11 +61,11 @@ 
 
 from mercurial.i18n import _
 from mercurial.node import bin, hex, short, nullid, nullrev
 from mercurial.lock import release
 from mercurial import commands, cmdutil, hg, scmutil, util, revset
-from mercurial import repair, extensions, error, phases
+from mercurial import repair, extensions, error, phases, bookmarks
 from mercurial import patch as patchmod
 from mercurial import localrepo
 import os, re, errno, shutil
 
 commands.norepo += " qclone"
@@ -3059,10 +3059,12 @@ 
             update = False
         finally:
             wlock.release()
 
     if opts.get('bookmark'):
+        if mark == repo._bookmarkcurrent:
+            bookmarks.setcurrent(repo, None)
         del marks[mark]
         marks.write()
         ui.write(_("bookmark '%s' deleted\n") % mark)
 
     repo.mq.strip(repo, revs, backup=backup, update=update,