Patchwork [5,of,5] bookmark: don't allow revsets as bookmark/branch/tag names

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

Comments

timeless@mozdev.org - Jan. 10, 2016, 6:57 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1451260878 0
#      Mon Dec 28 00:01:18 2015 +0000
# Node ID 05215827364872784c6a551a4ec6a955f037c952
# Parent  8d2ac62c7f35e860bd6fae43d6a03dd0289739b1
bookmark: don't allow revsets as bookmark/branch/tag names
Yuya Nishihara - Jan. 13, 2016, 3:40 p.m.
On Sun, 10 Jan 2016 12:57:52 -0600, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1451260878 0
> #      Mon Dec 28 00:01:18 2015 +0000
> # Node ID 05215827364872784c6a551a4ec6a955f037c952
> # Parent  8d2ac62c7f35e860bd6fae43d6a03dd0289739b1
> bookmark: don't allow revsets as bookmark/branch/tag names

Misleading. It's allowed, just warned.

> +def checklabelisrevset(repo, lbl, kind):
> +    if re.search("^[a-f0-9]{1,16}$", lbl):
> +        # technically, a label named `a` does interfere with a changeset
> +        # a...,
> +        # and a label named 10f interfers with a changeset named
> +        # 10f...,
> +        # but, we'll let that slide for now.
> +        return
> +    try:
> +        m = revset.matchany(repo.ui, [lbl], repo)
> +        items = []
> +        for rev in m(repo):
> +            items.append(rev)
> +            if len(items) > 1:
> +                break
> +        if items:
> +            if len(items) > 1:
> +                items = _("%s, %s, ...") % (items[0], items[1])
> +            else:
> +                items = items[0]
> +            repo.ui.warn(_(
> +                "warning: %s %s conflicts with a revset matching %s\n") %
> +                (kind, lbl, items))

Not possible to translate "kind".

> +    except error.ParseError:
> +        pass
> +    except error.RepoLookupError:
> +        pass

Perhaps LookupError, Abort, etc. can happen.

Do you really want to check all sort of revset expressions? From the test
output, it seems you just wanted to warn about conflicting names.
timeless - Jan. 14, 2016, 6:16 p.m.
Yuya Nishihara wrote:
> On Sun, 10 Jan 2016 12:57:52 -0600, timeless wrote:
>> bookmark: don't allow revsets as bookmark/branch/tag names

> Misleading. It's allowed, just warned.

Oh, yeah. My goal was to make it an abort. But I'm not sure how
comfortable we'd be w/ a change like that immediately, so initially i
decided to try a warning.

I'll resend w/ a [RFC] (BC) marker

> Not possible to translate "kind".

Grr, yeah, genders are a pain. Added a note, although it'd be helpful
if there was a link or something for people to read.

>> +    except error.ParseError:
>> +        pass
>> +    except error.RepoLookupError:
>> +        pass
>
> Perhaps LookupError, Abort, etc. can happen.

I wish I had some way of knowing if they could.

> Do you really want to check all sort of revset expressions? From the test
> output, it seems you just wanted to warn about conflicting names.

Oh, I didn't include my main case in my tests (added):

hg bookmark foo .^

specifically, that creates a bookmark named `.^` which conflicts with
the revset `.^`.
Yuya Nishihara - Jan. 15, 2016, 1:40 p.m.
On Thu, 14 Jan 2016 13:16:33 -0500, timeless wrote:
> Yuya Nishihara wrote:
> > On Sun, 10 Jan 2016 12:57:52 -0600, timeless wrote:
> >> +    except error.ParseError:
> >> +        pass
> >> +    except error.RepoLookupError:
> >> +        pass
> >
> > Perhaps LookupError, Abort, etc. can happen.
> 
> I wish I had some way of knowing if they could.

Oh, exception specifications.

> > Do you really want to check all sort of revset expressions? From the test
> > output, it seems you just wanted to warn about conflicting names.
> 
> Oh, I didn't include my main case in my tests (added):
> 
> hg bookmark foo .^
> 
> specifically, that creates a bookmark named `.^` which conflicts with
> the revset `.^`.

Then, how about checking if new bookmark name is parsable as a revset?
revset.parse() won't raise arbitrary exceptions. Also, hg bookmark "not all()"
should match nothing, but it does shadow the expression.

 1. check name conflicts
    not found -> ok
    found in other namespace -> warn
 2. check revset conflicts
    syntax error -> ok
    parsed to symbol -> ok
    parsed to expression -> warn
timeless - Jan. 15, 2016, 5:18 p.m.
Sounds good. I won't be able to do that before Wednesday, and it seems like
it's more than a simple one liner, so probably later.

Thanks for the suggestions.
On Jan 15, 2016 8:40 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:

> On Thu, 14 Jan 2016 13:16:33 -0500, timeless wrote:
> > Yuya Nishihara wrote:
> > > On Sun, 10 Jan 2016 12:57:52 -0600, timeless wrote:
> > >> +    except error.ParseError:
> > >> +        pass
> > >> +    except error.RepoLookupError:
> > >> +        pass
> > >
> > > Perhaps LookupError, Abort, etc. can happen.
> >
> > I wish I had some way of knowing if they could.
>
> Oh, exception specifications.
>
> > > Do you really want to check all sort of revset expressions? From the
> test
> > > output, it seems you just wanted to warn about conflicting names.
> >
> > Oh, I didn't include my main case in my tests (added):
> >
> > hg bookmark foo .^
> >
> > specifically, that creates a bookmark named `.^` which conflicts with
> > the revset `.^`.
>
> Then, how about checking if new bookmark name is parsable as a revset?
> revset.parse() won't raise arbitrary exceptions. Also, hg bookmark "not
> all()"
> should match nothing, but it does shadow the expression.
>
>  1. check name conflicts
>     not found -> ok
>     found in other namespace -> warn
>  2. check revset conflicts
>     syntax error -> ok
>     parsed to symbol -> ok
>     parsed to expression -> warn
>

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1103,6 +1103,7 @@ 
                 checkconflict(repo, mark, cur, force)
                 if not mark in repo._bookmarks:
                     scmutil.checknewlabel(repo, mark, 'bookmark')
+                    scmutil.checklabelisrevset(repo, mark, 'bookmark')
                 marks[mark] = marks[rename]
                 if repo._activebookmark == rename and not inactive:
                     bookmarks.activate(repo, mark)
@@ -1123,6 +1124,7 @@ 
                     checkconflict(repo, mark, cur, force, tgt)
                     if not mark in repo._bookmarks:
                         scmutil.checknewlabel(repo, mark, 'bookmark')
+                        scmutil.checklabelisrevset(repo, mark, 'bookmark')
                     marks[mark] = tgt
                 if not inactive and cur == marks[newact] and not rev:
                     bookmarks.activate(repo, newact)
@@ -1221,6 +1223,7 @@ 
                             hint=_("use 'hg update' to switch to it"))
             else:
                 scmutil.checknewlabel(repo, label, 'branch')
+                scmutil.checklabelisrevset(repo, label, 'branch')
             repo.dirstate.setbranch(label)
             ui.status(_('marked working directory as branch %s\n') % label)
 
@@ -6745,6 +6748,7 @@ 
                 if n in repo.tags():
                     raise error.Abort(_("tag '%s' already exists "
                                        "(use -f to force)") % n)
+                scmutil.checklabelisrevset(repo, n, 'tag')
         if not opts.get('local'):
             p1, p2 = repo.dirstate.parents()
             if p2 != nullid:
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -150,6 +150,34 @@ 
     except ValueError:
         pass
 
+def checklabelisrevset(repo, lbl, kind):
+    if re.search("^[a-f0-9]{1,16}$", lbl):
+        # technically, a label named `a` does interfere with a changeset
+        # a...,
+        # and a label named 10f interfers with a changeset named
+        # 10f...,
+        # but, we'll let that slide for now.
+        return
+    try:
+        m = revset.matchany(repo.ui, [lbl], repo)
+        items = []
+        for rev in m(repo):
+            items.append(rev)
+            if len(items) > 1:
+                break
+        if items:
+            if len(items) > 1:
+                items = _("%s, %s, ...") % (items[0], items[1])
+            else:
+                items = items[0]
+            repo.ui.warn(_(
+                "warning: %s %s conflicts with a revset matching %s\n") %
+                (kind, lbl, items))
+    except error.ParseError:
+        pass
+    except error.RepoLookupError:
+        pass
+
 def checkfilename(f):
     '''Check that the filename f is an acceptable filename for a tracked file'''
     if '\r' in f or '\n' in f:
diff --git a/tests/test-1993.t b/tests/test-1993.t
--- a/tests/test-1993.t
+++ b/tests/test-1993.t
@@ -7,6 +7,7 @@ 
   $ hg ci -Am1
   adding b
   $ hg tag -r0 default
+  warning: tag default conflicts with a revset matching 1
   warning: tag default conflicts with existing branch name
   $ hg log
   changeset:   2:30a83d1e4a1e
diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
--- a/tests/test-bookmarks.t
+++ b/tests/test-bookmarks.t
@@ -487,6 +487,7 @@ 
 test clone with a bookmark named "default" (issue3677)
 
   $ hg bookmark -r 1 -f -i default
+  warning: bookmark default conflicts with a revset matching 2
   $ hg clone . cloned-bookmark-default
   updating to branch default
   2 files updated, 0 files merged, 0 files removed, 0 files unresolved
diff --git a/tests/test-branch-tag-confict.t b/tests/test-branch-tag-confict.t
--- a/tests/test-branch-tag-confict.t
+++ b/tests/test-branch-tag-confict.t
@@ -13,6 +13,7 @@ 
 Create a branch with the same name as the tag.
 
   $ hg branch branchortag
+  warning: branch branchortag conflicts with a revset matching 0
   marked working directory as branch branchortag
   (branches are permanent and global, did you want a bookmark?)
   $ hg ci -m 'Create a branch with the same name as a tag.'
diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
--- a/tests/test-commandserver.t
+++ b/tests/test-commandserver.t
@@ -340,6 +340,7 @@ 
   ...     os.system('hg branch default')
   *** runcommand branch
   default
+  warning: branch foo conflicts with a revset matching 0
   marked working directory as branch foo
   (branches are permanent and global, did you want a bookmark?)
   *** runcommand branch
diff --git a/tests/test-encoding-align.t b/tests/test-encoding-align.t
--- a/tests/test-encoding-align.t
+++ b/tests/test-encoding-align.t
@@ -117,14 +117,17 @@ 
   (branches are permanent and global, did you want a bookmark?)
   $ hg tag $S
   $ hg book -f $S
+  warning: bookmark \xe7\x9f\xad\xe5\x90\x8d conflicts with a revset matching 2 (esc)
   $ hg branch $M
   marked working directory as branch MIDDLE_
   $ hg tag $M
   $ hg book -f $M
+  warning: bookmark MIDDLE_ conflicts with a revset matching 3
   $ hg branch $L
   marked working directory as branch \xe9\x95\xb7\xe3\x81\x84\xe9\x95\xb7\xe3\x81\x84\xe5\x90\x8d\xe5\x89\x8d (esc)
   $ hg tag $L
   $ hg book -f $L
+  warning: bookmark \xe9\x95\xb7\xe3\x81\x84\xe9\x95\xb7\xe3\x81\x84\xe5\x90\x8d\xe5\x89\x8d conflicts with a revset matching 4 (esc)
 
 check alignment of branches
 
diff --git a/tests/test-encoding.t b/tests/test-encoding.t
--- a/tests/test-encoding.t
+++ b/tests/test-encoding.t
@@ -41,6 +41,7 @@ 
   $ HGENCODING=utf-8 hg ci -l utf-8
   $ HGENCODING=latin-1 hg tag `cat latin-1-tag`
   $ HGENCODING=latin-1 hg branch `cat latin-1-tag`
+  warning: branch \xe9 conflicts with a revset matching 3 (esc)
   marked working directory as branch \xe9 (esc)
   (branches are permanent and global, did you want a bookmark?)
   $ HGENCODING=latin-1 hg ci -m 'latin1 branch'
diff --git a/tests/test-tag.t b/tests/test-tag.t
--- a/tests/test-tag.t
+++ b/tests/test-tag.t
@@ -218,6 +218,7 @@ 
   (branches are permanent and global, did you want a bookmark?)
   $ hg ci -m"discouraged"
   $ hg tag tag-and-branch-same-name
+  warning: tag tag-and-branch-same-name conflicts with a revset matching 11
   warning: tag tag-and-branch-same-name conflicts with existing branch name
 
 test custom commit messages