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

[Gregory Szorc]

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.


> +        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 at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170620/52c225cb/attachment.html>