Patchwork [3,of,9,bm-refactor] bookmarks: factor out delete logic from commands

login
register
mail settings
Submitter Sean Farley
Date June 21, 2017, 12:29 a.m.
Message ID <9f36f5280f7d2a0230ef.1498004966@1.0.0.127.in-addr.arpa>
Download mbox | patch
Permalink /patch/21556/
State Accepted
Headers show

Comments

Sean Farley - June 21, 2017, 12:29 a.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1497333768 25200
#      Mon Jun 12 23:02:48 2017 -0700
# Branch bm-refactor
# Node ID 9f36f5280f7d2a0230efe1fd1dcf1a1d57f56ae8
# Parent  92a9b407c3b96df20e12a2076578bdc664600e09
bookmarks: factor out delete logic from commands

While we're here, let's use fancy context managers. I believe this
should still work since our locks are re-entrant.
Gregory Szorc - June 21, 2017, 2:50 a.m.
On Tue, Jun 20, 2017 at 5:29 PM, Sean Farley <sean@farley.io> wrote:

> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1497333768 25200
> #      Mon Jun 12 23:02:48 2017 -0700
> # Branch bm-refactor
> # Node ID 9f36f5280f7d2a0230efe1fd1dcf1a1d57f56ae8
> # Parent  92a9b407c3b96df20e12a2076578bdc664600e09
> bookmarks: factor out delete logic from commands
>
> While we're here, let's use fancy context managers. I believe this
> should still work since our locks are re-entrant.
>
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> index 451c557..c344cfc 100644
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -689,5 +689,21 @@ def checkformat(repo, mark):
>      if not mark:
>          raise error.Abort(_("bookmark names cannot consist entirely of "
>                              "whitespace"))
>      scmutil.checknewlabel(repo, mark, 'bookmark')
>      return mark
> +
> +def delete(repo, names):
> +    """remove a mark from the bookmark store
> +
> +    Raises an abort error if mark does not exist.
> +    """
> +    with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>

I stopped reviewing at this patch because I'm unsure of this pattern of
obtaining locks and a transaction in such a low-level function. Yes, things
are re-entrant and functionally it should work. But it doesn't seem right
to me. Passing in a transaction instance that can be reused with other
bookmark modifications feels better. I'm just not sure if we have a soft
rule around this, so I'm deferring review.


> +        marks = repo._bookmarks
> +        for mark in names:
> +            if mark not in marks:
> +                raise error.Abort(_("bookmark '%s' does not exist") %
> +                                  mark)
> +            if mark == repo._activebookmark:
> +                deactivate(repo)
> +            del marks[mark]
> +        marks.recordchange(tr)
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> index 4dd2521..d017f0a 100644
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -972,19 +972,11 @@ def bookmark(ui, repo, *names, **opts):
>              wlock = repo.wlock()
>              lock = repo.lock()
>              cur = repo.changectx('.').node()
>              marks = repo._bookmarks
>              if delete:
> -                tr = repo.transaction('bookmark')
> -                for mark in names:
> -                    if mark not in marks:
> -                        raise error.Abort(_("bookmark '%s' does not
> exist") %
> -                                         mark)
> -                    if mark == repo._activebookmark:
> -                        bookmarks.deactivate(repo)
> -                    del marks[mark]
> -
> +                bookmarks.delete(repo, names)
>              elif rename:
>                  tr = repo.transaction('bookmark')
>                  if not names:
>                      raise error.Abort(_("new bookmark name required"))
>                  elif len(names) > 1:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Sean Farley - June 21, 2017, 4:05 a.m.
Gregory Szorc <gregory.szorc@gmail.com> writes:

> On Tue, Jun 20, 2017 at 5:29 PM, Sean Farley <sean@farley.io> wrote:
>
>> # HG changeset patch
>> # User Sean Farley <sean@farley.io>
>> # Date 1497333768 25200
>> #      Mon Jun 12 23:02:48 2017 -0700
>> # Branch bm-refactor
>> # Node ID 9f36f5280f7d2a0230efe1fd1dcf1a1d57f56ae8
>> # Parent  92a9b407c3b96df20e12a2076578bdc664600e09
>> bookmarks: factor out delete logic from commands
>>
>> While we're here, let's use fancy context managers. I believe this
>> should still work since our locks are re-entrant.
>>
>> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
>> index 451c557..c344cfc 100644
>> --- a/mercurial/bookmarks.py
>> +++ b/mercurial/bookmarks.py
>> @@ -689,5 +689,21 @@ def checkformat(repo, mark):
>>      if not mark:
>>          raise error.Abort(_("bookmark names cannot consist entirely of "
>>                              "whitespace"))
>>      scmutil.checknewlabel(repo, mark, 'bookmark')
>>      return mark
>> +
>> +def delete(repo, names):
>> +    """remove a mark from the bookmark store
>> +
>> +    Raises an abort error if mark does not exist.
>> +    """
>> +    with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>>
>
> I stopped reviewing at this patch because I'm unsure of this pattern of
> obtaining locks and a transaction in such a low-level function. Yes, things
> are re-entrant and functionally it should work. But it doesn't seem right
> to me. Passing in a transaction instance that can be reused with other
> bookmark modifications feels better. I'm just not sure if we have a soft
> rule around this, so I'm deferring review.

If you mean that commands.py still has a lock / try / finally, then my
response is that all that is removed by the end of this series.

If you mean bookmarks.py is low-level and shouldn't be getting a lock,
then I would disagree since many other functions in that module are
indeed obtaining the lock; quite frequently, too.
via Mercurial-devel - June 21, 2017, 5:30 p.m.
On Tue, Jun 20, 2017 at 9:05 PM, Sean Farley <sean@farley.io> wrote:
> Gregory Szorc <gregory.szorc@gmail.com> writes:
>
>> On Tue, Jun 20, 2017 at 5:29 PM, Sean Farley <sean@farley.io> wrote:
>>
>>> # HG changeset patch
>>> # User Sean Farley <sean@farley.io>
>>> # Date 1497333768 25200
>>> #      Mon Jun 12 23:02:48 2017 -0700
>>> # Branch bm-refactor
>>> # Node ID 9f36f5280f7d2a0230efe1fd1dcf1a1d57f56ae8
>>> # Parent  92a9b407c3b96df20e12a2076578bdc664600e09
>>> bookmarks: factor out delete logic from commands
>>>
>>> While we're here, let's use fancy context managers. I believe this
>>> should still work since our locks are re-entrant.
>>>
>>> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
>>> index 451c557..c344cfc 100644
>>> --- a/mercurial/bookmarks.py
>>> +++ b/mercurial/bookmarks.py
>>> @@ -689,5 +689,21 @@ def checkformat(repo, mark):
>>>      if not mark:
>>>          raise error.Abort(_("bookmark names cannot consist entirely of "
>>>                              "whitespace"))
>>>      scmutil.checknewlabel(repo, mark, 'bookmark')
>>>      return mark
>>> +
>>> +def delete(repo, names):
>>> +    """remove a mark from the bookmark store
>>> +
>>> +    Raises an abort error if mark does not exist.
>>> +    """
>>> +    with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>>>
>>
>> I stopped reviewing at this patch because I'm unsure of this pattern of
>> obtaining locks and a transaction in such a low-level function. Yes, things
>> are re-entrant and functionally it should work. But it doesn't seem right
>> to me. Passing in a transaction instance that can be reused with other
>> bookmark modifications feels better. I'm just not sure if we have a soft
>> rule around this, so I'm deferring review.
>
> If you mean that commands.py still has a lock / try / finally, then my
> response is that all that is removed by the end of this series.
>
> If you mean bookmarks.py is low-level and shouldn't be getting a lock,
> then I would disagree

I think I agree with Greg that the caller should be responsible for
locking and creating the transaction. So I would prefer the signature
to be bookmarks.delete(repo, tr, names). That would make it clearer
when the bookmark updates are part of a larger transaction. See my
recent patch to pass in a transaction to changegroup.apply():
https://www.mercurial-scm.org/repo/hg/rev/af31d531dda0.

> since many other functions in that module are
> indeed obtaining the lock; quite frequently, too.

Looking through the other functions, I think many of them should also
be given a transaction by the caller instead.

>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Sean Farley - June 21, 2017, 5:39 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Tue, Jun 20, 2017 at 9:05 PM, Sean Farley <sean@farley.io> wrote:
>> Gregory Szorc <gregory.szorc@gmail.com> writes:
>>
>>> On Tue, Jun 20, 2017 at 5:29 PM, Sean Farley <sean@farley.io> wrote:
>>>
>>>> # HG changeset patch
>>>> # User Sean Farley <sean@farley.io>
>>>> # Date 1497333768 25200
>>>> #      Mon Jun 12 23:02:48 2017 -0700
>>>> # Branch bm-refactor
>>>> # Node ID 9f36f5280f7d2a0230efe1fd1dcf1a1d57f56ae8
>>>> # Parent  92a9b407c3b96df20e12a2076578bdc664600e09
>>>> bookmarks: factor out delete logic from commands
>>>>
>>>> While we're here, let's use fancy context managers. I believe this
>>>> should still work since our locks are re-entrant.
>>>>
>>>> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
>>>> index 451c557..c344cfc 100644
>>>> --- a/mercurial/bookmarks.py
>>>> +++ b/mercurial/bookmarks.py
>>>> @@ -689,5 +689,21 @@ def checkformat(repo, mark):
>>>>      if not mark:
>>>>          raise error.Abort(_("bookmark names cannot consist entirely of "
>>>>                              "whitespace"))
>>>>      scmutil.checknewlabel(repo, mark, 'bookmark')
>>>>      return mark
>>>> +
>>>> +def delete(repo, names):
>>>> +    """remove a mark from the bookmark store
>>>> +
>>>> +    Raises an abort error if mark does not exist.
>>>> +    """
>>>> +    with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>>>>
>>>
>>> I stopped reviewing at this patch because I'm unsure of this pattern of
>>> obtaining locks and a transaction in such a low-level function. Yes, things
>>> are re-entrant and functionally it should work. But it doesn't seem right
>>> to me. Passing in a transaction instance that can be reused with other
>>> bookmark modifications feels better. I'm just not sure if we have a soft
>>> rule around this, so I'm deferring review.
>>
>> If you mean that commands.py still has a lock / try / finally, then my
>> response is that all that is removed by the end of this series.
>>
>> If you mean bookmarks.py is low-level and shouldn't be getting a lock,
>> then I would disagree
>
> I think I agree with Greg that the caller should be responsible for
> locking and creating the transaction. So I would prefer the signature
> to be bookmarks.delete(repo, tr, names). That would make it clearer
> when the bookmark updates are part of a larger transaction. See my
> recent patch to pass in a transaction to changegroup.apply():
> https://www.mercurial-scm.org/repo/hg/rev/af31d531dda0.

Yeah; given the discussion on IRC I would agree now. I'll rework this series.

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
index 451c557..c344cfc 100644
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -689,5 +689,21 @@  def checkformat(repo, mark):
     if not mark:
         raise error.Abort(_("bookmark names cannot consist entirely of "
                             "whitespace"))
     scmutil.checknewlabel(repo, mark, 'bookmark')
     return mark
+
+def delete(repo, names):
+    """remove a mark from the bookmark store
+
+    Raises an abort error if mark does not exist.
+    """
+    with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
+        marks = repo._bookmarks
+        for mark in names:
+            if mark not in marks:
+                raise error.Abort(_("bookmark '%s' does not exist") %
+                                  mark)
+            if mark == repo._activebookmark:
+                deactivate(repo)
+            del marks[mark]
+        marks.recordchange(tr)
diff --git a/mercurial/commands.py b/mercurial/commands.py
index 4dd2521..d017f0a 100644
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -972,19 +972,11 @@  def bookmark(ui, repo, *names, **opts):
             wlock = repo.wlock()
             lock = repo.lock()
             cur = repo.changectx('.').node()
             marks = repo._bookmarks
             if delete:
-                tr = repo.transaction('bookmark')
-                for mark in names:
-                    if mark not in marks:
-                        raise error.Abort(_("bookmark '%s' does not exist") %
-                                         mark)
-                    if mark == repo._activebookmark:
-                        bookmarks.deactivate(repo)
-                    del marks[mark]
-
+                bookmarks.delete(repo, names)
             elif rename:
                 tr = repo.transaction('bookmark')
                 if not names:
                     raise error.Abort(_("new bookmark name required"))
                 elif len(names) > 1: