Patchwork bookmarks: rename the active bookmark with no arguments

login
register
mail settings
Submitter David Demelier
Date July 19, 2017, 10:49 a.m.
Message ID <eeeafb331795055c7cae.1500461344@fedy>
Download mbox | patch
Permalink /patch/22524/
State Deferred, archived
Headers show

Comments

David Demelier - July 19, 2017, 10:49 a.m.
# HG changeset patch
# User David Demelier <demelier.david@gmail.com>
# Date 1500449739 -7200
#      Wed Jul 19 09:35:39 2017 +0200
# Node ID eeeafb331795055c7cae78cae5724c51637555dc
# Parent  9a944e908ecf9ac3aabf30a24406513370eeebe7
bookmarks: rename the active bookmark with no arguments

For convenience, the active bookmark is renamed if less than 2 arguments are
given.
Ryan McElroy - July 19, 2017, 11:59 a.m.
On 7/19/17 11:49 AM, David Demelier wrote:
> # HG changeset patch
> # User David Demelier <demelier.david@gmail.com>
> # Date 1500449739 -7200
> #      Wed Jul 19 09:35:39 2017 +0200
> # Node ID eeeafb331795055c7cae78cae5724c51637555dc
> # Parent  9a944e908ecf9ac3aabf30a24406513370eeebe7
> bookmarks: rename the active bookmark with no arguments
>
> For convenience, the active bookmark is renamed if less than 2 arguments are
> given.

I like this idea, but I have some concerns too. I'd like to hear what 
others think.

>
> diff -r 9a944e908ecf -r eeeafb331795 mercurial/commands.py
> --- a/mercurial/commands.py	Tue Jul 18 23:04:08 2017 +0530
> +++ b/mercurial/commands.py	Wed Jul 19 09:35:39 2017 +0200
> @@ -895,7 +895,7 @@
>       [('f', 'force', False, _('force')),
>       ('r', 'rev', '', _('revision for bookmark action'), _('REV')),
>       ('d', 'delete', False, _('delete a given bookmark')),
> -    ('m', 'rename', '', _('rename a given bookmark'), _('OLD')),
> +    ('m', 'rename', '', _('rename the active or given bookmark'), _('NAME')),
>       ('i', 'inactive', False, _('mark a bookmark inactive')),
>       ] + formatteropts,
>       _('hg bookmarks [OPTIONS]... [NAME]...'))
> @@ -920,6 +920,8 @@
>       A bookmark named '@' has the special property that :hg:`clone` will
>       check it out by default if it exists.
>   
> +    If only one argument is given to -m, the active bookmark is renamed.
> +

This is technically incorrect. Each flag in hg that accepts a parameter 
accepts only one parameter. If you want to pass multiple parameters, you 
have to pass the flag again (ie, with -r).

In this case, it's not -m taking two parameters, but -m taking one 
parameter and the bookmarks command taking a positional parameter. What 
you're doing is making the positional parameter optional if there's an 
active bookmark. This is a good direction, but you should be precise in 
the wording you use here.

I'd suggest 'if the positional parameter is omitted, then the active 
bookmark name will be changed'.

>       .. container:: verbose
>   
>         Examples:
> @@ -940,6 +942,10 @@
>   
>             hg book -m turkey dinner
>   
> +      - rename the active bookmark to dinner::
> +
> +          hg book -m dinner
> +
>         - move the '@' bookmark from another branch::
>   
>             hg book -f @
> @@ -964,11 +970,18 @@
>               if delete:
>                   bookmarks.delete(repo, tr, names)
>               elif rename:
> -                if not names:
> -                    raise error.Abort(_("new bookmark name required"))
> -                elif len(names) > 1:
> +                if len(names) == 0:
> +                    if not repo._activebookmark:
> +                        raise error.Abort(_("no active bookmark to rename"))

Suggestion for completeness/correctness: 'no active bookmark nor 
bookmark name given to rename'

> +                    old = repo._activebookmark
> +                    new = rename
> +                elif len(names) == 1:
> +                    old = rename
> +                    new = names[0]

This is the biggest concern I have with this change -- it swaps where 
the new name comes from.

BEFORE THIS PATCH:

$ hg book foo # foo is now active, no other bookmarks exist
$ hg book -m bar
abort: new bookmark name required
$ hg book -m bar foo
abort: bookmark 'bar' does not exist
$ hg book -m foo bar # foo is now named bar

AFTER THIS PATCH:

$ hg book foo # foo is now active, no other bookmarks exist
$ hg book -m bar # foo is now named bar
$ hg book -m bar foo # bar is now named foo
$ hg book -m foo bar # foo is now named to bar

Note that between the second and third invocations of hg above, the 
meaning of the parameter to -m has totally changed.

Personally, I'm okay with this change since it seems pretty natural to 
an end-user, but it's definitely inconsistent when you look at it 
technically.

> +                else:
>                       raise error.Abort(_("only one new bookmark name allowed"))
> -                bookmarks.rename(repo, tr, rename, names[0], force, inactive)
> +
> +                bookmarks.rename(repo, tr, old, new, force, inactive)
>               elif names:
>                   bookmarks.addbookmarks(repo, tr, names, rev, force, inactive)
>               elif inactive:
> diff -r 9a944e908ecf -r eeeafb331795 tests/test-bookmarks.t
> --- a/tests/test-bookmarks.t	Tue Jul 18 23:04:08 2017 +0530
> +++ b/tests/test-bookmarks.t	Wed Jul 19 09:35:39 2017 +0200
> @@ -187,6 +187,27 @@
>     abort: bookmark 'Y' already exists (use -f to force)
>     [255]
>   
> +rename the active bookmark
> +
> +  $ hg bookmark A
> +  $ hg bookmark -m B
> +  $ hg bookmarks
> +   * B                         2:db815d6d32e6
> +     X                         2:db815d6d32e6
> +     X2                        1:925d80f479bb
> +     Y                         -1:000000000000
> +     Z                         0:f7b1eb17ad24
> +  $ hg book -d B
> +  $ hg up -q Y
> +
> +rename no active bookmark
> +
> +  $ hg up -q tip
> +  $ hg book -m test
> +  abort: no active bookmark to rename
> +  [255]
> +  $ hg up -q Y
> +
>   force rename to existent bookmark
>   
>     $ hg bookmark -f -m X Y
> @@ -203,7 +224,7 @@
>     $ hg bookmark -r ':tip' TIP
>     $ hg up -q TIP
>     $ hg bookmarks
> -     REVSET                    0:f7b1eb17ad24
> +     REVSET                    -1:000000000000
>      * TIP                       2:db815d6d32e6
>        X2                        1:925d80f479bb
>        Y                         2:db815d6d32e6
> @@ -212,11 +233,8 @@
>     $ hg bookmark -d REVSET
>     $ hg bookmark -d TIP
>   
> -rename without new name or multiple names
> +rename with multiple names
>   
> -  $ hg bookmark -m Y
> -  abort: new bookmark name required
> -  [255]
>     $ hg bookmark -m Y Y2 Y3
>     abort: only one new bookmark name allowed
>     [255]
>
Augie Fackler - July 25, 2017, 1:28 a.m.
> On Jul 19, 2017, at 7:59 AM, Ryan McElroy <rm@fb.com> wrote:
> 
>> +                    old = repo._activebookmark
>> +                    new = rename
>> +                elif len(names) == 1:
>> +                    old = rename
>> +                    new = names[0]
> 
> This is the biggest concern I have with this change -- it swaps where the new name comes from.
> 
> BEFORE THIS PATCH:
> 
> $ hg book foo # foo is now active, no other bookmarks exist
> $ hg book -m bar
> abort: new bookmark name required
> $ hg book -m bar foo
> abort: bookmark 'bar' does not exist
> $ hg book -m foo bar # foo is now named bar
> 
> AFTER THIS PATCH:
> 
> $ hg book foo # foo is now active, no other bookmarks exist
> $ hg book -m bar # foo is now named bar
> $ hg book -m bar foo # bar is now named foo
> $ hg book -m foo bar # foo is now named to bar
> 
> Note that between the second and third invocations of hg above, the meaning of the parameter to -m has totally changed.
> 
> Personally, I'm okay with this change since it seems pretty natural to an end-user, but it's definitely inconsistent when you look at it technically.

I think this feels okay-ish. David, please re-send after August 1st.

(Sorry this feel through the cracks.)
David Demelier - July 27, 2017, 7:09 a.m.
On Mon, 2017-07-24 at 21:28 -0400, Augie Fackler wrote:
> I think this feels okay-ish. David, please re-send after August 1st.
> 
> (Sorry this feel through the cracks.)

Yes, I'll also update it to use '.' argument instead as it's common in
several other commands (hg push/pull -B .) to specify the active
bookmark and it also addresses the issues that Ryan pointed out. It
does not swap the logic of old/new selection.

Thus, we will have the following behaviour:

hg book -m old_new (as before)
hg book -m . new (rename the active bookmark to new if any)

Regards,

Patch

diff -r 9a944e908ecf -r eeeafb331795 mercurial/commands.py
--- a/mercurial/commands.py	Tue Jul 18 23:04:08 2017 +0530
+++ b/mercurial/commands.py	Wed Jul 19 09:35:39 2017 +0200
@@ -895,7 +895,7 @@ 
     [('f', 'force', False, _('force')),
     ('r', 'rev', '', _('revision for bookmark action'), _('REV')),
     ('d', 'delete', False, _('delete a given bookmark')),
-    ('m', 'rename', '', _('rename a given bookmark'), _('OLD')),
+    ('m', 'rename', '', _('rename the active or given bookmark'), _('NAME')),
     ('i', 'inactive', False, _('mark a bookmark inactive')),
     ] + formatteropts,
     _('hg bookmarks [OPTIONS]... [NAME]...'))
@@ -920,6 +920,8 @@ 
     A bookmark named '@' has the special property that :hg:`clone` will
     check it out by default if it exists.
 
+    If only one argument is given to -m, the active bookmark is renamed.
+
     .. container:: verbose
 
       Examples:
@@ -940,6 +942,10 @@ 
 
           hg book -m turkey dinner
 
+      - rename the active bookmark to dinner::
+
+          hg book -m dinner
+
       - move the '@' bookmark from another branch::
 
           hg book -f @
@@ -964,11 +970,18 @@ 
             if delete:
                 bookmarks.delete(repo, tr, names)
             elif rename:
-                if not names:
-                    raise error.Abort(_("new bookmark name required"))
-                elif len(names) > 1:
+                if len(names) == 0:
+                    if not repo._activebookmark:
+                        raise error.Abort(_("no active bookmark to rename"))
+                    old = repo._activebookmark
+                    new = rename
+                elif len(names) == 1:
+                    old = rename
+                    new = names[0]
+                else:
                     raise error.Abort(_("only one new bookmark name allowed"))
-                bookmarks.rename(repo, tr, rename, names[0], force, inactive)
+
+                bookmarks.rename(repo, tr, old, new, force, inactive)
             elif names:
                 bookmarks.addbookmarks(repo, tr, names, rev, force, inactive)
             elif inactive:
diff -r 9a944e908ecf -r eeeafb331795 tests/test-bookmarks.t
--- a/tests/test-bookmarks.t	Tue Jul 18 23:04:08 2017 +0530
+++ b/tests/test-bookmarks.t	Wed Jul 19 09:35:39 2017 +0200
@@ -187,6 +187,27 @@ 
   abort: bookmark 'Y' already exists (use -f to force)
   [255]
 
+rename the active bookmark
+
+  $ hg bookmark A
+  $ hg bookmark -m B
+  $ hg bookmarks
+   * B                         2:db815d6d32e6
+     X                         2:db815d6d32e6
+     X2                        1:925d80f479bb
+     Y                         -1:000000000000
+     Z                         0:f7b1eb17ad24
+  $ hg book -d B
+  $ hg up -q Y
+
+rename no active bookmark
+
+  $ hg up -q tip
+  $ hg book -m test
+  abort: no active bookmark to rename
+  [255]
+  $ hg up -q Y
+
 force rename to existent bookmark
 
   $ hg bookmark -f -m X Y
@@ -203,7 +224,7 @@ 
   $ hg bookmark -r ':tip' TIP
   $ hg up -q TIP
   $ hg bookmarks
-     REVSET                    0:f7b1eb17ad24
+     REVSET                    -1:000000000000
    * TIP                       2:db815d6d32e6
      X2                        1:925d80f479bb
      Y                         2:db815d6d32e6
@@ -212,11 +233,8 @@ 
   $ hg bookmark -d REVSET
   $ hg bookmark -d TIP
 
-rename without new name or multiple names
+rename with multiple names
 
-  $ hg bookmark -m Y
-  abort: new bookmark name required
-  [255]
   $ hg bookmark -m Y Y2 Y3
   abort: only one new bookmark name allowed
   [255]