Patchwork [4,of,4,evolve-ext] inhibit: Add -D option to the bookmark command

login
register
mail settings
Submitter Laurent Charignon
Date April 3, 2015, 2:09 a.m.
Message ID <d0184d43212ee581dc31.1428026998@lcharignon-mbp.dhcp.thefacebook.com>
Download mbox | patch
Permalink /patch/8467/
State Changes Requested
Headers show

Comments

Laurent Charignon - April 3, 2015, 2:09 a.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1427836666 25200
#      Tue Mar 31 14:17:46 2015 -0700
# Node ID d0184d43212ee581dc3163ebf4606fd1c62e7dc6
# Parent  efcfb550ae9a38972af97d25c841b1d1d132f499
inhibit: Add -D option to the bookmark command

The -D option for bookmark is similar to the -D option for strip.
It deletes the bookmark and prunes the changes underneath it that are
not reachable.
Sean Farley - April 3, 2015, 2:41 a.m.
Laurent Charignon <lcharignon@fb.com> writes:

> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1427836666 25200
> #      Tue Mar 31 14:17:46 2015 -0700
> # Node ID d0184d43212ee581dc3163ebf4606fd1c62e7dc6
> # Parent  efcfb550ae9a38972af97d25c841b1d1d132f499
> inhibit: Add -D option to the bookmark command
>
> The -D option for bookmark is similar to the -D option for strip.
> It deletes the bookmark and prunes the changes underneath it that are
> not reachable.

I'm guessing you mean 'The -D option for bookmark is similar to the -B
option for strip'?
Pierre-Yves David - April 3, 2015, 6:08 p.m.
On 04/02/2015 07:41 PM, Sean Farley wrote:
>
> Laurent Charignon <lcharignon@fb.com> writes:
>
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1427836666 25200
>> #      Tue Mar 31 14:17:46 2015 -0700
>> # Node ID d0184d43212ee581dc3163ebf4606fd1c62e7dc6
>> # Parent  efcfb550ae9a38972af97d25c841b1d1d132f499
>> inhibit: Add -D option to the bookmark command
>>
>> The -D option for bookmark is similar to the -D option for strip.
>> It deletes the bookmark and prunes the changes underneath it that are
>> not reachable.
>
> I'm guessing you mean 'The -D option for bookmark is similar to the -B
> option for strip'?

He does, nice catch, I updated it while getting the patch.
Jordi Gutiérrez Hermoso - April 3, 2015, 6:31 p.m.
On Thu, 2015-04-02 at 19:09 -0700, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1427836666 25200
> #      Tue Mar 31 14:17:46 2015 -0700
> # Node ID d0184d43212ee581dc3163ebf4606fd1c62e7dc6
> # Parent  efcfb550ae9a38972af97d25c841b1d1d132f499
> inhibit: Add -D option to the bookmark command
> 
> The -D option for bookmark is similar to the -D option for strip.
> It deletes the bookmark and prunes the changes underneath it that are
> not reachable.

Beg your pardon? There is no -D option for strip. It's -B, and both
strip and prune already have it.
Jordi Gutiérrez Hermoso - April 3, 2015, 6:33 p.m.
On Fri, 2015-04-03 at 11:08 -0700, Pierre-Yves David wrote:
> 
> On 04/02/2015 07:41 PM, Sean Farley wrote:
> >
> > Laurent Charignon <lcharignon@fb.com> writes:
> >
> >> # HG changeset patch
> >> # User Laurent Charignon <lcharignon@fb.com>
> >> # Date 1427836666 25200
> >> #      Tue Mar 31 14:17:46 2015 -0700
> >> # Node ID d0184d43212ee581dc3163ebf4606fd1c62e7dc6
> >> # Parent  efcfb550ae9a38972af97d25c841b1d1d132f499
> >> inhibit: Add -D option to the bookmark command
> >>
> >> The -D option for bookmark is similar to the -D option for strip.
> >> It deletes the bookmark and prunes the changes underneath it that are
> >> not reachable.
> >
> > I'm guessing you mean 'The -D option for bookmark is similar to the -B
> > option for strip'?
> 
> He does, nice catch, I updated it while getting the patch.

What? `hg book` doesn't have a -D option either, only a -d option.
Pierre-Yves David - April 3, 2015, 6:37 p.m.
On 04/03/2015 11:33 AM, Jordi Gutiérrez Hermoso wrote:
> On Fri, 2015-04-03 at 11:08 -0700, Pierre-Yves David wrote:
>>
>> On 04/02/2015 07:41 PM, Sean Farley wrote:
>>>
>>> Laurent Charignon <lcharignon@fb.com> writes:
>>>
>>>> # HG changeset patch
>>>> # User Laurent Charignon <lcharignon@fb.com>
>>>> # Date 1427836666 25200
>>>> #      Tue Mar 31 14:17:46 2015 -0700
>>>> # Node ID d0184d43212ee581dc3163ebf4606fd1c62e7dc6
>>>> # Parent  efcfb550ae9a38972af97d25c841b1d1d132f499
>>>> inhibit: Add -D option to the bookmark command
>>>>
>>>> The -D option for bookmark is similar to the -D option for strip.
>>>> It deletes the bookmark and prunes the changes underneath it that are
>>>> not reachable.
>>>
>>> I'm guessing you mean 'The -D option for bookmark is similar to the -B
>>> option for strip'?
>>
>> He does, nice catch, I updated it while getting the patch.
>
> What? `hg book` doesn't have a -D option either, only a -d option.

That's why this patch (in inhibit) is adding such option.
Pierre-Yves David - April 3, 2015, 6:56 p.m.
On 04/02/2015 07:09 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1427836666 25200
> #      Tue Mar 31 14:17:46 2015 -0700
> # Node ID d0184d43212ee581dc3163ebf4606fd1c62e7dc6
> # Parent  efcfb550ae9a38972af97d25c841b1d1d132f499
> inhibit: Add -D option to the bookmark command
>
> The -D option for bookmark is similar to the -D option for strip.
> It deletes the bookmark and prunes the changes underneath it that are
> not reachable.

This -D option seems to skip all working copy update. This means that 
`hg book -D current-bookmark` will not move the working copy, and 
therefor, keep all the changesets visible. This should probably be fixed 
and tested. This can (must?) go in a follow up patch.

> diff --git a/hgext/inhibit.py b/hgext/inhibit.py
> --- a/hgext/inhibit.py
> +++ b/hgext/inhibit.py
> @@ -31,6 +31,7 @@
>   from mercurial import error
>   from mercurial import commands
>   from mercurial import bookmarks
> +from mercurial.i18n import _
>
>   cmdtable = {}
>   command = cmdutil.command(cmdtable)
> @@ -74,6 +75,41 @@
>       _inhibitmarkers(repo, bkmstorenodes)
>       return orig(bkmstoreinst, *args, **kwargs)
>
> +def _bookmark(orig, ui, repo, *bookmarks, **opts):
> +    """
> +    Wraps bookmark to add the -D option to prune the commits underneath after
> +    deleting the bookmark.
> +    """
> +    haspruneopt = opts.get('prune', False)
> +
> +    if not haspruneopt:
> +        return orig(ui, repo, *bookmarks, **opts)
> +
> +    opts.pop('prune')
> +    lock = repo.lock()
> +    try:
> +        tr = repo.transaction("bookmark -D")
> +        try:
> +            # Get ancestors of the pruned bookmark not covered by another head
> +            # or bookmark
> +            prunedbkm = repo._bookmarks[bookmarks[0]]
> +            ancestors = set(repo.set('only(%s, (head() or bookmark())'+
> +                                        'and not %s)', prunedbkm, prunedbkm))

- You should put the revset string in a tmp variable for readability,
- The "+" is superfluous: ('foo' 'bar') will give you 'foobar'.

> +            # Call bookmark -d to delete the bookmark
> +            opts['delete'] = True
> +            orig(ui, repo, *bookmarks, **opts)
> +            # Prune the ancestors
> +            relations = [ (c,()) for c in ancestors ]
> +            obsolete.createmarkers(repo, relations)
> +
> +            tr.close()
> +        finally:
> +            tr.release()
> +    finally:
> +        lock.release()

See the `release` function in the `lock` module. It let your do

lock = tr = None
try:
   lock = …
   tr = …
   …
   tr.close()
finally:
   lockmod.release(tr, lock) # I'm unsure about the order)

> +
> +
> +
>   # obsolescence inhibitor
>   ########################
>
> @@ -184,6 +220,11 @@
>       # wrap both to add inhibition markers.
>       extensions.wrapfunction(bookmarks.bmstore, 'recordchange', _bookmarkchanged)
>       extensions.wrapfunction(bookmarks.bmstore, 'write', _bookmarkchanged)
> +    # Add bookmark -D option
> +    entry = extensions.wrapcommand(commands.table, 'bookmark', _bookmark)
> +    entry[1].append(('D','prune',None,
> +                    _('delete the bookmark and prune the commits underneath')))
> +
>
>
>   def gethashsymbols(tree):
> diff --git a/tests/test-inhibit.t b/tests/test-inhibit.t
> --- a/tests/test-inhibit.t
> +++ b/tests/test-inhibit.t
> @@ -250,9 +250,28 @@
>     |/
>     o  0:54ccbc537fc2 add cA
>
> -  $ hg bookmark -d book1
> -  $ hg prune --hidden 1::
> -  3 changesets pruned
> +
> +Removing a bookmark with bookmark -D should prune the changes underneath
> +that are not reachable from another bookmark or head
> +
> +  $ hg bookmark -r 1 book2
> +  $ hg bookmark -D book1
> +  $ hg log -G
> +  @  9:55c73a90e4b4 add cJ
> +  |
> +  | o  7:18214586bf78 add cJ
> +  |/
> +  o  6:cf5c4f4554ce add cH
> +  |
> +  o  5:5419eb264a33 add cG
> +  |
> +  o  4:98065434e5c6 add cE
> +  |
> +  | o  1:02bcbc3f6e56 add cB
> +  |/
> +  o  0:54ccbc537fc2 add cA
> +
> +  $ hg bookmark -D book2
>     $ hg log -G
>     @  9:55c73a90e4b4 add cJ
>     |
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/hgext/inhibit.py b/hgext/inhibit.py
--- a/hgext/inhibit.py
+++ b/hgext/inhibit.py
@@ -31,6 +31,7 @@ 
 from mercurial import error
 from mercurial import commands
 from mercurial import bookmarks
+from mercurial.i18n import _
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
@@ -74,6 +75,41 @@ 
     _inhibitmarkers(repo, bkmstorenodes)
     return orig(bkmstoreinst, *args, **kwargs)
 
+def _bookmark(orig, ui, repo, *bookmarks, **opts):
+    """
+    Wraps bookmark to add the -D option to prune the commits underneath after
+    deleting the bookmark.
+    """
+    haspruneopt = opts.get('prune', False)
+
+    if not haspruneopt:
+        return orig(ui, repo, *bookmarks, **opts)
+    
+    opts.pop('prune')
+    lock = repo.lock()
+    try:
+        tr = repo.transaction("bookmark -D")
+        try:
+            # Get ancestors of the pruned bookmark not covered by another head
+            # or bookmark
+            prunedbkm = repo._bookmarks[bookmarks[0]]
+            ancestors = set(repo.set('only(%s, (head() or bookmark())'+
+                                        'and not %s)', prunedbkm, prunedbkm))
+            # Call bookmark -d to delete the bookmark
+            opts['delete'] = True
+            orig(ui, repo, *bookmarks, **opts)
+            # Prune the ancestors
+            relations = [ (c,()) for c in ancestors ]
+            obsolete.createmarkers(repo, relations)
+
+            tr.close()
+        finally:
+            tr.release()
+    finally:
+        lock.release()
+
+   
+
 # obsolescence inhibitor
 ########################
 
@@ -184,6 +220,11 @@ 
     # wrap both to add inhibition markers.
     extensions.wrapfunction(bookmarks.bmstore, 'recordchange', _bookmarkchanged)
     extensions.wrapfunction(bookmarks.bmstore, 'write', _bookmarkchanged)
+    # Add bookmark -D option
+    entry = extensions.wrapcommand(commands.table, 'bookmark', _bookmark)
+    entry[1].append(('D','prune',None,
+                    _('delete the bookmark and prune the commits underneath')))
+
 
 
 def gethashsymbols(tree):
diff --git a/tests/test-inhibit.t b/tests/test-inhibit.t
--- a/tests/test-inhibit.t
+++ b/tests/test-inhibit.t
@@ -250,9 +250,28 @@ 
   |/
   o  0:54ccbc537fc2 add cA
   
-  $ hg bookmark -d book1
-  $ hg prune --hidden 1::
-  3 changesets pruned
+
+Removing a bookmark with bookmark -D should prune the changes underneath
+that are not reachable from another bookmark or head
+
+  $ hg bookmark -r 1 book2
+  $ hg bookmark -D book1
+  $ hg log -G
+  @  9:55c73a90e4b4 add cJ
+  |
+  | o  7:18214586bf78 add cJ
+  |/
+  o  6:cf5c4f4554ce add cH
+  |
+  o  5:5419eb264a33 add cG
+  |
+  o  4:98065434e5c6 add cE
+  |
+  | o  1:02bcbc3f6e56 add cB
+  |/
+  o  0:54ccbc537fc2 add cA
+  
+  $ hg bookmark -D book2
   $ hg log -G
   @  9:55c73a90e4b4 add cJ
   |