Patchwork [1,of,9,bm-refactor] commands: move checkformat to bookmarks module

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

Comments

Sean Farley - June 21, 2017, 12:29 a.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1497162778 25200
#      Sat Jun 10 23:32:58 2017 -0700
# Branch bm-refactor
# Node ID 03591d8a6d8247d35bbdc93cebce1757755ca463
# Parent  6d79e9109908c2cb468c9eeaf8869aa1926fbea8
commands: move checkformat to bookmarks module

commands.bookmark has grown quite large with two closures already. Let's
split this up (and in the process allow extensions to override the
default behavior).
Gregory Szorc - June 21, 2017, 2:46 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 1497162778 25200
> #      Sat Jun 10 23:32:58 2017 -0700
> # Branch bm-refactor
> # Node ID 03591d8a6d8247d35bbdc93cebce1757755ca463
> # Parent  6d79e9109908c2cb468c9eeaf8869aa1926fbea8
> commands: move checkformat to bookmarks module
>

Queued parts 1 and 2.


>
> commands.bookmark has grown quite large with two closures already. Let's
> split this up (and in the process allow extensions to override the
> default behavior).
>
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> index db01ef9..948bfbe 100644
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -17,10 +17,11 @@ from .node import (
>  from . import (
>      encoding,
>      error,
>      lock as lockmod,
>      obsolete,
> +    scmutil,
>      txnutil,
>      util,
>  )
>
>  def _getbkfile(repo):
> @@ -620,5 +621,17 @@ def validdest(repo, old, new):
>      elif repo.obsstore:
>          return new.node() in obsolete.foreground(repo, [old.node()])
>      else:
>          # still an independent clause as it is lazier (and therefore
> faster)
>          return old.descendant(new)
> +
> +def checkformat(repo, mark):
> +    """return a valid version of a potential bookmark name
> +
> +    Raises an abort error if the bookmark name is not valid.
> +    """
> +    mark = mark.strip()
> +    if not mark:
> +        raise error.Abort(_("bookmark names cannot consist entirely of "
> +                            "whitespace"))
> +    scmutil.checknewlabel(repo, mark, 'bookmark')
> +    return mark
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> index a56cc76..abb42f7 100644
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -955,18 +955,10 @@ def bookmark(ui, repo, *names, **opts):
>      rev = opts.get('rev')
>      delete = opts.get('delete')
>      rename = opts.get('rename')
>      inactive = opts.get('inactive')
>
> -    def checkformat(mark):
> -        mark = mark.strip()
> -        if not mark:
> -            raise error.Abort(_("bookmark names cannot consist entirely
> of "
> -                               "whitespace"))
> -        scmutil.checknewlabel(repo, mark, 'bookmark')
> -        return mark
> -
>      def checkconflict(repo, mark, cur, force=False, target=None):
>          if mark in marks and not force:
>              if target:
>                  if marks[mark] == target and target == cur:
>                      # re-activating a bookmark
> @@ -1037,11 +1029,11 @@ def bookmark(ui, repo, *names, **opts):
>                  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"))
> -                mark = checkformat(names[0])
> +                mark = bookmarks.checkformat(repo, names[0])
>                  if rename not in marks:
>                      raise error.Abort(_("bookmark '%s' does not exist")
>                                        % rename)
>                  checkconflict(repo, mark, cur, force)
>                  marks[mark] = marks[rename]
> @@ -1050,11 +1042,11 @@ def bookmark(ui, repo, *names, **opts):
>                  del marks[rename]
>              elif names:
>                  tr = repo.transaction('bookmark')
>                  newact = None
>                  for mark in names:
> -                    mark = checkformat(mark)
> +                    mark = bookmarks.checkformat(repo, mark)
>                      if newact is None:
>                          newact = mark
>                      if inactive and mark == repo._activebookmark:
>                          bookmarks.deactivate(repo)
>                          return
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
index db01ef9..948bfbe 100644
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -17,10 +17,11 @@  from .node import (
 from . import (
     encoding,
     error,
     lock as lockmod,
     obsolete,
+    scmutil,
     txnutil,
     util,
 )
 
 def _getbkfile(repo):
@@ -620,5 +621,17 @@  def validdest(repo, old, new):
     elif repo.obsstore:
         return new.node() in obsolete.foreground(repo, [old.node()])
     else:
         # still an independent clause as it is lazier (and therefore faster)
         return old.descendant(new)
+
+def checkformat(repo, mark):
+    """return a valid version of a potential bookmark name
+
+    Raises an abort error if the bookmark name is not valid.
+    """
+    mark = mark.strip()
+    if not mark:
+        raise error.Abort(_("bookmark names cannot consist entirely of "
+                            "whitespace"))
+    scmutil.checknewlabel(repo, mark, 'bookmark')
+    return mark
diff --git a/mercurial/commands.py b/mercurial/commands.py
index a56cc76..abb42f7 100644
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -955,18 +955,10 @@  def bookmark(ui, repo, *names, **opts):
     rev = opts.get('rev')
     delete = opts.get('delete')
     rename = opts.get('rename')
     inactive = opts.get('inactive')
 
-    def checkformat(mark):
-        mark = mark.strip()
-        if not mark:
-            raise error.Abort(_("bookmark names cannot consist entirely of "
-                               "whitespace"))
-        scmutil.checknewlabel(repo, mark, 'bookmark')
-        return mark
-
     def checkconflict(repo, mark, cur, force=False, target=None):
         if mark in marks and not force:
             if target:
                 if marks[mark] == target and target == cur:
                     # re-activating a bookmark
@@ -1037,11 +1029,11 @@  def bookmark(ui, repo, *names, **opts):
                 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"))
-                mark = checkformat(names[0])
+                mark = bookmarks.checkformat(repo, names[0])
                 if rename not in marks:
                     raise error.Abort(_("bookmark '%s' does not exist")
                                       % rename)
                 checkconflict(repo, mark, cur, force)
                 marks[mark] = marks[rename]
@@ -1050,11 +1042,11 @@  def bookmark(ui, repo, *names, **opts):
                 del marks[rename]
             elif names:
                 tr = repo.transaction('bookmark')
                 newact = None
                 for mark in names:
-                    mark = checkformat(mark)
+                    mark = bookmarks.checkformat(repo, mark)
                     if newact is None:
                         newact = mark
                     if inactive and mark == repo._activebookmark:
                         bookmarks.deactivate(repo)
                         return