Patchwork [2,of,5] commands: delay checknewlabel until after aborts

login
register
mail settings
Submitter timeless@mozdev.org
Date Jan. 10, 2016, 6:57 p.m.
Message ID <b15de606bd509eba7655.1452452269@waste.org>
Download mbox | patch
Permalink /patch/12632/
State Accepted
Headers show

Comments

timeless@mozdev.org - Jan. 10, 2016, 6:57 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1451185999 0
#      Sun Dec 27 03:13:19 2015 +0000
# Node ID b15de606bd509eba76554071d9e4e40b1a9bb879
# Parent  81f7508339fc28775b31ab1724c88eda6d11e85e
commands: delay checknewlabel until after aborts
Yuya Nishihara - Jan. 13, 2016, 3:10 p.m.
On Sun, 10 Jan 2016 12:57:49 -0600, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1451185999 0
> #      Sun Dec 27 03:13:19 2015 +0000
> # Node ID b15de606bd509eba76554071d9e4e40b1a9bb879
> # Parent  81f7508339fc28775b31ab1724c88eda6d11e85e
> commands: delay checknewlabel until after aborts

"until after aborts" ?

> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -1030,7 +1030,6 @@
>          if not mark:
>              raise error.Abort(_("bookmark names cannot consist entirely of "
>                                 "whitespace"))
> -        scmutil.checknewlabel(repo, mark, 'bookmark')
>          return mark

It doesn't make sense to me that checkformat() is still applied to known
bookmarks. Do we want to trust known bookmarks or don't we?

>      def checkconflict(repo, mark, cur, force=False, target=None):
> @@ -1102,6 +1101,8 @@
>                      raise error.Abort(_("bookmark '%s' does not exist")
>                                        % rename)
>                  checkconflict(repo, mark, cur, force)
> +                if not mark in repo._bookmarks:

Nit: "not in" is preferred than "not (x in y)".

> @@ -1209,13 +1212,15 @@
>              repo.dirstate.setbranch(label)
>              ui.status(_('reset working directory to branch %s\n') % label)
>          elif label:
> -            if not opts.get('force') and label in repo.branchmap():
> -                if label not in [p.branch() for p in repo[None].parents()]:
> -                    raise error.Abort(_('a branch of the same name already'
> -                                       ' exists'),
> -                                     # i18n: "it" refers to an existing branch
> -                                     hint=_("use 'hg update' to switch to it"))
> -            scmutil.checknewlabel(repo, label, 'branch')
> +            if label in repo.branchmap():
> +                if not opts.get('force'):
> +                    if label not in [p.branch() for p in repo[None].parents()]:
> +                        raise error.Abort(
> +                            _('a branch of the same name already exists'),
> +                            # i18n: "it" refers to an existing branch
> +                            hint=_("use 'hg update' to switch to it"))
> +            else:
> +                scmutil.checknewlabel(repo, label, 'branch')

This fixes https://bz.mercurial-scm.org/show_bug.cgi?id=5016 .
Can you add a test?

That said, I'm afraid of delaying checknewlabel() because we would have to be
more careful to not make new labels bypass the validation.
timeless - Jan. 14, 2016, 6:32 p.m.
Yuya Nishihara wrote:
>> commands: delay checknewlabel until after aborts
> "until after aborts" ?

"abort opportunities"?

The idea was that other aborts were more meaningful.

> It doesn't make sense to me that checkformat() is still applied to known
> bookmarks. Do we want to trust known bookmarks or don't we?

with my change, checkformat is really just "strip trailing whitespace
and ensure there's something left". I've renamed it.

> This fixes https://bz.mercurial-scm.org/show_bug.cgi?id=5016 .
> Can you add a test?

It took a bit to get it to work (I shake my first at both hg export
and hg import), but yes.

> That said, I'm afraid of delaying checknewlabel() because we would have to be
> more careful to not make new labels bypass the validation.
Yuya Nishihara - Jan. 15, 2016, 1:27 p.m.
On Thu, 14 Jan 2016 13:32:48 -0500, timeless wrote:
> > It doesn't make sense to me that checkformat() is still applied to known
> > bookmarks. Do we want to trust known bookmarks or don't we?
> 
> with my change, checkformat is really just "strip trailing whitespace
> and ensure there's something left". I've renamed it.
> 
> > This fixes https://bz.mercurial-scm.org/show_bug.cgi?id=5016 .
> > Can you add a test?
> 
> It took a bit to get it to work (I shake my first at both hg export
> and hg import), but yes.
> 
> > That said, I'm afraid of delaying checknewlabel() because we would have to be
> > more careful to not make new labels bypass the validation.

Given that checknewlabel() receives repo, it would be designed to eventually
do more specific things. And we have namespace API now. So perhaps, we can
make checknewlabel() allow existing badly-named label.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1030,7 +1030,6 @@ 
         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):
@@ -1102,6 +1101,8 @@ 
                     raise error.Abort(_("bookmark '%s' does not exist")
                                       % rename)
                 checkconflict(repo, mark, cur, force)
+                if not mark in repo._bookmarks:
+                    scmutil.checknewlabel(repo, mark, 'bookmark')
                 marks[mark] = marks[rename]
                 if repo._activebookmark == rename and not inactive:
                     bookmarks.activate(repo, mark)
@@ -1111,6 +1112,8 @@ 
                 newact = None
                 for mark in names:
                     mark = checkformat(mark)
+                    if not mark in repo._bookmarks:
+                        scmutil.checknewlabel(repo, mark, 'bookmark')
                     if newact is None:
                         newact = mark
                     if inactive and mark == repo._activebookmark:
@@ -1209,13 +1212,15 @@ 
             repo.dirstate.setbranch(label)
             ui.status(_('reset working directory to branch %s\n') % label)
         elif label:
-            if not opts.get('force') and label in repo.branchmap():
-                if label not in [p.branch() for p in repo[None].parents()]:
-                    raise error.Abort(_('a branch of the same name already'
-                                       ' exists'),
-                                     # i18n: "it" refers to an existing branch
-                                     hint=_("use 'hg update' to switch to it"))
-            scmutil.checknewlabel(repo, label, 'branch')
+            if label in repo.branchmap():
+                if not opts.get('force'):
+                    if label not in [p.branch() for p in repo[None].parents()]:
+                        raise error.Abort(
+                            _('a branch of the same name already exists'),
+                            # i18n: "it" refers to an existing branch
+                            hint=_("use 'hg update' to switch to it"))
+            else:
+                scmutil.checknewlabel(repo, label, 'branch')
             repo.dirstate.setbranch(label)
             ui.status(_('marked working directory as branch %s\n') % label)