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

[Sean Farley]

Martin von Zweigbergk <martinvonz at google.com> writes:

> On Tue, Jun 20, 2017 at 9:05 PM, Sean Farley <sean at farley.io> wrote:
>> Gregory Szorc <gregory.szorc at gmail.com> writes:
>>
>>> On Tue, Jun 20, 2017 at 5:29 PM, Sean Farley <sean at farley.io> wrote:
>>>
>>>> # HG changeset patch
>>>> # User Sean Farley <sean at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170621/69c15ea5/attachment.sig>