bookmarks: check bookmark format during rename (issue3662)

mercurial/commands.py

@@ -789,6 +789,24 b' def bookmark(ui, repo, mark=None, rev=No'
     marks = repo._bookmarks
     cur   = repo.changectx('.').node()
 
+    def checkformat(mark):
+        if "\n" in mark:
+            raise util.Abort(_("bookmark name cannot contain newlines"))
+        mark = mark.strip()
+        if not mark:
+            raise util.Abort(_("bookmark names cannot consist entirely of "
+                               "whitespace"))
+        return mark
+
+    def checkconflict(repo, mark, force=False):
+        if mark in marks and not force:
+            raise util.Abort(_("bookmark '%s' already exists "
+                               "(use -f to force)") % mark)
+        if ((mark in repo.branchmap() or mark == repo.dirstate.branch())
+            and not force):
+            raise util.Abort(
+                _("a bookmark cannot have the name of an existing branch"))
+
     if delete:
         if mark is None:
             raise util.Abort(_("bookmark name required"))
@@ -801,13 +819,12 b' def bookmark(ui, repo, mark=None, rev=No'
         return
 
     if rename:
+        if mark is None:
+            raise util.Abort(_("new bookmark name required"))
+        mark = checkformat(mark)
         if rename not in marks:
             raise util.Abort(_("bookmark '%s' does not exist") % rename)
-        if mark in marks and not force:
-            raise util.Abort(_("bookmark '%s' already exists "
-                               "(use -f to force)") % mark)
-        if mark is None:
-            raise util.Abort(_("new bookmark name required"))
+        checkconflict(repo, mark, force)
         marks[mark] = marks[rename]
         if repo._bookmarkcurrent == rename and not inactive:
             bookmarks.setcurrent(repo, mark)
@@ -816,22 +833,11 b' def bookmark(ui, repo, mark=None, rev=No'
         return
 
     if mark is not None:
-        if "\n" in mark:
-            raise util.Abort(_("bookmark name cannot contain newlines"))
-        mark = mark.strip()
-        if not mark:
-            raise util.Abort(_("bookmark names cannot consist entirely of "
-                               "whitespace"))
+        mark = checkformat(mark)
         if inactive and mark == repo._bookmarkcurrent:
             bookmarks.setcurrent(repo, None)
             return
-        if mark in marks and not force:
-            raise util.Abort(_("bookmark '%s' already exists "
-                               "(use -f to force)") % mark)
-        if ((mark in repo.branchmap() or mark == repo.dirstate.branch())
-            and not force):
-            raise util.Abort(
-                _("a bookmark cannot have the name of an existing branch"))
+        checkconflict(repo, mark, force)
         if rev:
             marks[mark] = scmutil.revsingle(repo, rev).node()
         else:

tests/test-bookmarks.t

@@ -217,12 +217,31 b' reject bookmark name with newline'
   abort: bookmark name cannot contain newlines
   [255]
 
+  $ hg bookmark -m Z '
+  > '
+  abort: bookmark name cannot contain newlines
+  [255]
+
 bookmark with existing name
 
   $ hg bookmark Z
   abort: bookmark 'Z' already exists (use -f to force)
   [255]
 
+  $ hg bookmark -m Y Z
+  abort: bookmark 'Z' already exists (use -f to force)
+  [255]
+
+bookmark with name of branch
+
+  $ hg bookmark default
+  abort: a bookmark cannot have the name of an existing branch
+  [255]
+
+  $ hg bookmark -m Y default
+  abort: a bookmark cannot have the name of an existing branch
+  [255]
+
 force bookmark with existing name
 
   $ hg bookmark -f Z
@@ -247,6 +266,10 b' bookmark name with whitespace only'
   abort: bookmark names cannot consist entirely of whitespace
   [255]
 
+  $ hg bookmark -m Y ' '
+  abort: bookmark names cannot consist entirely of whitespace
+  [255]
+
 invalid bookmark
 
   $ hg bookmark 'foo:bar'