Patchwork Bug 3749 --help does not show non-command help topics

login
register
mail settings
Submitter Ankur Ankan
Date April 17, 2013, 9:47 a.m.
Message ID <CANnmyOuTm+enoJJXd=Pgdok9v+QMe4dG5pXhxdOVT1T02JmmCA@mail.gmail.com>
Download mbox | patch
Permalink /patch/1381/
State Under Review, archived
Headers show

Comments

Ankur Ankan - April 17, 2013, 9:47 a.m.
# HG changeset patch
# User Ankur Ankan <ankurankan@gmail.com>
# Date 1366191853 -19800
#      Wed Apr 17 15:14:13 2013 +0530
# Node ID 3c08507d8eff9c67d376e7283e096e64e794197b
# Parent  a59e575c6ff87b517a8bb167c509a93003bfef53
help: show topic help with --help (issue3749)

+
+      - Open branch heads:
+
+          hg log -r "head() and not closed()"
+
+      - Changesets between tags 1.3 and 1.5 mentioning "bug" that affect
+        "hgext/*":
+
+          hg log -r "1.3::1.5 and keyword(bug) and file('hgext/*')"
+
+      - Changesets committed in May 2008, sorted by user:
+
+          hg log -r "sort(date('May 2008'), user)"
+
+      - Changesets mentioning "bug" or "issue" that are not in a tagged
release:
+
+          hg log -r "(keyword(bug) or keyword(issue)) and not
ancestors(tag())"
+
+Test for Unknown Command when hg revset --help is used
+
+  $ hg revset --help
+  hg: unknown command 'revset'
+  Mercurial Distributed SCM
+
+  basic commands:
+
+   add           add the specified files on the next commit
+   annotate      show changeset information by line for each file
+   clone         make a copy of an existing repository
+   commit        commit the specified files or all outstanding changes
+   diff          diff repository (or selected files)
+   export        dump the header and diffs for one or more changesets
+   forget        forget the specified files on the next commit
+   init          create a new repository in the given directory
+   log           show revision history of entire repository or files
+   merge         merge working directory with another revision
+   pull          pull changes from the specified source
+   push          push changes to the specified destination
+   remove        remove the specified files on the next commit
+   serve         start stand-alone webserver
+   status        show changed files in the working directory
+   summary       summarize working directory state
+   update        update working directory (or switch revisions)
+
+  use "hg help" for the full list of commands or "hg -v" for details
+  [255]

 Test templating help


On Tue, Apr 16, 2013 at 6:48 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Tue, 2013-04-16 at 15:23 +0530, Ankur Ankan wrote:
> > # HG changeset patch
> > # User Ankur Ankan <ankurankan@gmail.com>
> > # Date 1366096607 -19800
> > #      Tue Apr 16 12:46:47 2013 +0530
> > # Node ID 9014d2cc9eee50f8b2369c142ba9041a86b29720
> > # Parent  257afe5489d4a6ebca6a9e13bba1b69a9289b89d
> > help: --help showing non-command help topics
>
> Getting closer.
>
> The commit message should probably be something like:
>
>   help: show topic help with --help (issue3749)
>
>   This should make 'hg --help revsets' work, with enabling 'hg revsets
>   --help'.
>
> Also: this probably needs two short tests added to tests/test-help.t to
> match the above.
>
> > I know this patch doesn't look good. A better solution that I thought
> > was to pass an optional parameter (say ishelp) to findcmd which would
> > be True if --help flag is set. And if ishelp is True findcmd would
> > return the cmd in aliases without checking if it is in the table. The
> > problem here is what should be the value of "entry"?
>
> I think your first patch was closer?
>
> --
> Mathematics is the supreme nostalgia of our time.
>
>
>
Mads Kiilerich - April 17, 2013, 1:43 p.m.
[Resending with proper formatting ... mehope]

Hi

Nice patch.

Anyway, here are some comments that perhaps can make it even better.

On 04/17/2013 11:47 AM, Ankur Ankan wrote:
> # HG changeset patch
> # User Ankur Ankan <ankurankan@gmail.com <mailto:ankurankan@gmail.com>>
> # Date 1366191853 -19800
> #      Wed Apr 17 15:14:13 2013 +0530
> # Node ID 3c08507d8eff9c67d376e7283e096e64e794197b
> # Parent  a59e575c6ff87b517a8bb167c509a93003bfef53
> help: show topic help with --help (issue3749)
>

I would like to see a bit more description here. ( 
http://mercurial.selenic.com/wiki/ContributingChanges#Patch_descriptions )

First of all I would like to see an example of what command line it is 
that will be possible. It is not obvious from the summary line.

I assume that one thing you learned while creating this patch is that it 
is a bit tricky because help topics not are commands and options like 
--help can't take optional parameters. A reviewer might be wondering why 
this can't be implemented the same way as ordinary --help is. Briefly 
explaining the tricky part makes it easier to review ... and to 
understand the change later on.

Have you considered the alternative approach of adding help topics to 
the command list? It with perhaps be interesting with a brief note 
telling why you decided not do that.

> diff -r a59e575c6ff8 -r 3c08507d8eff mercurial/dispatch.py
> --- a/mercurial/dispatch.pyTue Apr 16 15:33:18 2013 +0200
> +++ b/mercurial/dispatch.pyWed Apr 17 15:14:13 2013 +0530
> @@ -635,7 +635,18 @@
>          encoding.fallbackencoding = fallback
>      fullargs = args
> -    cmd, func, args, options, cmdoptions = _parse(lui, args)
> +    try:
> +        cmd, func, args, options, cmdoptions = _parse(lui, args)
> +    except error.UnknownCommand:
> +        if '--help' in fullargs[:-1]:
> +            commands.help_(ui, fullargs[-1])

These two lines seems to assume that the options are given exactly as 
'--help topicname'.

Shouldn't 'hg topicname --help' be supported too? The tests indicate 
that it intentionally shouldn't. I would expect a brief explanation of 
that in the description.

And how about 'hg -v --help urls --debug' ?

> +            return
> +        else:

else is not necessary after an unconditional return. It could be argued 
that being explicit is good ... but usually we like briefness and no 
redundancy even more.

> +            for args in fullargs:
> +                if args[0] != '-':
> +                    cmd = args
> +                    break
> +            raise error.UnknownCommand(cmd)

This is all just to raise the exception again if there was no --help? 
Then just use 'raise'.

>      if options["config"]:
>          raise util.Abort(_("option --config may not be abbreviated!"))
> diff -r a59e575c6ff8 -r 3c08507d8eff tests/test-help.t
> --- a/tests/test-help.tTue Apr 16 15:33:18 2013 +0200
> +++ b/tests/test-help.tWed Apr 17 15:14:13 2013 +0530
> @@ -741,6 +741,419 @@
>        The reserved name "." indicates the working directory parent. If no
>        working directory is checked out, it is equivalent to null. If an
>        uncommitted merge is in progress, "." is the revision of the 
> first parent.
> +Test for topic help with --help

I suggest adding an empty line before the description of the new test - 
that makes the test file more readable.

> +
> +  $ hg --help revset

(revset is one of the bigger help topics. Using a short one like 
'multirevs' makes the test less verbose and more readable.)

> +Test for Unknown Command when hg revset --help is used
> +
> +  $ hg revset --help
> +  hg: unknown command 'revset'
> +  Mercurial Distributed SCM

It might also be an idea to test some other 'evil' command lines such as 
the example I gave above - no matter if you intentionally do support 
them or not.

'hg --help foo' might also be interesting to test ... unless we already 
have test coverage for that.

/Mads

Patch

diff -r a59e575c6ff8 -r 3c08507d8eff mercurial/dispatch.py
--- a/mercurial/dispatch.py Tue Apr 16 15:33:18 2013 +0200
+++ b/mercurial/dispatch.py Wed Apr 17 15:14:13 2013 +0530
@@ -635,7 +635,18 @@ 
         encoding.fallbackencoding = fallback

     fullargs = args
-    cmd, func, args, options, cmdoptions = _parse(lui, args)
+    try:
+        cmd, func, args, options, cmdoptions = _parse(lui, args)
+    except error.UnknownCommand:
+        if '--help' in fullargs[:-1]:
+            commands.help_(ui, fullargs[-1])
+            return
+        else:
+            for args in fullargs:
+                if args[0] != '-':
+                    cmd = args
+                    break
+            raise error.UnknownCommand(cmd)

     if options["config"]:
         raise util.Abort(_("option --config may not be abbreviated!"))
diff -r a59e575c6ff8 -r 3c08507d8eff tests/test-help.t
--- a/tests/test-help.t Tue Apr 16 15:33:18 2013 +0200
+++ b/tests/test-help.t Wed Apr 17 15:14:13 2013 +0530
@@ -741,6 +741,419 @@ 
       The reserved name "." indicates the working directory parent. If no
       working directory is checked out, it is equivalent to null. If an
       uncommitted merge is in progress, "." is the revision of the first
parent.
+Test for topic help with --help
+
+  $ hg --help revset
+  Specifying Revision Sets
+  """"""""""""""""""""""""
+
+      Mercurial supports a functional language for selecting a set of
revisions.
+
+      The language supports a number of predicates which are joined by
infix
+      operators. Parenthesis can be used for grouping.
+
+      Identifiers such as branch names may need quoting with single or
double
+      quotes if they contain characters like "-" or if they match one of
the
+      predefined predicates.
+
+      Special characters can be used in quoted identifiers by escaping
them,
+      e.g., "\n" is interpreted as a newline. To prevent them from being
+      interpreted, strings can be prefixed with "r", e.g. "r'...'".
+
+      There is a single prefix operator:
+
+      "not x"
+        Changesets not in x. Short form is "! x".
+
+      These are the supported infix operators:
+
+      "x::y"
+        A DAG range, meaning all changesets that are descendants of x and
+        ancestors of y, including x and y themselves. If the first
endpoint is
+        left out, this is equivalent to "ancestors(y)", if the second is
left
+        out it is equivalent to "descendants(x)".
+
+        An alternative syntax is "x..y".
+
+      "x:y"
+        All changesets with revision numbers between x and y, both
inclusive.
+        Either endpoint can be left out, they default to 0 and tip.
+
+      "x and y"
+        The intersection of changesets in x and y. Short form is "x & y".
+
+      "x or y"
+        The union of changesets in x and y. There are two alternative short
+        forms: "x | y" and "x + y".
+
+      "x - y"
+        Changesets in x but not in y.
+
+      "x^n"
+        The nth parent of x, n == 0, 1, or 2. For n == 0, x; for n == 1,
the
+        first parent of each changeset in x; for n == 2, the second parent
of
+        changeset in x.
+
+      "x~n"
+        The nth first ancestor of x; "x~0" is x; "x~3" is "x^^^".
+
+      There is a single postfix operator:
+
+      "x^"
+        Equivalent to "x^1", the first parent of each changeset in x.
+
+      The following predicates are supported:
+
+      "adds(pattern)"
+        Changesets that add a file matching pattern.
+
+      "all()"
+        All changesets, the same as "0:tip".
+
+      "ancestor(*changeset)"
+        Greatest common ancestor of the changesets.
+
+        Accepts 0 or more changesets. Will return empty list when passed no
+        args. Greatest common ancestor of a single changeset is that
changeset.
+
+      "ancestors(set)"
+        Changesets that are ancestors of a changeset in set.
+
+      "author(string)"
+        Alias for "user(string)".
+
+      "bisect(string)"
+        Changesets marked in the specified bisect status:
+
+        - "good", "bad", "skip": csets explicitly marked as good/bad/skip
+        - "goods", "bads"      : csets topologically good/bad
+        - "range"              : csets taking part in the bisection
+        - "pruned"             : csets that are goods, bads or skipped
+        - "untested"           : csets whose fate is yet unknown
+        - "ignored"            : csets ignored due to DAG topology
+        - "current"            : the cset currently being bisected
+
+      "bookmark([name])"
+        The named bookmark or all bookmarks.
+
+        If "name" starts with "re:", the remainder of the name is treated
as a
+        regular expression. To match a bookmark that actually starts with
"re:",
+        use the prefix "literal:".
+
+      "branch(string or set)"
+        All changesets belonging to the given branch or the branches of the
+        given changesets.
+
+        If "string" starts with "re:", the remainder of the name is
treated as a
+        regular expression. To match a branch that actually starts with
"re:",
+        use the prefix "literal:".
+
+      "branchpoint()"
+        Changesets with more than one child.
+
+      "bumped()"
+        Mutable changesets marked as successors of public changesets.
+
+        Only non-public and non-obsolete changesets can be "bumped".
+
+      "bundle()"
+        Changesets in the bundle.
+
+        Bundle must be specified by the -R option.
+
+      "children(set)"
+        Child changesets of changesets in set.
+
+      "closed()"
+        Changeset is closed.
+
+      "contains(pattern)"
+        Revision contains a file matching pattern. See "hg help patterns"
for
+        information about file patterns.
+
+      "converted([id])"
+        Changesets converted from the given identifier in the old
repository if
+        present, or all converted changesets if no identifier is specified.
+
+      "date(interval)"
+        Changesets within the interval, see "hg help dates".
+
+      "desc(string)"
+        Search commit message for string. The match is case-insensitive.
+
+      "descendants(set)"
+        Changesets which are descendants of changesets in set.
+
+      "destination([set])"
+        Changesets that were created by a graft, transplant or rebase
operation,
+        with the given revisions specified as the source.  Omitting the
optional
+        set is the same as passing all().
+
+      "divergent()"
+        Final successors of changesets with an alternative set of final
+        successors.
+
+      "draft()"
+        Changeset in draft phase.
+
+      "extinct()"
+        Obsolete changesets with obsolete descendants only.
+
+      "extra(label, [value])"
+        Changesets with the given label in the extra metadata, with the
given
+        optional value.
+
+        If "value" starts with "re:", the remainder of the value is
treated as a
+        regular expression. To match a value that actually starts with
"re:",
+        use the prefix "literal:".
+
+      "file(pattern)"
+        Changesets affecting files matched by pattern.
+
+        For a faster but less accurate result, consider using "filelog()"
+        instead.
+
+      "filelog(pattern)"
+        Changesets connected to the specified filelog.
+
+        For performance reasons, "filelog()" does not show every changeset
that
+        affects the requested file(s). See "hg help log" for details. For a
+        slower, more accurate result, use "file()".
+
+      "first(set, [n])"
+        An alias for limit().
+
+      "follow([file])"
+        An alias for "::." (ancestors of the working copy's first parent).
If a
+        filename is specified, the history of the given file is followed,
+        including copies.
+
+      "grep(regex)"
+        Like "keyword(string)" but accepts a regex. Use "grep(r'...')" to
ensure
+        special escape characters are handled correctly. Unlike
+        "keyword(string)", the match is case-sensitive.
+
+      "head()"
+        Changeset is a named branch head.
+
+      "heads(set)"
+        Members of set with no children in set.
+
+      "hidden()"
+        Hidden changesets.
+
+      "id(string)"
+        Revision non-ambiguously specified by the given hex string prefix.
+
+      "keyword(string)"
+        Search commit message, user name, and names of changed files for
string.
+        The match is case-insensitive.
+
+      "last(set, [n])"
+        Last n members of set, defaulting to 1.
+
+      "limit(set, [n])"
+        First n members of set, defaulting to 1.
+
+      "matching(revision [, field])"
+        Changesets in which a given set of fields match the set of fields
in the
+        selected revision or set.
+
+        To match more than one field pass the list of fields to match
separated
+        by spaces (e.g. "author description").
+
+        Valid fields are most regular revision fields and some special
fields.
+
+        Regular revision fields are "description", "author", "branch",
"date",
+        "files", "phase", "parents", "substate", "user" and "diff". Note
that
+        "author" and "user" are synonyms. "diff" refers to the contents of
the
+        revision. Two revisions matching their "diff" will also match their
+        "files".
+
+        Special fields are "summary" and "metadata": "summary" matches the
first
+        line of the description. "metadata" is equivalent to matching
+        "description user date" (i.e. it matches the main metadata fields).
+
+        "metadata" is the default field which is used when no fields are
+        specified. You can match more than one field at a time.
+
+      "max(set)"
+        Changeset with highest revision number in set.
+
+      "merge()"
+        Changeset is a merge changeset.
+
+      "min(set)"
+        Changeset with lowest revision number in set.
+
+      "modifies(pattern)"
+        Changesets modifying files matched by pattern.
+
+      "obsolete()"
+        Mutable changeset with a newer version.
+
+      "origin([set])"
+        Changesets that were specified as a source for the grafts,
transplants
+        or rebases that created the given revisions.  Omitting the
optional set
+        is the same as passing all().  If a changeset created by these
+        operations is itself specified as a source for one of these
operations,
+        only the source changeset for the first operation is selected.
+
+      "outgoing([path])"
+        Changesets not found in the specified destination repository, or
the
+        default push location.
+
+      "p1([set])"
+        First parent of changesets in set, or the working directory.
+
+      "p2([set])"
+        Second parent of changesets in set, or the working directory.
+
+      "parents([set])"
+        The set of all parents for all changesets in set, or the working
+        directory.
+
+      "present(set)"
+        An empty set, if any revision in set isn't found; otherwise, all
+        revisions in set.
+
+        If any of specified revisions is not present in the local
repository,
+        the query is normally aborted. But this predicate allows the query
to
+        continue even in such cases.
+
+      "public()"
+        Changeset in public phase.
+
+      "remote([id [,path]])"
+        Local revision that corresponds to the given identifier in a remote
+        repository, if present. Here, the '.' identifier is a synonym for
the
+        current local branch.
+
+      "removes(pattern)"
+        Changesets which remove files matching pattern.
+
+      "rev(number)"
+        Revision with the given numeric identifier.
+
+      "reverse(set)"
+        Reverse order of set.
+
+      "roots(set)"
+        Changesets in set with no parent changeset in set.
+
+      "secret()"
+        Changeset in secret phase.
+
+      "sort(set[, [-]key...])"
+        Sort set by keys. The default sort order is ascending, specify a
key as
+        "-key" to sort in descending order.
+
+        The keys can be:
+
+        - "rev" for the revision number,
+        - "branch" for the branch name,
+        - "desc" for the commit message (description),
+        - "user" for user name ("author" can be used as an alias),
+        - "date" for the commit date
+
+      "tag([name])"
+        The specified tag by name, or all tagged revisions if no name is
given.
+
+      "unstable()"
+        Non-obsolete changesets with obsolete ancestors.
+
+      "user(string)"
+        User name contains string. The match is case-insensitive.
+
+        If "string" starts with "re:", the remainder of the string is
treated as
+        a regular expression. To match a user that actually contains
"re:", use
+        the prefix "literal:".
+
+      New predicates (known as "aliases") can be defined, using any
combination
+      of existing predicates or other aliases. An alias definition looks
like:
+
+        <alias> = <definition>
+
+      in the "revsetalias" section of a Mercurial configuration file.
Arguments
+      of the form "$1", "$2", etc. are substituted from the alias into the
+      definition.
+
+      For example,
+
+        [revsetalias]
+        h = heads()
+        d($1) = sort($1, date)
+        rs($1, $2) = reverse(sort($1, $2))
+
+      defines three aliases, "h", "d", and "rs". "rs(0:tip, author)" is
exactly
+      equivalent to "reverse(sort(0:tip, author))".
+
+      Command line equivalents for "hg log":
+
+        -f    ->  ::.
+        -d x  ->  date(x)
+        -k x  ->  keyword(x)
+        -m    ->  merge()
+        -u x  ->  user(x)
+        -b x  ->  branch(x)
+        -P x  ->  !::x
+        -l x  ->  limit(expr, x)
+
+      Some sample queries:
+
+      - Changesets on the default branch:
+
+          hg log -r "branch(default)"
+
+      - Changesets on the default branch since tag 1.5 (excluding merges):
+
+          hg log -r "branch(default) and 1.5:: and not merge()"