This is an archive of the discontinued Mercurial Phabricator instance.

Differential D360

log: add a "graphwidth" template variable
ClosedPublic

Authored by hooper on Aug 11 2017, 8:25 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Commits
rHG6f6c87888b22: log: add a "graphwidth" template variable
Summary

Wrapping text in templates for 'hg log --graph' can't be done very well,
because the template doesn't know how wide the graph drawing is. The edge
drawing function needs to know the number of lines in the template output, so
we need to also determine how wide that drawing would be before we call the
edgefn or evaluate the template.

This patch adds an optional widthfn callback alongside edgefn so that callers
to displaygraph() can enable it to pass the computed graph width into the
template. The new argument is added so as to avoid breaking any extensions
calling the current displaygraph(). The stock asciiedges() gets a stock
asciiwidth() so that we can do something like this:

COLUMNS=10 hg log --graph --template "{fill(desc, termwidth - graphwidth)}"
@  a a a a
|  a a a a
|  a a a a
o    a a a
|\   a a a
| |  a a a
| |  a a a

Using extensions to do this would be relatively complicated due to a lack of
hooks in this area of the code.

In the future it may make sense to have a more generic "textwidth" that tells
you how many columns you can expect to fill without causing the terminal to
wrap your output. I'm not sure there are other situations to motivate this yet,
or if it is entirely feasible.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

hooper created this revision.Aug 11 2017, 8:25 PM
hooper edited the summary of this revision. (Show Details)Aug 11 2017, 8:27 PM
hooper edited the summary of this revision. (Show Details)Aug 11 2017, 8:34 PM
hooper updated this revision to Diff 822.
hooper edited the summary of this revision. (Show Details)Aug 11 2017, 8:35 PM
yuja added a subscriber: yuja.Aug 13 2017, 6:01 AM

The edge
drawing function needs to know the number of lines in the template output, so
we need to also determine how wide that drawing would be before we call the
edgefn or evaluate the template.

Actually asciiedges() doesn't need the lines to be rendered, so we could do
something as follows:

edges = list(edgefn(type, char, state, rev, parents))
graphwidth = asciiwidth(edges)
displayer.show(...)
lines = displayer.hunk.pop()...
for type, char, coldata in edges:
    ascii(ui, state, type, char, lines, coldata)
    lines = []

This would be better in API point of view, but I'm not sure if asciiwidth() could get
simpler by this change.

mercurial/templatekw.py
768

s/graphwidth/showgraphwidth/, and functions should be sorted alphabetically.

hooper added a comment.Aug 14 2017, 1:20 PM
In D360#5639, @yuja wrote:

The edge
drawing function needs to know the number of lines in the template output, so
we need to also determine how wide that drawing would be before we call the
edgefn or evaluate the template.

Actually asciiedges() doesn't need the lines to be rendered, so we could do
something as follows:

edges = list(edgefn(type, char, state, rev, parents))
graphwidth = asciiwidth(edges)
displayer.show(...)
lines = displayer.hunk.pop()...
for type, char, coldata in edges:
    ascii(ui, state, type, char, lines, coldata)
    lines = []

This would be better in API point of view, but I'm not sure if asciiwidth() could get
simpler by this change.

I had avoided this change because it might break extensions with their own edgefns. Do you think that's a concern?

yuja added a comment.Aug 14 2017, 9:25 PM

I had avoided this change because it might break extensions with their own edgefns. Do you think that's a concern?

It's probably okay to change the function interface. I don't think the current edgefn
is any useful for extensions. And I heard no problem after 97cb1aeaca78.

hooper edited the summary of this revision. (Show Details)Aug 15 2017, 2:13 PM
hooper updated this revision to Diff 940.
hooper added inline comments.Aug 15 2017, 2:25 PM
mercurial/templatekw.py
768

Can you explain why it should be renamed? I thought it should mirror termwidth. They both return an integer for computations in templates.

hooper updated this revision to Diff 941.Aug 15 2017, 2:29 PM
yuja added inline comments.Aug 15 2017, 11:44 PM
mercurial/cmdutil.py
2558

Just a nit. Is there any practical benefit to compute edges lazy?
I think edges can be simply converted to a list.

mercurial/templatekw.py
768

That's just a convention of templatekw. termwidth should be renamed too.

hooper marked 2 inline comments as done.Aug 16 2017, 3:12 PM
hooper added inline comments.
mercurial/cmdutil.py
2558

There can be many of them. E.g. in the hg repo it looks like a couple hundred with "hg log --graph -r 'tag()'". It just seems easy enough to hedge against it being expensive, though you might argue premature optimization.

hooper updated this revision to Diff 1009.Aug 16 2017, 3:13 PM
yuja accepted this revision.Aug 17 2017, 9:16 AM

Slightly adjusted the commit message for new version, and queued, thanks.

mercurial/cmdutil.py
2558

Okay, I guess using a list would be slightly faster in common scenario, but I
have no strong opinion about this.

mercurial/graphmod.py
210

Removed this lines in flight.

223

Perhaps the width can be computed from the coldata afterwards, but
that wouldn't make much difference.

width = indentation_level * 2 + 1

# graphmod.ascii()
    indentation_level = max(ncols, ncols + coldiff)
    for (line, logstr) in zip(lines, text):
        ln = "%-*s %s" % (2 * indentation_level, "".join(line), logstr)
This revision is now accepted and ready to land.Aug 17 2017, 9:16 AM
Closed by commit rHG6f6c87888b22: log: add a "graphwidth" template variable (authored by hooper). · Explain WhyAug 17 2017, 9:25 AM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Aug 21 2017, 12:44 PM

it may make sense to have a more generic "textwidth"

Good idea! That seems like what one usually wants. Can we not simply add that (defined as "termwidth - graphwidth")? If we had that, when would one want termwidth or graphwidth themselves?

hooper edited the summary of this revision. (Show Details)Aug 29 2017, 3:26 PM

Revision Contents
Changeset List

Diff 821

hgext/show.py

def showwork(ui, repo, displayer): def showwork(ui, repo, displayer):
"""changesets that aren't finished""" """changesets that aren't finished"""
# TODO support date-based limiting when calling revset. # TODO support date-based limiting when calling revset.
revs = repo.revs('sort(_underway(), topo)') revs = repo.revs('sort(_underway(), topo)')
revdag = graphmod.dagwalker(repo, revs) revdag = graphmod.dagwalker(repo, revs)
ui.setconfig('experimental', 'graphshorten', True) ui.setconfig('experimental', 'graphshorten', True)
cmdutil.displaygraph(ui, repo, revdag, displayer, graphmod.asciiedges) cmdutil.displaygraph(ui, repo, revdag, displayer, graphmod.asciiedges,
widthfn=graphmod.asciiwidth)
def extsetup(ui): def extsetup(ui):
# Alias `hg <prefix><view>` to `hg show <view>`. # Alias `hg <prefix><view>` to `hg show <view>`.
for prefix in ui.configlist('commands', 'show.aliasprefix'): for prefix in ui.configlist('commands', 'show.aliasprefix'):
for view in showview._table: for view in showview._table:
name = '%s%s' % (prefix, view) name = '%s%s' % (prefix, view)
choice, allcommands = cmdutil.findpossible(name, commands.table, choice, allcommands = cmdutil.findpossible(name, commands.table,

mercurial/cmdutil.py

props['ctx'] = ctx props['ctx'] = ctx
props['repo'] = repo props['repo'] = repo
props['ui'] = repo.ui props['ui'] = repo.ui
props['revcache'] = {} props['revcache'] = {}
return templ.render(props) return templ.render(props)
return formatnode return formatnode
def displaygraph(ui, repo, dag, displayer, edgefn, getrenamed=None, def displaygraph(ui, repo, dag, displayer, edgefn, getrenamed=None,
filematcher=None): filematcher=None, widthfn=None):
formatnode = _graphnodeformatter(ui, displayer) formatnode = _graphnodeformatter(ui, displayer)
state = graphmod.asciistate() state = graphmod.asciistate()
styles = state['styles'] styles = state['styles']
# only set graph styling if HGPLAIN is not set. # only set graph styling if HGPLAIN is not set.
if ui.plain('graph'): if ui.plain('graph'):
# set all edge styles to |, the default pre-3.8 behaviour # set all edge styles to |, the default pre-3.8 behaviour
styles.update(dict.fromkeys(styles, '|')) styles.update(dict.fromkeys(styles, '|'))
copies = [] copies = []
for fn in ctx.files(): for fn in ctx.files():
rename = getrenamed(fn, ctx.rev()) rename = getrenamed(fn, ctx.rev())
if rename: if rename:
copies.append((fn, rename[0])) copies.append((fn, rename[0]))
revmatchfn = None revmatchfn = None
if filematcher is not None: if filematcher is not None:
revmatchfn = filematcher(ctx.rev()) revmatchfn = filematcher(ctx.rev())
displayer.show(ctx, copies=copies, matchfn=revmatchfn) graphwidth = None
if widthfn:
graphwidth = widthfn(state, rev, parents)
displayer.show(ctx, copies=copies, matchfn=revmatchfn,
_graphwidth=graphwidth)
lines = displayer.hunk.pop(rev).split('\n') lines = displayer.hunk.pop(rev).split('\n')
if not lines[-1]: if not lines[-1]:
del lines[-1] del lines[-1]
displayer.flush(ctx) displayer.flush(ctx)
edges = edgefn(type, char, lines, state, rev, parents) edges = edgefn(type, char, lines, state, rev, parents)
for type, char, lines, coldata in edges: for type, char, lines, coldata in edges:
yujaUnsubmitted
Not Done

Just a nit. Is there any practical benefit to compute edges lazy?
I think edges can be simply converted to a list.

yuja: Just a nit. Is there any practical benefit to compute edges lazy? I think `edges` can be simply…
hooperAuthorUnsubmitted
Not Done

There can be many of them. E.g. in the hg repo it looks like a couple hundred with "hg log --graph -r 'tag()'". It just seems easy enough to hedge against it being expensive, though you might argue premature optimization.

hooper: There can be many of them. E.g. in the hg repo it looks like a couple hundred with "hg log…
yujaUnsubmitted
Not Done

Okay, I guess using a list would be slightly faster in common scenario, but I
have no strong opinion about this.

yuja: Okay, I guess using a list would be slightly faster in common scenario, but I have no strong…
graphmod.ascii(ui, state, type, char, lines, coldata) graphmod.ascii(ui, state, type, char, lines, coldata)
displayer.close() displayer.close()
def graphlog(ui, repo, pats, opts): def graphlog(ui, repo, pats, opts):
# Parameters are identical to log command ones # Parameters are identical to log command ones
revs, expr, filematcher = getgraphlogrevs(repo, pats, opts) revs, expr, filematcher = getgraphlogrevs(repo, pats, opts)
revdag = graphmod.dagwalker(repo, revs) revdag = graphmod.dagwalker(repo, revs)
getrenamed = None getrenamed = None
if opts.get('copies'): if opts.get('copies'):
endrev = None endrev = None
if opts.get('rev'): if opts.get('rev'):
endrev = scmutil.revrange(repo, opts.get('rev')).max() + 1 endrev = scmutil.revrange(repo, opts.get('rev')).max() + 1
getrenamed = templatekw.getrenamedfn(repo, endrev=endrev) getrenamed = templatekw.getrenamedfn(repo, endrev=endrev)
ui.pager('log') ui.pager('log')
displayer = show_changeset(ui, repo, opts, buffered=True) displayer = show_changeset(ui, repo, opts, buffered=True)
displaygraph(ui, repo, revdag, displayer, graphmod.asciiedges, getrenamed, displaygraph(ui, repo, revdag, displayer, graphmod.asciiedges,
filematcher) getrenamed, filematcher, widthfn=graphmod.asciiwidth)
def checkunsupportedgraphflags(pats, opts): def checkunsupportedgraphflags(pats, opts):
for op in ["newest_first"]: for op in ["newest_first"]:
if op in opts and opts[op]: if op in opts and opts[op]:
raise error.Abort(_("-G/--graph option is incompatible with --%s") raise error.Abort(_("-G/--graph option is incompatible with --%s")
% op.replace("_", "-")) % op.replace("_", "-"))
def graphrevs(repo, nodes, opts): def graphrevs(repo, nodes, opts):

mercurial/commands.py

Returns 0 if there are incoming changes, 1 otherwise. Returns 0 if there are incoming changes, 1 otherwise.
""" """
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
if opts.get('graph'): if opts.get('graph'):
cmdutil.checkunsupportedgraphflags([], opts) cmdutil.checkunsupportedgraphflags([], opts)
def display(other, chlist, displayer): def display(other, chlist, displayer):
revdag = cmdutil.graphrevs(other, chlist, opts) revdag = cmdutil.graphrevs(other, chlist, opts)
cmdutil.displaygraph(ui, repo, revdag, displayer, cmdutil.displaygraph(ui, repo, revdag, displayer,
graphmod.asciiedges) graphmod.asciiedges,
widthfn=graphmod.asciiwidth)
hg._incoming(display, lambda: 1, ui, repo, source, opts, buffered=True) hg._incoming(display, lambda: 1, ui, repo, source, opts, buffered=True)
return 0 return 0
if opts.get('bundle') and opts.get('subrepos'): if opts.get('bundle') and opts.get('subrepos'):
raise error.Abort(_('cannot combine --bundle and --subrepos')) raise error.Abort(_('cannot combine --bundle and --subrepos'))
if opts.get('bookmarks'): if opts.get('bookmarks'):
o, other = hg._outgoing(ui, repo, dest, opts) o, other = hg._outgoing(ui, repo, dest, opts)
if not o: if not o:
cmdutil.outgoinghooks(ui, repo, other, opts, o) cmdutil.outgoinghooks(ui, repo, other, opts, o)
return return
revdag = cmdutil.graphrevs(repo, o, opts) revdag = cmdutil.graphrevs(repo, o, opts)
ui.pager('outgoing') ui.pager('outgoing')
displayer = cmdutil.show_changeset(ui, repo, opts, buffered=True) displayer = cmdutil.show_changeset(ui, repo, opts, buffered=True)
cmdutil.displaygraph(ui, repo, revdag, displayer, graphmod.asciiedges) cmdutil.displaygraph(ui, repo, revdag, displayer, graphmod.asciiedges,
widthfn=graphmod.asciiwidth)
cmdutil.outgoinghooks(ui, repo, other, opts, o) cmdutil.outgoinghooks(ui, repo, other, opts, o)
return 0 return 0
if opts.get('bookmarks'): if opts.get('bookmarks'):
dest = ui.expandpath(dest or 'default-push', dest or 'default') dest = ui.expandpath(dest or 'default-push', dest or 'default')
dest, branches = hg.parseurl(dest, opts.get('branch')) dest, branches = hg.parseurl(dest, opts.get('branch'))
other = hg.peer(repo, opts, dest) other = hg.peer(repo, opts, dest)
if 'bookmarks' not in other.listkeys('namespaces'): if 'bookmarks' not in other.listkeys('namespaces'):

mercurial/graphmod.py

# calls. Nodes with more than two parents break this constraint so we # calls. Nodes with more than two parents break this constraint so we
# introduce intermediate expansion lines to grow the active node list # introduce intermediate expansion lines to grow the active node list
# slowly. # slowly.
edges.append((nodeidx, nodeidx)) edges.append((nodeidx, nodeidx))
edges.append((nodeidx, nodeidx + 1)) edges.append((nodeidx, nodeidx + 1))
nmorecols = 1 nmorecols = 1
yield (type, char, lines, (nodeidx, edges, ncols, nmorecols)) yield (type, char, lines, (nodeidx, edges, ncols, nmorecols))
char = '\\' char = '\\'
lines = [] lines = []
yujaUnsubmitted
Not Done

Removed this lines in flight.

yuja: Removed this `lines` in flight.
nodeidx += 1 nodeidx += 1
ncols += 1 ncols += 1
edges = [] edges = []
del newparents[0] del newparents[0]
if len(newparents) > 0: if len(newparents) > 0:
edges.append((nodeidx, nodeidx)) edges.append((nodeidx, nodeidx))
if len(newparents) > 1: if len(newparents) > 1:
edges.append((nodeidx, nodeidx + 1)) edges.append((nodeidx, nodeidx + 1))
nmorecols = len(nextseen) - ncols nmorecols = len(nextseen) - ncols
# remove current node from edge characters, no longer needed # remove current node from edge characters, no longer needed
state['edges'].pop(rev, None) state['edges'].pop(rev, None)
yield (type, char, lines, (nodeidx, edges, ncols, nmorecols)) yield (type, char, lines, (nodeidx, edges, ncols, nmorecols))
yujaUnsubmitted
Not Done

Perhaps the width can be computed from the coldata afterwards, but
that wouldn't make much difference.

width = indentation_level * 2 + 1

# graphmod.ascii()
    indentation_level = max(ncols, ncols + coldiff)
    for (line, logstr) in zip(lines, text):
        ln = "%-*s %s" % (2 * indentation_level, "".join(line), logstr)
yuja: Perhaps the `width` can be computed from the `coldata` afterwards, but that wouldn't make much…
def asciiwidth(state, rev, parents):
"""returns the width of the graph drawn by ascii() based on asciiedges()"""
seen = state['seen'][:]
if rev not in seen:
seen.append(rev)
columns = len(seen)
# We might be adding columns to handle previously unseen parents, but only
# one of them goes into the graph width for this rev.
newparents = 0
for ptype, parent in parents:
if parent != rev and parent not in seen:
newparents += 1
columns = max(columns, columns + min(2, newparents) - 1)
# Each column is 2 characters, and there's another character of padding.
return 1 + 2 * columns
def _fixlongrightedges(edges): def _fixlongrightedges(edges):
for (i, (start, end)) in enumerate(edges): for (i, (start, end)) in enumerate(edges):
if end > start: if end > start:
edges[i] = (start, end + 1) edges[i] = (start, end + 1)
def _getnodelineedgestail( def _getnodelineedgestail(
echars, idx, pidx, ncols, coldiff, pdiff, fix_tail): echars, idx, pidx, ncols, coldiff, pdiff, fix_tail):
if fix_tail and coldiff == pdiff and coldiff != 0: if fix_tail and coldiff == pdiff and coldiff != 0:

mercurial/templatekw.py

for name, func in registrarobj._table.iteritems(): for name, func in registrarobj._table.iteritems():
keywords[name] = func keywords[name] = func
@templatekeyword('termwidth') @templatekeyword('termwidth')
def termwidth(repo, ctx, templ, **args): def termwidth(repo, ctx, templ, **args):
"""Integer. The width of the current terminal.""" """Integer. The width of the current terminal."""
return repo.ui.termwidth() return repo.ui.termwidth()
@templatekeyword('graphwidth')
def graphwidth(repo, ctx, templ, **args):
yujaUnsubmitted
Done

s/graphwidth/showgraphwidth/, and functions should be sorted alphabetically.

yuja: s/graphwidth/showgraphwidth/, and functions should be sorted alphabetically.
hooperAuthorUnsubmitted
Done

Can you explain why it should be renamed? I thought it should mirror termwidth. They both return an integer for computations in templates.

hooper: Can you explain why it should be renamed? I thought it should mirror termwidth. They both…
yujaUnsubmitted
Not Done

That's just a convention of templatekw. termwidth should be renamed too.

yuja: That's just a convention of templatekw. `termwidth` should be renamed too.
"""Integer. The width of the graph drawn by 'log --graph' or zero."""
# The value args['graphwidth'] will be this function, so we use an internal
# name to pass the value through props into this function.
return args.get('_graphwidth', 0)
@templatekeyword('troubles') @templatekeyword('troubles')
def showtroubles(**args): def showtroubles(**args):
"""List of strings. Evolution troubles affecting the changeset. """List of strings. Evolution troubles affecting the changeset.
(EXPERIMENTAL) (EXPERIMENTAL)
""" """
args = pycompat.byteskwargs(args) args = pycompat.byteskwargs(args)
return showlist('trouble', args['ctx'].troubles(), args) return showlist('trouble', args['ctx'].troubles(), args)
# tell hggettext to extract docstrings from these functions: # tell hggettext to extract docstrings from these functions:
i18nfunctions = keywords.values() i18nfunctions = keywords.values()

tests/test-command-template.t

> [extensions] > [extensions]
> customfunc = $TESTTMP/customfunc.py > customfunc = $TESTTMP/customfunc.py
> EOF > EOF
$ hg log -r . -T "{custom()}\n" --config customfunc.enabled=true $ hg log -r . -T "{custom()}\n" --config customfunc.enabled=true
custom custom
$ cd .. $ cd ..
Test 'graphwidth' in 'hg log' on various topologies. The key here is that the
printed graphwidths 3, 5, 7, etc. should all line up in their respective
columns. We don't care about other aspects of the graph rendering here.
$ hg init graphwidth
$ cd graphwidth
$ wrappabletext="a a a a a a a a a a a a"
$ printf "first\n" > file
$ hg add file
$ hg commit -m "$wrappabletext"
$ printf "first\nsecond\n" > file
$ hg commit -m "$wrappabletext"
$ hg checkout 0
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ printf "third\nfirst\n" > file
$ hg commit -m "$wrappabletext"
created new head
$ hg merge
merging file
0 files updated, 1 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit)
$ hg log --graph -T "{graphwidth}"
@ 3
|
| @ 5
|/
o 3
$ hg commit -m "$wrappabletext"
$ hg log --graph -T "{graphwidth}"
@ 5
|\
| o 5
| |
o | 5
|/
o 3
$ hg checkout 0
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ printf "third\nfirst\nsecond\n" > file
$ hg commit -m "$wrappabletext"
created new head
$ hg log --graph -T "{graphwidth}"
@ 3
|
| o 7
| |\
+---o 7
| |
| o 5
|/
o 3
$ hg log --graph -T "{graphwidth}" -r 3
o 5
|\
~ ~
$ hg log --graph -T "{graphwidth}" -r 1
o 3
|
~
$ hg merge
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit)
$ hg commit -m "$wrappabletext"
$ printf "seventh\n" >> file
$ hg commit -m "$wrappabletext"
$ hg log --graph -T "{graphwidth}"
@ 3
|
o 5
|\
| o 5
| |
o | 7
|\ \
| o | 7
| |/
o / 5
|/
o 3
The point of graphwidth is to allow wrapping that accounts for the space taken
by the graph.
$ COLUMNS=10 hg log --graph -T "{fill(desc, termwidth - graphwidth)}"
@ a a a a
| a a a a
| a a a a
o a a a
|\ a a a
| | a a a
| | a a a
| o a a a
| | a a a
| | a a a
| | a a a
o | a a
|\ \ a a
| | | a a
| | | a a
| | | a a
| | | a a
| o | a a
| |/ a a
| | a a
| | a a
| | a a
| | a a
o | a a a
|/ a a a
| a a a
| a a a
o a a a a
a a a a
a a a a
Something tricky happens when there are elided nodes; the next drawn row of
edges can be more than one column wider, but the graph width only increases by
one column. The remaining columns are added in between the nodes.
$ hg log --graph -T "{graphwidth}" -r "0|2|4|5"
o 5
|\
| \
| :\
o : : 7
:/ /
: o 5
:/
o 3
$ cd ..