Patchwork [4,of,7,bm-refactor,V3] commands: replace locking code with a context manager

login
register
mail settings
Submitter Sean Farley
Date June 23, 2017, 6:33 p.m.
Message ID <5431515879d0efdf3682.1498242800@1.0.0.127.in-addr.arpa>
Download mbox | patch
Permalink /patch/21641/
State Accepted
Headers show

Comments

Sean Farley - June 23, 2017, 6:33 p.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1497998203 25200
#      Tue Jun 20 15:36:43 2017 -0700
# Branch bm-refactor
# Node ID 5431515879d0efdf36825c094d66b9345f635e95
# Parent  cf4fcf960a580afc9650ae73b5ab4d2c10e8a594
commands: replace locking code with a context manager
via Mercurial-devel - June 23, 2017, 6:51 p.m.
On Fri, Jun 23, 2017 at 11:33 AM, Sean Farley <sean@farley.io> wrote:
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1497998203 25200
> #      Tue Jun 20 15:36:43 2017 -0700
> # Branch bm-refactor
> # Node ID 5431515879d0efdf36825c094d66b9345f635e95
> # Parent  cf4fcf960a580afc9650ae73b5ab4d2c10e8a594
> commands: replace locking code with a context manager
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> index 86f811e..085dcfb 100644
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -965,40 +965,28 @@ def bookmark(ui, repo, *names, **opts):
>          raise error.Abort(_("--rev is incompatible with --rename"))
>      if not names and (delete or rev):
>          raise error.Abort(_("bookmark name required"))
>
>      if delete or rename or names or inactive:
> -        wlock = lock = tr = None
> -        try:
> -            wlock = repo.wlock()
> -            lock = repo.lock()
> -            marks = repo._bookmarks
> +        with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>              if delete:
> -                tr = repo.transaction('bookmark')
>                  bookmarks.delete(repo, tr, names)
>              elif rename:
> -                tr = repo.transaction('bookmark')
>                  if not names:
>                      raise error.Abort(_("new bookmark name required"))
>                  elif len(names) > 1:
>                      raise error.Abort(_("only one new bookmark name allowed"))
>                  bookmarks.rename(repo, tr, rename, names[0], force, inactive)
>              elif names:
> -                tr = repo.transaction('bookmark')
>                  bookmarks.addbookmarks(repo, tr, names, rev, force, inactive)
>              elif inactive:

We now create a transaction in this case too, which we didn't before.
I can't see any harm in that, though. I still wanted to mention it
since maybe that's the reason the transaction was not created earlier
before.

> -                if len(marks) == 0:
> +                if len(repo._bookmarks) == 0:
>                      ui.status(_("no bookmarks set\n"))
>                  elif not repo._activebookmark:
>                      ui.status(_("no active bookmark\n"))
>                  else:
>                      bookmarks.deactivate(repo)
> -            if tr is not None:
> -                marks.recordchange(tr)
> -                tr.close()
> -        finally:
> -            lockmod.release(tr, lock, wlock)
>      else: # show bookmarks
>          fm = ui.formatter('bookmarks', opts)
>          hexfn = fm.hexfunc
>          marks = repo._bookmarks
>          if len(marks) == 0 and fm.isplain():
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Sean Farley - June 23, 2017, 7:04 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Fri, Jun 23, 2017 at 11:33 AM, Sean Farley <sean@farley.io> wrote:
>> # HG changeset patch
>> # User Sean Farley <sean@farley.io>
>> # Date 1497998203 25200
>> #      Tue Jun 20 15:36:43 2017 -0700
>> # Branch bm-refactor
>> # Node ID 5431515879d0efdf36825c094d66b9345f635e95
>> # Parent  cf4fcf960a580afc9650ae73b5ab4d2c10e8a594
>> commands: replace locking code with a context manager
>>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> index 86f811e..085dcfb 100644
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -965,40 +965,28 @@ def bookmark(ui, repo, *names, **opts):
>>          raise error.Abort(_("--rev is incompatible with --rename"))
>>      if not names and (delete or rev):
>>          raise error.Abort(_("bookmark name required"))
>>
>>      if delete or rename or names or inactive:
>> -        wlock = lock = tr = None
>> -        try:
>> -            wlock = repo.wlock()
>> -            lock = repo.lock()
>> -            marks = repo._bookmarks
>> +        with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>>              if delete:
>> -                tr = repo.transaction('bookmark')
>>                  bookmarks.delete(repo, tr, names)
>>              elif rename:
>> -                tr = repo.transaction('bookmark')
>>                  if not names:
>>                      raise error.Abort(_("new bookmark name required"))
>>                  elif len(names) > 1:
>>                      raise error.Abort(_("only one new bookmark name allowed"))
>>                  bookmarks.rename(repo, tr, rename, names[0], force, inactive)
>>              elif names:
>> -                tr = repo.transaction('bookmark')
>>                  bookmarks.addbookmarks(repo, tr, names, rev, force, inactive)
>>              elif inactive:
>
> We now create a transaction in this case too, which we didn't before.
> I can't see any harm in that, though. I still wanted to mention it
> since maybe that's the reason the transaction was not created earlier
> before.

That's a good spot. Maybe I / you could add it to the commit message?
via Mercurial-devel - June 23, 2017, 7:30 p.m.
On Fri, Jun 23, 2017 at 12:04 PM, Sean Farley <sean@farley.io> wrote:
> Martin von Zweigbergk <martinvonz@google.com> writes:
>
>> On Fri, Jun 23, 2017 at 11:33 AM, Sean Farley <sean@farley.io> wrote:
>>> # HG changeset patch
>>> # User Sean Farley <sean@farley.io>
>>> # Date 1497998203 25200
>>> #      Tue Jun 20 15:36:43 2017 -0700
>>> # Branch bm-refactor
>>> # Node ID 5431515879d0efdf36825c094d66b9345f635e95
>>> # Parent  cf4fcf960a580afc9650ae73b5ab4d2c10e8a594
>>> commands: replace locking code with a context manager
>>>
>>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>>> index 86f811e..085dcfb 100644
>>> --- a/mercurial/commands.py
>>> +++ b/mercurial/commands.py
>>> @@ -965,40 +965,28 @@ def bookmark(ui, repo, *names, **opts):
>>>          raise error.Abort(_("--rev is incompatible with --rename"))
>>>      if not names and (delete or rev):
>>>          raise error.Abort(_("bookmark name required"))
>>>
>>>      if delete or rename or names or inactive:
>>> -        wlock = lock = tr = None
>>> -        try:
>>> -            wlock = repo.wlock()
>>> -            lock = repo.lock()
>>> -            marks = repo._bookmarks
>>> +        with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>>>              if delete:
>>> -                tr = repo.transaction('bookmark')
>>>                  bookmarks.delete(repo, tr, names)
>>>              elif rename:
>>> -                tr = repo.transaction('bookmark')
>>>                  if not names:
>>>                      raise error.Abort(_("new bookmark name required"))
>>>                  elif len(names) > 1:
>>>                      raise error.Abort(_("only one new bookmark name allowed"))
>>>                  bookmarks.rename(repo, tr, rename, names[0], force, inactive)
>>>              elif names:
>>> -                tr = repo.transaction('bookmark')
>>>                  bookmarks.addbookmarks(repo, tr, names, rev, force, inactive)
>>>              elif inactive:
>>
>> We now create a transaction in this case too, which we didn't before.
>> I can't see any harm in that, though. I still wanted to mention it
>> since maybe that's the reason the transaction was not created earlier
>> before.
>
> That's a good spot. Maybe I / you could add it to the commit message?

Sure, I'll add the following paragraph. Let me know if it's incorrect
or you want to make some adjustment.

Note that this means that we're unnecessarily creating a transaction
in the pure "--inactive" (i.e. when deactivating the current
bookmark), but that should be harmless.
Sean Farley - June 23, 2017, 7:32 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Fri, Jun 23, 2017 at 12:04 PM, Sean Farley <sean@farley.io> wrote:
>> Martin von Zweigbergk <martinvonz@google.com> writes:
>>
>>> On Fri, Jun 23, 2017 at 11:33 AM, Sean Farley <sean@farley.io> wrote:
>>>> # HG changeset patch
>>>> # User Sean Farley <sean@farley.io>
>>>> # Date 1497998203 25200
>>>> #      Tue Jun 20 15:36:43 2017 -0700
>>>> # Branch bm-refactor
>>>> # Node ID 5431515879d0efdf36825c094d66b9345f635e95
>>>> # Parent  cf4fcf960a580afc9650ae73b5ab4d2c10e8a594
>>>> commands: replace locking code with a context manager
>>>>
>>>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>>>> index 86f811e..085dcfb 100644
>>>> --- a/mercurial/commands.py
>>>> +++ b/mercurial/commands.py
>>>> @@ -965,40 +965,28 @@ def bookmark(ui, repo, *names, **opts):
>>>>          raise error.Abort(_("--rev is incompatible with --rename"))
>>>>      if not names and (delete or rev):
>>>>          raise error.Abort(_("bookmark name required"))
>>>>
>>>>      if delete or rename or names or inactive:
>>>> -        wlock = lock = tr = None
>>>> -        try:
>>>> -            wlock = repo.wlock()
>>>> -            lock = repo.lock()
>>>> -            marks = repo._bookmarks
>>>> +        with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
>>>>              if delete:
>>>> -                tr = repo.transaction('bookmark')
>>>>                  bookmarks.delete(repo, tr, names)
>>>>              elif rename:
>>>> -                tr = repo.transaction('bookmark')
>>>>                  if not names:
>>>>                      raise error.Abort(_("new bookmark name required"))
>>>>                  elif len(names) > 1:
>>>>                      raise error.Abort(_("only one new bookmark name allowed"))
>>>>                  bookmarks.rename(repo, tr, rename, names[0], force, inactive)
>>>>              elif names:
>>>> -                tr = repo.transaction('bookmark')
>>>>                  bookmarks.addbookmarks(repo, tr, names, rev, force, inactive)
>>>>              elif inactive:
>>>
>>> We now create a transaction in this case too, which we didn't before.
>>> I can't see any harm in that, though. I still wanted to mention it
>>> since maybe that's the reason the transaction was not created earlier
>>> before.
>>
>> That's a good spot. Maybe I / you could add it to the commit message?
>
> Sure, I'll add the following paragraph. Let me know if it's incorrect
> or you want to make some adjustment.
>
> Note that this means that we're unnecessarily creating a transaction
> in the pure "--inactive" (i.e. when deactivating the current
> bookmark), but that should be harmless.

Looks good to me!

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
index 86f811e..085dcfb 100644
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -965,40 +965,28 @@  def bookmark(ui, repo, *names, **opts):
         raise error.Abort(_("--rev is incompatible with --rename"))
     if not names and (delete or rev):
         raise error.Abort(_("bookmark name required"))
 
     if delete or rename or names or inactive:
-        wlock = lock = tr = None
-        try:
-            wlock = repo.wlock()
-            lock = repo.lock()
-            marks = repo._bookmarks
+        with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
             if delete:
-                tr = repo.transaction('bookmark')
                 bookmarks.delete(repo, tr, names)
             elif rename:
-                tr = repo.transaction('bookmark')
                 if not names:
                     raise error.Abort(_("new bookmark name required"))
                 elif len(names) > 1:
                     raise error.Abort(_("only one new bookmark name allowed"))
                 bookmarks.rename(repo, tr, rename, names[0], force, inactive)
             elif names:
-                tr = repo.transaction('bookmark')
                 bookmarks.addbookmarks(repo, tr, names, rev, force, inactive)
             elif inactive:
-                if len(marks) == 0:
+                if len(repo._bookmarks) == 0:
                     ui.status(_("no bookmarks set\n"))
                 elif not repo._activebookmark:
                     ui.status(_("no active bookmark\n"))
                 else:
                     bookmarks.deactivate(repo)
-            if tr is not None:
-                marks.recordchange(tr)
-                tr.close()
-        finally:
-            lockmod.release(tr, lock, wlock)
     else: # show bookmarks
         fm = ui.formatter('bookmarks', opts)
         hexfn = fm.hexfunc
         marks = repo._bookmarks
         if len(marks) == 0 and fm.isplain():