Patchwork [2,of,2,RFC] dispatch: look up command by [<space>:]<cmdname> syntax (PoC)

login
register
mail settings
Submitter Yuya Nishihara
Date Feb. 22, 2018, 2:54 p.m.
Message ID <cb9e1cad42a9a13f17c4.1519311287@mimosa>
Download mbox | patch
Permalink /patch/28260/
State New
Headers show

Comments

Yuya Nishihara - Feb. 22, 2018, 2:54 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1516798904 -32400
#      Wed Jan 24 22:01:44 2018 +0900
# Node ID cb9e1cad42a9a13f17c4c75d350cd509b08f4a21
# Parent  e2030eaec92b1ed12577cbe48cd0495d106818a9
dispatch: look up command by [<space>:]<cmdname> syntax (PoC)

This allows us to run the show command without giving up our show alias.
The separator ':' is copied from the merge-tools syntax, ":<internal-tool>".

  [alias]
  show = log -pvr
  work = show:show work

  $ hg work

So, do we like it?
Anton Shestakov - Feb. 23, 2018, 5:25 a.m.
On Thu, 22 Feb 2018 23:54:47 +0900
Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1516798904 -32400
> #      Wed Jan 24 22:01:44 2018 +0900
> # Node ID cb9e1cad42a9a13f17c4c75d350cd509b08f4a21
> # Parent  e2030eaec92b1ed12577cbe48cd0495d106818a9
> dispatch: look up command by [<space>:]<cmdname> syntax (PoC)
> 
> This allows us to run the show command without giving up our show alias.
> The separator ':' is copied from the merge-tools syntax, ":<internal-tool>".
> 
>   [alias]
>   show = log -pvr
>   work = show:show work

This syntax looks alright to me as a user, but maybe this should also
support "internal:foo" form in addition to ":foo", in case people want
to spell it out, and to be fully compatible syntax-wise with
merge-tools. My thought is that if somebody asks why not use backslash
with core commands to avoid aliases (like in bash, e.g. \ls) then we
can say that this is simply 100% reuse of pre-existing syntax (in
addition to mentioning namespaces for extensions).
Yuya Nishihara - Feb. 24, 2018, 12:54 p.m.
On Fri, 23 Feb 2018 13:25:52 +0800, Anton Shestakov wrote:
> On Thu, 22 Feb 2018 23:54:47 +0900
> Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1516798904 -32400
> > #      Wed Jan 24 22:01:44 2018 +0900
> > # Node ID cb9e1cad42a9a13f17c4c75d350cd509b08f4a21
> > # Parent  e2030eaec92b1ed12577cbe48cd0495d106818a9
> > dispatch: look up command by [<space>:]<cmdname> syntax (PoC)
> > 
> > This allows us to run the show command without giving up our show alias.
> > The separator ':' is copied from the merge-tools syntax, ":<internal-tool>".
> > 
> >   [alias]
> >   show = log -pvr
> >   work = show:show work
> 
> This syntax looks alright to me as a user, but maybe this should also
> support "internal:foo" form in addition to ":foo", in case people want
> to spell it out, and to be fully compatible syntax-wise with
> merge-tools.

That would conflict with the "internal" extension if we had. That's unlikely
in practice, but I don't want to special-case it.

> My thought is that if somebody asks why not use backslash
> with core commands to avoid aliases (like in bash, e.g. \ls) then we
> can say that this is simply 100% reuse of pre-existing syntax (in
> addition to mentioning namespaces for extensions).

Hmm, nobody would want double backslashes, I suppose.
Gregory Szorc - Feb. 24, 2018, 7:39 p.m.
On Thu, Feb 22, 2018 at 6:54 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1516798904 -32400
> #      Wed Jan 24 22:01:44 2018 +0900
> # Node ID cb9e1cad42a9a13f17c4c75d350cd509b08f4a21
> # Parent  e2030eaec92b1ed12577cbe48cd0495d106818a9
> dispatch: look up command by [<space>:]<cmdname> syntax (PoC)
>
> This allows us to run the show command without giving up our show alias.
> The separator ':' is copied from the merge-tools syntax,
> ":<internal-tool>".
>
>   [alias]
>   show = log -pvr
>   work = show:show work
>
>   $ hg work
>
> So, do we like it?
>

I don't know the code that well, so I can't comment on the patch.

I will say that I believe we had general consensus at a previous sprint to
implement proper support for sub-commands. This would allow us to implement
arguments on individual show views. e.g. `hg show work --maxage 14d`. It
could also be useful for commands like bookmarks and topics, which try to
shoehorn lots of functionality into a single command. From a UX
perspective, I think explicit verbs are better than dash prefixed arguments
for controlling mode of operation. e.g. `hg bookmark delete foo` (possibly
bad example due to BC concerns).

If this is the direction we want to go in, I'd be hesitant to implement
[alias] support unless we're sure it would be compatible with proper
sub-commands.


>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -707,6 +707,19 @@ def findcmd(cmd, table, strict=True):
>
>      raise error.UnknownCommand(cmd, allcmds)
>
> +def findcmdspace(name, commands, strict=True):
> +    """Look up (aliases, command table entry) from commands module"""
> +    # TODO: ':' vs '.'
> +    ns, sep, cmd = name.partition(':')
> +    if not sep:
> +        return findcmd(name, commands.table, strict=strict)
> +    table = commands.namespace.get(ns)
> +    if table is None:
> +        raise error.UnknownCommand(name)
> +    # TODO: wrap Ambiguous/UnknownCommand exception to return a full name?
> +    # TODO: might want to require strict=True for namespaced name
> +    return findcmd(cmd, table, strict=strict)
> +
>  def changebranch(ui, repo, revs, label):
>      """ Change the branch name of given revs to label """
>
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -234,7 +234,7 @@ def _runcatch(req):
>          try:
>              cmdargs = fancyopts.fancyopts(req.args[:],
> commands.globalopts, {})
>              cmd = cmdargs[0]
> -            aliases, entry = cmdutil.findcmd(cmd, commands.table, False)
> +            aliases, entry = cmdutil.findcmdspace(cmd, commands,
> strict=False)
>              realcmd = aliases[0]
>          except (error.UnknownCommand, error.AmbiguousCommand,
>                  IndexError, getopt.GetoptError):
> @@ -602,8 +602,8 @@ def _parse(ui, args):
>
>      if args:
>          cmd, args = args[0], args[1:]
> -        aliases, entry = cmdutil.findcmd(cmd, commands.table,
> -                                         ui.configbool("ui", "strict"))
> +        aliases, entry = cmdutil.findcmdspace(cmd, commands,
> +                                              ui.configbool("ui",
> "strict"))
>          cmd = aliases[0]
>          args = aliasargs(entry[0], args)
>          defaults = ui.config("defaults", cmd)
> diff --git a/mercurial/help.py b/mercurial/help.py
> --- a/mercurial/help.py
> +++ b/mercurial/help.py
> @@ -322,8 +322,8 @@ def help_(ui, commands, name, unknowncmd
>
>      def helpcmd(name, subtopic=None):
>          try:
> -            aliases, entry = cmdutil.findcmd(name, commands.table,
> -                                             strict=unknowncmd)
> +            aliases, entry = cmdutil.findcmdspace(name, commands,
> +                                                  strict=unknowncmd)
>          except error.AmbiguousCommand as inst:
>              # py3k fix: except vars can't be used outside the scope of the
>              # except block, nor can be used inside a lambda. python
> issue4617
> @@ -524,7 +524,7 @@ def help_(ui, commands, name, unknowncmd
>              indicateomitted(rst, omitted)
>
>          try:
> -            cmdutil.findcmd(name, commands.table)
> +            cmdutil.findcmdspace(name, commands)
>              rst.append(_("\nuse 'hg help -c %s' to see help for "
>                         "the %s command\n") % (name, name))
>          except error.UnknownCommand:
> diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t
> --- a/tests/test-dispatch.t
> +++ b/tests/test-dispatch.t
> @@ -11,6 +11,24 @@ Redundant options used to crash (issue43
>    $ hg ci -Ama
>    adding a
>
> +Look up command by canonical name:
> +
> +  $ hg :log
> +  changeset:   0:cb9a9f314b8b
> +  tag:         tip
> +  user:        test
> +  date:        Thu Jan 01 00:00:00 1970 +0000
> +  summary:     a
> +
> +  $ hg --config alias.log='log -G' :log
> +  changeset:   0:cb9a9f314b8b
> +  tag:         tip
> +  user:        test
> +  date:        Thu Jan 01 00:00:00 1970 +0000
> +  summary:     a
> +
> +  $ hg --config extensions.purge= purge:purge
> +
>  Missing arg:
>
>    $ hg cat
> diff --git a/tests/test-help.t b/tests/test-help.t
> --- a/tests/test-help.t
> +++ b/tests/test-help.t
> @@ -496,6 +496,14 @@ Test ambiguous command help
>
>    (use 'hg help -v ad' to show built-in aliases and global options)
>
> +  $ hg help :ad
> +  list of commands:
> +
> +   add           add the specified files on the next commit
> +   addremove     add all new files, delete all missing files
> +
> +  (use 'hg help -v :ad' to show built-in aliases and global options)
> +
>  Test command without options
>
>    $ hg help verify
> @@ -670,6 +678,13 @@ Test command without options
>    (use 'hg help' for the full list of commands or 'hg -v' for details)
>    [255]
>
> +Look up command by canonical name:
> +
> +  $ hg --config alias.status=log help status | grep 'alias for'
> +  alias for: hg log
> +  $ hg --config alias.status=log help :status | grep 'alias for'
> +  [1]
> +
>  Typoed command gives suggestion
>    $ hg puls
>    hg: unknown command 'puls'
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - Feb. 25, 2018, 1:42 a.m.
On Sat, 24 Feb 2018 11:39:26 -0800, Gregory Szorc wrote:
> I will say that I believe we had general consensus at a previous sprint to
> implement proper support for sub-commands. This would allow us to implement
> arguments on individual show views. e.g. `hg show work --maxage 14d`. It
> could also be useful for commands like bookmarks and topics, which try to
> shoehorn lots of functionality into a single command. From a UX
> perspective, I think explicit verbs are better than dash prefixed arguments
> for controlling mode of operation. e.g. `hg bookmark delete foo` (possibly
> bad example due to BC concerns).
> 
> If this is the direction we want to go in, I'd be hesitant to implement
> [alias] support unless we're sure it would be compatible with proper
> sub-commands.

Ah, okay, I didn't know the sub-command thingy. Let's revisit after that.
I don't wanna make the dispatcher more complicated.
Gregory Szorc - Feb. 25, 2018, 2:03 a.m.
On Sat, Feb 24, 2018 at 5:42 PM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sat, 24 Feb 2018 11:39:26 -0800, Gregory Szorc wrote:
> > I will say that I believe we had general consensus at a previous sprint
> to
> > implement proper support for sub-commands. This would allow us to
> implement
> > arguments on individual show views. e.g. `hg show work --maxage 14d`. It
> > could also be useful for commands like bookmarks and topics, which try to
> > shoehorn lots of functionality into a single command. From a UX
> > perspective, I think explicit verbs are better than dash prefixed
> arguments
> > for controlling mode of operation. e.g. `hg bookmark delete foo`
> (possibly
> > bad example due to BC concerns).
> >
> > If this is the direction we want to go in, I'd be hesitant to implement
> > [alias] support unless we're sure it would be compatible with proper
> > sub-commands.
>
> Ah, okay, I didn't know the sub-command thingy. Let's revisit after that.
> I don't wanna make the dispatcher more complicated.
>

Well, nobody is actively working on sub-commands. So if you see a way to
make this aliasing thing work such that it is forward compatible with
sub-commands, then my vote is to do it.
Yuya Nishihara - Feb. 25, 2018, 2:37 a.m.
On Sat, 24 Feb 2018 18:03:49 -0800, Gregory Szorc wrote:
> On Sat, Feb 24, 2018 at 5:42 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Sat, 24 Feb 2018 11:39:26 -0800, Gregory Szorc wrote:
> > > I will say that I believe we had general consensus at a previous sprint
> > to
> > > implement proper support for sub-commands. This would allow us to
> > implement
> > > arguments on individual show views. e.g. `hg show work --maxage 14d`. It
> > > could also be useful for commands like bookmarks and topics, which try to
> > > shoehorn lots of functionality into a single command. From a UX
> > > perspective, I think explicit verbs are better than dash prefixed
> > arguments
> > > for controlling mode of operation. e.g. `hg bookmark delete foo`
> > (possibly
> > > bad example due to BC concerns).
> > >
> > > If this is the direction we want to go in, I'd be hesitant to implement
> > > [alias] support unless we're sure it would be compatible with proper
> > > sub-commands.
> >
> > Ah, okay, I didn't know the sub-command thingy. Let's revisit after that.
> > I don't wanna make the dispatcher more complicated.
> >
> 
> Well, nobody is actively working on sub-commands. So if you see a way to
> make this aliasing thing work such that it is forward compatible with
> sub-commands, then my vote is to do it.

I have no idea how sub-commands will be handled internally. I think sub-commands
and alias-bypass are orthogonal feature, but they may have some conflicts at
syntax level.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -707,6 +707,19 @@  def findcmd(cmd, table, strict=True):
 
     raise error.UnknownCommand(cmd, allcmds)
 
+def findcmdspace(name, commands, strict=True):
+    """Look up (aliases, command table entry) from commands module"""
+    # TODO: ':' vs '.'
+    ns, sep, cmd = name.partition(':')
+    if not sep:
+        return findcmd(name, commands.table, strict=strict)
+    table = commands.namespace.get(ns)
+    if table is None:
+        raise error.UnknownCommand(name)
+    # TODO: wrap Ambiguous/UnknownCommand exception to return a full name?
+    # TODO: might want to require strict=True for namespaced name
+    return findcmd(cmd, table, strict=strict)
+
 def changebranch(ui, repo, revs, label):
     """ Change the branch name of given revs to label """
 
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -234,7 +234,7 @@  def _runcatch(req):
         try:
             cmdargs = fancyopts.fancyopts(req.args[:], commands.globalopts, {})
             cmd = cmdargs[0]
-            aliases, entry = cmdutil.findcmd(cmd, commands.table, False)
+            aliases, entry = cmdutil.findcmdspace(cmd, commands, strict=False)
             realcmd = aliases[0]
         except (error.UnknownCommand, error.AmbiguousCommand,
                 IndexError, getopt.GetoptError):
@@ -602,8 +602,8 @@  def _parse(ui, args):
 
     if args:
         cmd, args = args[0], args[1:]
-        aliases, entry = cmdutil.findcmd(cmd, commands.table,
-                                         ui.configbool("ui", "strict"))
+        aliases, entry = cmdutil.findcmdspace(cmd, commands,
+                                              ui.configbool("ui", "strict"))
         cmd = aliases[0]
         args = aliasargs(entry[0], args)
         defaults = ui.config("defaults", cmd)
diff --git a/mercurial/help.py b/mercurial/help.py
--- a/mercurial/help.py
+++ b/mercurial/help.py
@@ -322,8 +322,8 @@  def help_(ui, commands, name, unknowncmd
 
     def helpcmd(name, subtopic=None):
         try:
-            aliases, entry = cmdutil.findcmd(name, commands.table,
-                                             strict=unknowncmd)
+            aliases, entry = cmdutil.findcmdspace(name, commands,
+                                                  strict=unknowncmd)
         except error.AmbiguousCommand as inst:
             # py3k fix: except vars can't be used outside the scope of the
             # except block, nor can be used inside a lambda. python issue4617
@@ -524,7 +524,7 @@  def help_(ui, commands, name, unknowncmd
             indicateomitted(rst, omitted)
 
         try:
-            cmdutil.findcmd(name, commands.table)
+            cmdutil.findcmdspace(name, commands)
             rst.append(_("\nuse 'hg help -c %s' to see help for "
                        "the %s command\n") % (name, name))
         except error.UnknownCommand:
diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t
--- a/tests/test-dispatch.t
+++ b/tests/test-dispatch.t
@@ -11,6 +11,24 @@  Redundant options used to crash (issue43
   $ hg ci -Ama
   adding a
 
+Look up command by canonical name:
+
+  $ hg :log
+  changeset:   0:cb9a9f314b8b
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     a
+  
+  $ hg --config alias.log='log -G' :log
+  changeset:   0:cb9a9f314b8b
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     a
+  
+  $ hg --config extensions.purge= purge:purge
+
 Missing arg:
 
   $ hg cat
diff --git a/tests/test-help.t b/tests/test-help.t
--- a/tests/test-help.t
+++ b/tests/test-help.t
@@ -496,6 +496,14 @@  Test ambiguous command help
   
   (use 'hg help -v ad' to show built-in aliases and global options)
 
+  $ hg help :ad
+  list of commands:
+  
+   add           add the specified files on the next commit
+   addremove     add all new files, delete all missing files
+  
+  (use 'hg help -v :ad' to show built-in aliases and global options)
+
 Test command without options
 
   $ hg help verify
@@ -670,6 +678,13 @@  Test command without options
   (use 'hg help' for the full list of commands or 'hg -v' for details)
   [255]
 
+Look up command by canonical name:
+
+  $ hg --config alias.status=log help status | grep 'alias for'
+  alias for: hg log
+  $ hg --config alias.status=log help :status | grep 'alias for'
+  [1]
+
 Typoed command gives suggestion
   $ hg puls
   hg: unknown command 'puls'