Patchwork bookmarks: reject bookmark names that unambiguously resolve to a node (BC)

login
register
mail settings
Submitter Augie Fackler
Date May 22, 2017, 11:44 p.m.
Message ID <2b2ecc8ef94635e26ba4.1495496692@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/20854/
State Superseded
Headers show

Comments

Augie Fackler - May 22, 2017, 11:44 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1495495092 14400
#      Mon May 22 19:18:12 2017 -0400
# Node ID 2b2ecc8ef94635e26ba41246db5bd693e6078481
# Parent  8db2feb04cebc1878c6232dd2650f2c5468d350e
bookmarks: reject bookmark names that unambiguously resolve to a node (BC)

I just burned myself on this today because I left out the -r in my `hg
bookmark` command, which then left me confused because I didn't notice
the bookmark I created in the wrong place that was silently shadowing
the revision I was trying to check out. Let's just reject this without
--force to avoid user confusion.

This patch only enforces the check on bookmark names 4 characters long
or longer. We can tweak that if we'd like, I selected that since
that's the fewest characters shortest will use in the templater
output, but it does increase the odds that an otherwise-valid bookmark
will become unusable later (eg 'cafe' which currently only matches
hidden revisions in my clone of Mercurial.) I think the inconvenience
on short hex-friendly words is worth the extra sanity for users in the
long term.
Yuya Nishihara - May 23, 2017, 2:29 p.m.
On Mon, 22 May 2017 19:44:52 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1495495092 14400
> #      Mon May 22 19:18:12 2017 -0400
> # Node ID 2b2ecc8ef94635e26ba41246db5bd693e6078481
> # Parent  8db2feb04cebc1878c6232dd2650f2c5468d350e
> bookmarks: reject bookmark names that unambiguously resolve to a node (BC)
> 
> I just burned myself on this today because I left out the -r in my `hg
> bookmark` command, which then left me confused because I didn't notice
> the bookmark I created in the wrong place that was silently shadowing
> the revision I was trying to check out. Let's just reject this without
> --force to avoid user confusion.
> 
> This patch only enforces the check on bookmark names 4 characters long
> or longer. We can tweak that if we'd like, I selected that since
> that's the fewest characters shortest will use in the templater
> output, but it does increase the odds that an otherwise-valid bookmark
> will become unusable later (eg 'cafe' which currently only matches
> hidden revisions in my clone of Mercurial.) I think the inconvenience
> on short hex-friendly words is worth the extra sanity for users in the
> long term.

I like the idea, but the new behavior might be a bit confusing because
the conflicted names aren't statically predictable. It might also cause
problems in scripting and automation use.

So, how about showing a warning message?

> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -968,6 +968,10 @@ def bookmark(ui, repo, *names, **opts):
>              and not force):
>              raise error.Abort(
>                  _("a bookmark cannot have the name of an existing branch"))
> +        if len(mark) > 3 and mark in repo and not force:

repo.__contains__() may raise error.LookupError if mark is an ambiguous hash.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -968,6 +968,10 @@  def bookmark(ui, repo, *names, **opts):
             and not force):
             raise error.Abort(
                 _("a bookmark cannot have the name of an existing branch"))
+        if len(mark) > 3 and mark in repo and not force:
+            raise error.Abort(
+                _("bookmark %s matches a changeset hash") % mark,
+                hint=_("did you leave a -r out of an 'hg bookmark' command?"))
 
     if delete and rename:
         raise error.Abort(_("--delete and --rename are incompatible"))
diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
--- a/tests/test-bookmarks.t
+++ b/tests/test-bookmarks.t
@@ -311,6 +311,12 @@  bookmark with integer name
   abort: cannot use an integer as a name
   [255]
 
+bookmark with a name that matches a node id
+  $ hg bookmark 925d80f479bb db815d6d32e6
+  abort: bookmark 925d80f479bb matches a changeset hash
+  (did you leave a -r out of an 'hg bookmark' command?)
+  [255]
+
 incompatible options
 
   $ hg bookmark -m Y -d Z