Patchwork bookmark: don't allow integers as bookmark/branch/tag names

login
register
mail settings
Submitter Durham Goode
Date Feb. 6, 2013, 1:57 a.m.
Message ID <3e4293758d473380c287.1360115825@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/818/
State Accepted
Commit 341868ef0cf6bb3fc9fc4f5b4f34047ba57b5a77
Headers show

Comments

Durham Goode - Feb. 6, 2013, 1:57 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1360110173 28800
# Node ID 3e4293758d473380c2877d07ead9eaeccff34b65
# Parent  c6377e34cb1ed79cb142f01652b0acfa09ef8c1f
bookmark: don't allow integers as bookmark/branch/tag names

Bookmarks/branches/tags shouldn't be allowed to be integers because that
overlaps with revision numbers.  Right now if a user created one they can't
use it anyway because the revision numbers take precedence.

The check only happens when creating a new bookmark/etc from a command so it
shouldn't affect existing bookmarks/branches/tags or importing branches from
git.

This fix was prompted by us having a user create a bookmark named "404" then
accidentally checkout a very old version of our repository.
Bryan O'Sullivan - Feb. 8, 2013, 11:18 a.m.
On Tue, Feb 5, 2013 at 5:57 PM, Durham Goode <durham@fb.com> wrote:

> bookmark: don't allow integers as bookmark/branch/tag names
>

Pushed to crew, thanks.
Bryan O'Sullivan - Feb. 8, 2013, 11:24 a.m.
On Tue, Feb 5, 2013 at 5:57 PM, Durham Goode <durham@fb.com> wrote:

> The check only happens when creating a new bookmark/etc from a command so
> it
> shouldn't affect existing bookmarks/branches/tags or importing branches
> from
> git.
>

This is important to note: all this change does is forbid the creation of
new tags/branches/bookmarks with integer names. Existing ones can still be
retrieved using revset queries. (It has always been the case that if you
try "hg log -r33" and you have a bookmark, branch, or tag named "33",
you're gonna have a bad time.)
Wagner Bruna - April 19, 2013, 4:59 p.m.
I noticed this during translation:

Em 05-02-2013 23:57, Durham Goode escreveu:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1360110173 28800
> # Node ID 3e4293758d473380c2877d07ead9eaeccff34b65
> # Parent  c6377e34cb1ed79cb142f01652b0acfa09ef8c1f
> bookmark: don't allow integers as bookmark/branch/tag names

(...)

> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -34,6 +34,11 @@
>      for c in (':', '\0', '\n', '\r'):
>          if c in lbl:
>              raise util.Abort(_("%r cannot be used in a name") % c)
> +    try:
> +        int(lbl)
> +        raise util.Abort(_("a %s cannot have an integer as its name") % kind)

This message can't be translated, since it'd need to match the unknown
parameter's particular grammatical rules (for instance, in the Brazilian
Portuguese translation, 'bookmark' and 'branch' are masculine nouns, but 'tag'
is feminine; the article at the beginning would need to match both).

But I'm not sure what would be a good message here... Some related translation
discussions (dealing with the same function!):

http://selenic.com/pipermail/mercurial-devel/2012-October/045542.html
http://selenic.com/pipermail/mercurial-devel/2012-October/045567.html

Regards,
Wagner
Mads Kiilerich - April 19, 2013, 5:25 p.m.
On 04/19/2013 06:59 PM, Wagner Bruna wrote:
> I noticed this during translation:
>
> Em 05-02-2013 23:57, Durham Goode escreveu:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1360110173 28800
>> # Node ID 3e4293758d473380c2877d07ead9eaeccff34b65
>> # Parent  c6377e34cb1ed79cb142f01652b0acfa09ef8c1f
>> bookmark: don't allow integers as bookmark/branch/tag names
> (...)
>
>> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>> --- a/mercurial/scmutil.py
>> +++ b/mercurial/scmutil.py
>> @@ -34,6 +34,11 @@
>>       for c in (':', '\0', '\n', '\r'):
>>           if c in lbl:
>>               raise util.Abort(_("%r cannot be used in a name") % c)
>> +    try:
>> +        int(lbl)
>> +        raise util.Abort(_("a %s cannot have an integer as its name") % kind)
> This message can't be translated, since it'd need to match the unknown
> parameter's particular grammatical rules (for instance, in the Brazilian
> Portuguese translation, 'bookmark' and 'branch' are masculine nouns, but 'tag'
> is feminine; the article at the beginning would need to match both).
>
> But I'm not sure what would be a good message here... Some related translation
> discussions (dealing with the same function!):
>
> http://selenic.com/pipermail/mercurial-devel/2012-October/045542.html
> http://selenic.com/pipermail/mercurial-devel/2012-October/045567.html

So:

http://selenic.com/pipermail/mercurial-devel/2012-October/045567.html
a patch that
1. removed the 'kind' parameter and
2. replaced it with whatever

http://selenic.com/pipermail/mercurial-devel/2012-October/045600.html
suggested using the term 'label'

http://selenic.com/repo/hg//rev/71c1513fd560
the resent and applied patch that
1. did not remove the 'kind' parameter (and thus tricked Durham into 
using it)
2. used 'name' instead of 'label'

I suggest making
a patch for stable that change 'name' to 'label'
a patch for stable that changes the new integer message to use 'label' 
too and thus make the kind parameter unused again
a patch for default after 2.6 that removes the kind parameter

/Mads
Durham Goode - April 19, 2013, 5:51 p.m.
On 4/19/13 10:25 AM, "Mads Kiilerich" <mads@kiilerich.com> wrote:

>On 04/19/2013 06:59 PM, Wagner Bruna wrote:
>> I noticed this during translation:
>>
>>> +    try:
>>> +        int(lbl)
>>> +        raise util.Abort(_("a %s cannot have an integer as its name")
>>>% kind)
>> This message can't be translated, since it'd need to match the unknown
>> parameter's particular grammatical rules (for instance, in the Brazilian
>> Portuguese translation, 'bookmark' and 'branch' are masculine nouns,
>>but 'tag'
>> is feminine; the article at the beginning would need to match both).
>>
>> But I'm not sure what would be a good message here... Some related
>>translation
>> discussions (dealing with the same function!):
>>
>> http://selenic.com/pipermail/mercurial-devel/2012-October/045542.html
>> http://selenic.com/pipermail/mercurial-devel/2012-October/045567.html
>
>I suggest making
>a patch for stable that change 'name' to 'label'
>a patch for stable that changes the new integer message to use 'label'
>too and thus make the kind parameter unused again
>a patch for default after 2.6 that removes the kind parameter

Matt's comment in that original thread is to not remove the 'kinds'
parameter since it might be used in the future.  I'll just add a comment
in the function saying not to use it in ui output.  Matt's comment also
mentions the kind of label is implicit, so actually mentioning 'branch,
bookmark, or tag' isn't necessary.

I'll submit a patch for stable to change it to "cannot use an integer as a
label"

Durham

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -34,6 +34,11 @@ 
     for c in (':', '\0', '\n', '\r'):
         if c in lbl:
             raise util.Abort(_("%r cannot be used in a name") % c)
+    try:
+        int(lbl)
+        raise util.Abort(_("a %s cannot have an integer as its name") % kind)
+    except ValueError:
+        pass
 
 def checkfilename(f):
     '''Check that the filename f is an acceptable filename for a tracked file'''
diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
--- a/tests/test-bookmarks.t
+++ b/tests/test-bookmarks.t
@@ -257,6 +257,12 @@ 
   abort: a bookmark cannot have the name of an existing branch
   [255]
 
+bookmark with integer name
+
+  $ hg bookmark 10
+  abort: a bookmark cannot have an integer as its name
+  [255]
+
 incompatible options
 
   $ hg bookmark -m Y -d Z
diff --git a/tests/test-rebase-collapse.t b/tests/test-rebase-collapse.t
--- a/tests/test-rebase-collapse.t
+++ b/tests/test-rebase-collapse.t
@@ -496,15 +496,15 @@ 
   $ hg ci -Am 'A'
   adding a
 
-  $ hg branch '1'
-  marked working directory as branch 1
+  $ hg branch 'one'
+  marked working directory as branch one
   (branches are permanent and global, did you want a bookmark?)
   $ echo 'b' > b
   $ hg ci -Am 'B'
   adding b
 
-  $ hg branch '2'
-  marked working directory as branch 2
+  $ hg branch 'two'
+  marked working directory as branch two
   (branches are permanent and global, did you want a bookmark?)
   $ echo 'c' > c
   $ hg ci -Am 'C'
@@ -518,9 +518,9 @@ 
   $ hg tglog
   @  3: 'D'
   |
-  | o  2: 'C' 2
+  | o  2: 'C' two
   | |
-  | o  1: 'B' 1
+  | o  1: 'B' one
   |/
   o  0: 'A'
   
@@ -546,9 +546,9 @@ 
   |/
   o  3: 'D'
   |
-  | o  2: 'C' 2
+  | o  2: 'C' two
   | |
-  | o  1: 'B' 1
+  | o  1: 'B' one
   |/
   o  0: 'A'
   
@@ -559,9 +559,9 @@ 
   |
   o  3: 'D'
   |
-  | o  2: 'C' 2
+  | o  2: 'C' two
   | |
-  | o  1: 'B' 1
+  | o  1: 'B' one
   |/
   o  0: 'A'