Patchwork [stable] branch: strip whitespace before testing known branch name

login
register
mail settings
Submitter Yuya Nishihara
Date May 7, 2013, 10:45 p.m.
Message ID <f42a6337cac2c3920fb7.1367966756@gimlet>
Download mbox | patch
Permalink /patch/1589/
State Accepted
Commit 12dbdd348bb0977366200bf96cb6d2afa85faf13
Headers show

Comments

Yuya Nishihara - May 7, 2013, 10:45 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1367938143 -32400
#      Tue May 07 23:49:03 2013 +0900
# Branch stable
# Node ID f42a6337cac2c3920fb7587797ada022c1141182
# Parent  0a12e5f3a979ee302dc10647483200df00a105ab
branch: strip whitespace before testing known branch name

Because dirstate._branch() strips leading/trailing spaces from .hg/branch,
"hg branch ' foo '" should abort if branch "foo" exists in another head.

tag command had a similar bug and fixed by 3d0a9c8d7184.
Augie Fackler - May 9, 2013, 1:49 p.m.
On Wed, May 08, 2013 at 07:45:56AM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1367938143 -32400
> #      Tue May 07 23:49:03 2013 +0900
> # Branch stable
> # Node ID f42a6337cac2c3920fb7587797ada022c1141182
> # Parent  0a12e5f3a979ee302dc10647483200df00a105ab
> branch: strip whitespace before testing known branch name

LGTM, but my paranoia wants either mpm to see this and ack it, or
another crew member to do so before queueing this for stable, since it
is technically a behavior change (even though it strikes me as a bug
fix in light of 3d0a9c8d7184.)

>
> Because dirstate._branch() strips leading/trailing spaces from .hg/branch,
> "hg branch ' foo '" should abort if branch "foo" exists in another head.
>
> tag command had a similar bug and fixed by 3d0a9c8d7184.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -943,6 +943,9 @@ def branch(ui, repo, label=None, **opts)
>
>      Returns 0 on success.
>      """
> +    if label:
> +        label = label.strip()
> +
>      if not opts.get('clean') and not label:
>          ui.write("%s\n" % repo.dirstate.branch())
>          return
> diff --git a/tests/test-branches.t b/tests/test-branches.t
> --- a/tests/test-branches.t
> +++ b/tests/test-branches.t
> @@ -68,6 +68,18 @@ invalid characters
>    abort: '\n' cannot be used in a name
>    [255]
>
> +trailing or leading spaces should be stripped before testing duplicates
> +
> +  $ hg branch 'b '
> +  abort: a branch of the same name already exists
> +  (use 'hg update' to switch to it)
> +  [255]
> +
> +  $ hg branch ' b'
> +  abort: a branch of the same name already exists
> +  (use 'hg update' to switch to it)
> +  [255]
> +
>  verify update will accept invalid legacy branch names
>
>    $ hg init test-invalid-branch-name
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - May 10, 2013, 4:07 p.m.
On Thu, 9 May 2013 14:58:43 -0500, Kevin Bullock wrote:
> On 7 May 2013, at 5:45 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1367938143 -32400
> > #      Tue May 07 23:49:03 2013 +0900
> > # Branch stable
> > # Node ID f42a6337cac2c3920fb7587797ada022c1141182
> > # Parent  0a12e5f3a979ee302dc10647483200df00a105ab
> > branch: strip whitespace before testing known branch name
> > 
> > Because dirstate._branch() strips leading/trailing spaces from .hg/branch,
> > "hg branch ' foo '" should abort if branch "foo" exists in another head.
> 
> This should be done in scmutil.checknewlabel() if possible. It's already done
> for tags, and should apply to bookmarks as well.

It seems checknewlabel() is a bit different. checknewlabel() tests invalid
label, which is mandatory check. But in this case, it's valid to have multiple
branch heads of the same name.

This patch tries to address the following problem:

 - "hg branch ' foo '" creates the branch "foo" on next commit.
 - but it bypasses "label in repo.branchmap()", and thus doesn't abort even
   if there exists the branch "foo" in another head.

Regards,

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -943,6 +943,9 @@  def branch(ui, repo, label=None, **opts)
 
     Returns 0 on success.
     """
+    if label:
+        label = label.strip()
+
     if not opts.get('clean') and not label:
         ui.write("%s\n" % repo.dirstate.branch())
         return
diff --git a/tests/test-branches.t b/tests/test-branches.t
--- a/tests/test-branches.t
+++ b/tests/test-branches.t
@@ -68,6 +68,18 @@  invalid characters
   abort: '\n' cannot be used in a name
   [255]
 
+trailing or leading spaces should be stripped before testing duplicates
+
+  $ hg branch 'b '
+  abort: a branch of the same name already exists
+  (use 'hg update' to switch to it)
+  [255]
+
+  $ hg branch ' b'
+  abort: a branch of the same name already exists
+  (use 'hg update' to switch to it)
+  [255]
+
 verify update will accept invalid legacy branch names
 
   $ hg init test-invalid-branch-name