[PATCH 2 of 2 V2] show: new extension for displaying various repository data

Gregory Szorc gregory.szorc at gmail.com
Fri Mar 24 22:18:27 EDT 2017


On Wed, Mar 22, 2017 at 7:38 AM, Ryan McElroy <rm at fb.com> wrote:

> On 3/22/17 6:49 AM, Gregory Szorc wrote:
>
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc at gmail.com>
>> # Date 1490165337 25200
>> #      Tue Mar 21 23:48:57 2017 -0700
>> # Node ID 80ca2bee4a06887f918e3328b3f005e4c1cb1ab1
>> # Parent  ae796e23fd42b036352b298f570af8949c2db2d9
>> show: new extension for displaying various repository data
>>
>> Currently, Mercurial has a number of commands to show information. And,
>> there are features coming down the pipe that will introduce more
>> commands for showing information.
>>
>> Currently, when introducing a new class of data or a view that we
>> wish to expose to the user, the strategy is to introduce a new command
>> or overload an existing command, sometimes both. For example, there is
>> a desire to formalize the wip/smartlog/underway/mine functionality that
>> many have devised. There is also a desire to introduce a "topics"
>> concept. Others would like views of "the current stack." In the
>> current model, we'd need a new command for wip/smartlog/etc (that
>> behaves a lot like a pre-defined alias of `hg log`). For topics,
>> we'd likely overload `hg topic[s]` to both display and manipulate
>> topics.
>>
>> Adding new commands for every pre-defined query doesn't scale well
>> and pollutes `hg help`. Overloading commands to perform read-only and
>> write operations is arguably an UX anti-pattern: while having all
>> functionality for a given concept in one command is nice, having a
>> single command doing multiple discrete operations is not. Furthermore,
>> a user may be surprised that a command they thought was read-only
>> actually changes something.
>>
>> We discussed this at the Mercurial 4.0 Sprint in Paris and decided that
>> having a single command where we could hang pre-defined views of
>> various data would be a good idea. Having such a command would:
>>
>> * Help prevent an explosion of new query-related commands
>> * Create a clear separation between read and write operations
>>    (mitigates footguns)
>> * Avoids overloading the meaning of commands that manipulate data
>>    (bookmark, tag, branch, etc) (while we can't take away the
>>    existing behavior for BC reasons, we now won't introduce this
>>    behavior on new commands)
>>
> I'm not convinced that these "footguns" are a bad ui though. I certainly
> would be less comfortable asking people who are used to writing "hg boo" to
> change to "hg show bookmarks" (see comment on
>
> * Allows users to discover informational views more easily by
>>    aggregating them in a single location
>> * Lowers the barrier to creating the new views (since the barrier
>>    to creating a top-level command is relatively high)
>>
>> So, this commit introduces the `hg show` command via the "show"
>> extension. This command accepts a positional argument of the
>> "view" to show. New views can be registered with a decorator. To
>> prove it works, we implement the "bookmarks" view, which shows a
>> table of bookmarks and their associated nodes.
>>
>> We introduce a new style to hold everything used by `hg show`.
>>
> Ah, this answers some of my inline questions below about the new styles.
>
>
>> For our initial bookmarks view, the output varies from `hg bookmarks`:
>>
>> * Padding is performed in the template itself as opposed to Python
>> * Revision integers are not shown
>> * shortest() is used to display a 5 character node by default (as
>>    opposed to static 12 characters)
>>
>> I chose to implement the "bookmarks" view first because it is simple
>> and shouldn't invite too much bikeshedding that detracts from the
>> evaluation of `hg show` itself. But there is an important point
>> to consider: we now have 2 ways to show a list of bookmarks. I'm not
>> a fan of introducing multiple ways to do very similar things. So it
>> might be worth discussing how we wish to tackle this issue for
>> bookmarks, tags, branches, MQ series, etc.
>>
> I'm pretty concerned about this too. I don't have an answer right now but
> I'll think about it.
>
>
>> I also made the choice of explicitly declaring the default show
>> template not part of the standard BC guarantees. History has shown
>> that we make mistakes and poor choices with output formatting but
>> can't fix these mistakes later because random tools are parsing
>> output and we don't want to break these tools. Optimizing for human
>> consumption is one of my goals for `hg show`. So, by not covering
>> the formatting as part of BC, the barrier to future change is much
>> lower and humans benefit.
>>
> This is awesome. See inline comments for additional idea. First
> "porcelain" hg command :-p.
>
>
>> There are some improvements that can be made to formatting. For
>> example, we don't yet use label() in the templates. We obviously
>> want this for color. But I'm not sure if we should reuse the existing
>> log.* labels or invent new ones. I figure we can punt that to a
>> follow-up.
>>
> Seems that this patch is good for collecting shed colors, but this would
> probably want to be broken up into a series before it actually goes in,
> right?


I figured this patch would land as a big chunk and we'd iterate using the
normal process. I think trying to split the initial landing into multiple
parts is a bit difficult because there's a lot to take in. But if people
want me to split it, I can.


>
>
>
>> At the aforementioned Sprint, we discussed and discarded various
>> alternatives to `hg show`.
>>
>> We considered making `hg log <view>` perform this behavior. The main
>> reason we can't do this is because a positional argument to `hg log`
>> can be a file path and if there is a conflict between a path name and
>> a view name, behavior is ambiguous. We could have introduced
>> `hg log --view` or similar, but we felt that required too much typing
>> (we don't want to require a command flag to show a view) and wasn't
>> very discoverable. Furthermore, `hg log` is optimized for showing
>> changelog data and there are things that `hg display` could display
>> that aren't changelog centric.
>>
>> There were concerns about using "show" as the command name.
>>
>> Some users already have a "show" alias that is similar to `hg export`.
>>
>> There were also concerns that Git users adapted to `git show` would
>> be confused by `hg show`'s different behavior. The main difference
>> here is `git show` prints an `hg export` like view of the current
>> commit by default and `hg show` requires an argument. `git show`
>> can also display any Git object. `git show` does not support
>> displaying more complex views: just single objects. If we
>> implemented `hg show <hash>` or `hg show <identifier>`, `hg show`
>> would be a superset of `git show`. Although, I'm hesitant to do that
>> at this time because I view `hg show` as a higher-level querying
>> command and there are namespace collisions between valid identifiers
>> and registered views.
>>
>> There is also a prefix collision with `hg showconfig`, which is an
>> alias of `hg config`.
>>
> I'm not too concerned about this. As I said in the other thread, fb has a
> show extension as well. It's basically a thin wrapper around `hg log -vpr`
> that allows a few additional flags and has a default '.' parameter:
> https://bitbucket.org/facebook/hg-experimental/src/default/
> hgext3rd/show.py
>
>
>> We also considered `hg view`, but that is already used by the "hgk"
>> extension.
>>
> Meh. Can we deprecate hgk? (Serious question: I don't know how widely it's
> used. We seem to have it installed at FB but it seems totally broken and
> nobody has ever complained so it seems that out of thousands of users
> nobody has ever noticed it's there).
>

I'm not opposed to renaming this `hg view`. But I'd prefer that be done as
a follow-up because I want to get this "framework" landed!


>
>
>> `hg display` was also proposed at one point. It has a prefix collision
>> with `hg diff`. General consensus was "show" or "view" are the best
>> verbs. And since "view" was taken, "show" was chosen.
>>
> Seems reasonable. Feel free to ignore my bikeshedding above. I'm okay with
> show overall.
>
>
>> diff --git a/contrib/wix/templates.wxs b/contrib/wix/templates.wxs
>> --- a/contrib/wix/templates.wxs
>> +++ b/contrib/wix/templates.wxs
>> @@ -32,6 +32,7 @@
>>             <File Name="map-cmdline.changelog" KeyPath="yes" />
>>             <File Name="map-cmdline.compact" />
>>             <File Name="map-cmdline.default" />
>> +          <File Name="map-cmdline.show" />
>>             <File Name="map-cmdline.bisect" />
>>             <File Name="map-cmdline.xml" />
>>             <File Name="map-cmdline.status" />
>>
> What are these files? I'm not familiar with what's going on here.


The map-cmdline.* files are various styles that can be used with -T. I
invented a new style for this command. I'm not sure that if this is proper.
Yuya already directed me to not have various template names conflict with
built-in templates (hence the "show*" prefixing). So perhaps I could roll
these templates into map-cmdline.default?


>
>
> diff --git a/hgext/show.py b/hgext/show.py
>> new file mode 100644
>> --- /dev/null
>> +++ b/hgext/show.py
>> @@ -0,0 +1,115 @@
>> +# show.py - Extension implementing `hg show`
>> +#
>> +# Copyright 2017 Gregory Szorc <gregory.szorc at gmail.com>
>> +#
>> +# This software may be used and distributed according to the terms of the
>> +# GNU General Public License version 2 or any later version.
>> +
>> +"""unified command to show various repository information (EXPERIMENTAL)
>> +
>> +This extension provides the :hg:`show` command, which provides a central
>> +command for displaying commonly-accessed repository data and views of
>> that
>> +data.
>> +"""
>> +
>> +from __future__ import absolute_import
>> +
>> +from mercurial.i18n import _
>> +from mercurial import (
>> +    cmdutil,
>> +    commands,
>> +    error,
>> +    registrar,
>> +)
>> +
>> +# Note for extension authors: ONLY specify testedwith =
>> 'ships-with-hg-core' for
>> +# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
>> +# be specifying the version(s) of Mercurial they are tested with, or
>> +# leave the attribute unspecified.
>> +testedwith = 'ships-with-hg-core'
>> +
>> +cmdtable = {}
>> +command = cmdutil.command(cmdtable)
>> +
>> +class showcmdfunc(registrar._funcregistrarbase):
>> +    """Register a function to be invoked for an `hg show <thing>`."""
>> +    _docformat = '%s -- %s'
>>
> Unused variable?
>
> +
>> +    def _extrasetup(self, name, func, fmtopic=None):
>> +        func._fmtopic = fmtopic
>>
> Docstring please -- I have no idea what this is for. Also, it's unused?
>

This stuff is used. I'll add comments.


>
> +
>> +showview = showcmdfunc()
>> +
>> + at command('show', commands.formatteropts, _('[VIEW]'))
>> +def show(ui, repo, view=None, template=None):
>> +    """show various repository information
>> +
>> +    A requested view of repository data is displayed.
>> +
>> +    If no view is requested, the list of available views is shown.
>>
> (and we abort)
>
> +
>> +    .. note::
>> +
>> +       The default output from this command is not covered under
>> Mercurial's
>> +       default backwards-compatible mechanism (which puts an emphasis on
>> +       not changing behavior). This means output from this command may
>> change
>> +       in any future release. However, the values fed to the formatter
>> are
>> +       covered under the default backwards-compatible mechanism.
>> +
>> +       Automated consumers of this command should specify an explicit
>> template
>> +       via ``-T/--template`` to guarantee output is stable.
>>
> YES! I love it.
>
> We should consider aborting if HGPLAIN=1 and no template is specified, in
> fact!
>
> +
>> +    List of available views:
>> +
>> +    """
>> +    views = showview._table
>> +
>> +    if not view:
>> +        ui.write(_('available views:\n'))
>> +        ui.write('\n')
>>
> Should this use a formatter? I could imagine wanting to consume available
> views in json, for example.
>

Probably. But I'm not sure how to do it though (I'm not sure how to get the
header and footer printed while also printing multiple items). I've added a
TODO in the next version.


>
> +
>> +        for name, func in sorted(views.items()):
>> +            ui.write(_('%s\n') % func.__doc__)
>>
> The entire docstring seems like too much. We should probably have only the
> first line of the docstring here. Also, you don't use the name. Shouldn't
> the name be here as well?
>

But it's not the full docstring: that _docformat variable does magic ;)


>
> Nitpick: I don't think it's useful to internationalize '%s\n'.
>
> +
>> +        ui.write('\n')
>> +        raise error.Abort(_('no view requested'),
>> +                          hint=_('use `hg show <view>` to choose a
>> view'))
>>
> This hint format is different that what I usually see in hg. Shouldn't it
> be 'hg show VIEW' so it's consistent with our other command help output?
> Also, we tend to not use backticks, right?
>
> +
>> +    if view not in views:
>> +        raise error.Abort(_('unknown view: %s') % view,
>> +                          hint=_('run `hg show` to see available views'))
>>
> We should probably do prefix matching to be consistent with other commands
> in hg (and to help out lazy typers).
>

Agreed. I'll add a TODO (I don't want to bloat scope too much).


>
> +
>> +    template = template or 'show'
>> +    fmtopic = 'show%s' % views[view]._fmtopic
>> +
>> +    ui.pager('show')
>>
> Why is this down here but not up where we print a list of views?
>

Will fix.


>
> +    with ui.formatter(fmtopic, {'template': template}) as fm:
>> +        return views[view](ui, repo, fm)
>> +
>> + at showview('bookmarks', fmtopic='bookmarks')
>> +def showbookmarks(ui, repo, fm):
>> +    """bookmarks and their associated changeset"""
>> +    marks = repo._bookmarks
>> +    if not len(marks):
>> +        ui.write(_('(no bookmarks set)\n'))
>> +        return
>> +
>> +    active = repo._activebookmark
>> +    longest = max(len(b) for b in marks)
>> +
>> +    for bm, node in sorted(marks.items()):
>> +        fm.startitem()
>> +        fm.context(ctx=repo[node])
>> +        fm.write('bookmark', '%s', bm)
>> +        fm.write('node', fm.hexfunc(node), fm.hexfunc(node))
>> +        fm.data(active=bm == active,
>> +                _longestlen=longest)
>>
> _longest should probably be _longestname, because shortened nodes can also
> have interesting length properties -- and we may want to pass _longestnode
> in the future if there is more data we want to show here and we want to pad
> nicely.
>

Agreed. Although the longest shorted node is a bit difficult to implement
because AFAICT there is no obvious function to call. I'll TODO that one.


>
> +
>> +# Adjust the docstring of the show command so it shows all registered
>> views.
>> +# This is a bit hacky because it runs at the end of module load. When
>> moved
>> +# into core or when another extension wants to provide a view, we'll need
>> +# to do this more robustly.
>> +longest = max(map(len, showview._table.keys()))
>> +for key in sorted(showview._table.keys()):
>> +    cmdtable['show'][0].__doc__ += ' %s   %s\n' % (
>> +        key.ljust(longest), showview._table[key]._origdoc)
>>
> Yuck! Surely we can do better with a formatter and padding? At the very
> least, we can just compute this when we need to print it out and not at
> module load time. But it would be better to use the formatter if possible,
> then we get things like JSON for free.
>

Tell me about it. This ideally wants some kind of hook in the help
mechanism. I didn't want to bloat scope and implement that at this
juncture. I've added a TODO.


>
> diff --git a/mercurial/templates/map-cmdline.show
>> b/mercurial/templates/map-cmdline.show
>> new file mode 100644
>> --- /dev/null
>> +++ b/mercurial/templates/map-cmdline.show
>> @@ -0,0 +1,2 @@
>> +# TODO add label() once we figure out which namespace the labels belong
>> on.
>> +showbookmarks = '{if(active, "*", " ")} {pad(bookmark, _longestlen +
>> 4)}{shortest(node, 5)}\n'
>>
> What is this file?
>

Our custom style (which may or may not be needed).


>
> diff --git a/tests/test-command-template.t b/tests/test-command-template.t
>> --- a/tests/test-command-template.t
>> +++ b/tests/test-command-template.t
>> @@ -1108,11 +1108,11 @@ Error if no style:
>>       $ hg log --style notexist
>>     abort: style 'notexist' not found
>> -  (available styles: bisect, changelog, compact, default, phases,
>> status, xml)
>> +  (available styles: bisect, changelog, compact, default, phases, show,
>> status, xml)
>>     [255]
>>       $ hg log -T list
>> -  available styles: bisect, changelog, compact, default, phases, status,
>> xml
>> +  available styles: bisect, changelog, compact, default, phases, show,
>> status, xml
>>     abort: specify a template
>>     [255]
>>
> I'm confused by these test changes. Why does show show up here?


Because this is listing available styles (the map-cmdline.* files).


>
>
>   diff --git a/tests/test-log.t b/tests/test-log.t
>> --- a/tests/test-log.t
>> +++ b/tests/test-log.t
>> @@ -148,7 +148,7 @@ log on directory
>>       $ hg log -f -l1 --style something
>>     abort: style 'something' not found
>> -  (available styles: bisect, changelog, compact, default, phases,
>> status, xml)
>> +  (available styles: bisect, changelog, compact, default, phases, show,
>> status, xml)
>>     [255]
>>     -f, phases style
>> diff --git a/tests/test-show.t b/tests/test-show.t
>> new file mode 100644
>> --- /dev/null
>> +++ b/tests/test-show.t
>> @@ -0,0 +1,111 @@
>> +  $ cat >> $HGRCPATH << EOF
>> +  > [extensions]
>> +  > show =
>> +  > EOF
>> +
>> +No arguments shows available views
>> +
>> +  $ hg init empty
>> +  $ cd empty
>> +  $ hg show
>> +  available views:
>> +
>> +  bookmarks -- bookmarks and their associated changeset
>> +
>> +  abort: no view requested
>> +  (use `hg show <view>` to choose a view)
>> +  [255]
>> +
>> +`hg help show` prints available views
>> +
>> +  $ hg help show
>> +  hg show [VIEW]
>>
> If we abort otherwise, I wouldn't say that VIEW is optional. I guess I'm
> against the abort overall, though...
>
>
> +
>> +  show various repository information
>> +
>> +      A requested view of repository data is displayed.
>> +
>> +      If no view is requested, the list of available views is shown.
>> +
>> +      Note:
>> +         The default output from this command is not covered under
>> Mercurial's
>> +         default backwards-compatible mechanism (which puts an emphasis
>> on not
>> +         changing behavior). This means output from this command may
>> change in
>> +         any future release. However, the values fed to the formatter are
>> +         covered under the default backwards-compatible mechanism.
>> +
>> +         Automated consumers of this command should specify an explicit
>> template
>> +         via "-T/--template" to guarantee output is stable.
>> +
>> +      List of available views:
>> +
>> +       bookmarks   bookmarks and their associated changeset
>> +
>> +  (use 'hg help -e show' to show help for the show extension)
>> +
>> +  options:
>> +
>> +  (some details hidden, use --verbose to show complete help)
>> +
>> +Unknown view prints error
>> +
>> +  $ hg show badview
>> +  abort: unknown view: badview
>> +  (run `hg show` to see available views)
>> +  [255]
>> +
>> +  $ cd ..
>> +
>> +bookmarks view with no bookmarks prints empty message
>> +
>> +  $ hg init books
>> +  $ cd books
>> +  $ touch f0
>> +  $ hg -q commit -A -m initial
>> +
>> +  $ hg show bookmarks
>> +  (no bookmarks set)
>> +
>> +bookmarks view shows bookmarks in an aligned table
>> +
>> +  $ echo book1 > f0
>> +  $ hg commit -m 'commit for book1'
>> +  $ echo book2 > f0
>> +  $ hg commit -m 'commit for book2'
>> +
>> +  $ hg bookmark -r 1 book1
>> +  $ hg bookmark a-longer-bookmark
>> +
>> +  $ hg show bookmarks
>> +  * a-longer-bookmark    7b570
>> +    book1                b757f
>> +
>> +Bookmarks plain view
>> +
>> +  $ HGPLAIN= hg show bookmarks
>> +  * a-longer-bookmark    7b570
>> +    book1                b757f
>>
> Let's make this abort instead.
>
>
> +
>> +A custom bookmarks template works
>> +
>> +  $ hg show bookmarks -T '{node} {bookmark} {active}\n'
>> +  7b5709ab64cbc34da9b4367b64afff47f2c4ee83 a-longer-bookmark True
>> +  b757f780b8ffd71267c6ccb32e0882d9d32a8cc0 book1 False
>> +
>> +bookmarks JSON works
>> +
>> +  $ hg show bookmarks -T json
>> +  [
>> +   {
>> +    "active": true,
>> +    "bookmark": "a-longer-bookmark",
>> +    "node": "7b5709ab64cbc34da9b4367b64afff47f2c4ee83"
>> +   },
>> +   {
>> +    "active": false,
>> +    "bookmark": "book1",
>> +    "node": "b757f780b8ffd71267c6ccb32e0882d9d32a8cc0"
>> +   }
>> +  ]
>> +
>> +  $ cd ..
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170324/fdba39bc/attachment-0001.html>


More information about the Mercurial-devel mailing list