[PATCH] bookmarks: rename the active bookmark with no arguments

[Ryan McElroy]

On 7/19/17 11:49 AM, David Demelier wrote:
> # HG changeset patch
> # User David Demelier <demelier.david at 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]
>