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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
775

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
775

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
2664

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
775

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
2664

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
2664

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

mercurial/graphmod.py
211–212

Removed this lines in flight.

226

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 1026

mercurial/cmdutil.py

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) edges = edgefn(type, char, state, rev, parents)
firstedge = next(edges)
width = firstedge[2]
displayer.show(ctx, copies=copies, matchfn=revmatchfn,
_graphwidth=width)
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) for type, char, width, coldata in itertools.chain([firstedge], 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…
for type, char, lines, coldata in edges:
graphmod.ascii(ui, state, type, char, lines, coldata) graphmod.ascii(ui, state, type, char, lines, coldata)
lines = []
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

mercurial/graphmod.py

ecol, next.index(p), color, ecol, next.index(p), color,
bconf.get('width', -1), bconf.get('width', -1),
bconf.get('color', ''))) bconf.get('color', '')))
# Yield and move on # Yield and move on
yield (cur, type, data, (col, color), edges) yield (cur, type, data, (col, color), edges)
seen = next seen = next
def asciiedges(type, char, lines, state, rev, parents): def asciiedges(type, char, state, rev, parents):
"""adds edge info to changelog DAG walk suitable for ascii()""" """adds edge info to changelog DAG walk suitable for ascii()"""
seen = state['seen'] seen = state['seen']
if rev not in seen: if rev not in seen:
seen.append(rev) seen.append(rev)
nodeidx = seen.index(rev) nodeidx = seen.index(rev)
knownparents = [] knownparents = []
newparents = [] newparents = []
for ptype, parent in parents: for ptype, parent in parents:
if parent == rev: if parent == rev:
# self reference (should only be seen in null rev) # self reference (should only be seen in null rev)
continue continue
if parent in seen: if parent in seen:
knownparents.append(parent) knownparents.append(parent)
else: else:
newparents.append(parent) newparents.append(parent)
state['edges'][parent] = state['styles'].get(ptype, '|') state['edges'][parent] = state['styles'].get(ptype, '|')
ncols = len(seen) ncols = len(seen)
width = 1 + ncols * 2
nextseen = seen[:] nextseen = seen[:]
nextseen[nodeidx:nodeidx + 1] = newparents nextseen[nodeidx:nodeidx + 1] = newparents
edges = [(nodeidx, nextseen.index(p)) for p in knownparents] edges = [(nodeidx, nextseen.index(p)) for p in knownparents]
seen[:] = nextseen seen[:] = nextseen
while len(newparents) > 2: while len(newparents) > 2:
# ascii() only knows how to add or remove a single column between two # ascii() only knows how to add or remove a single column between two
# 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)) width += 2
yield (type, char, width, (nodeidx, edges, ncols, nmorecols))
char = '\\' char = '\\'
lines = []
nodeidx += 1 nodeidx += 1
yujaUnsubmitted
Not Done

Removed this lines in flight.

yuja: Removed this `lines` in flight.
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
if nmorecols > 0:
width += 2
# 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, width, (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 _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):

mercurial/templatekw.py

return '@' return '@'
elif ctx.obsolete(): elif ctx.obsolete():
return 'x' return 'x'
elif ctx.closesbranch(): elif ctx.closesbranch():
return '_' return '_'
else: else:
return 'o' return 'o'
@templatekeyword('graphwidth')
def showgraphwidth(repo, ctx, templ, **args):
"""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('index') @templatekeyword('index')
def showindex(**args): def showindex(**args):
"""Integer. The current iteration of the loop. (0 indexed)""" """Integer. The current iteration of the loop. (0 indexed)"""
# just hosts documentation; should be overridden by template mapping # just hosts documentation; should be overridden by template mapping
raise error.Abort(_("can't use index in this context")) raise error.Abort(_("can't use index in this context"))
@templatekeyword('latesttag') @templatekeyword('latesttag')
def showlatesttag(**args): def showlatesttag(**args):
keywords[name] = func keywords[name] = func
@templatekeyword('termwidth') @templatekeyword('termwidth')
def showtermwidth(repo, ctx, templ, **args): def showtermwidth(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('troubles') @templatekeyword('troubles')
def showtroubles(repo, **args): def showtroubles(repo, **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.
"""List of strings. Evolution troubles affecting the changeset. """List of strings. Evolution troubles affecting the changeset.
(DEPRECATED) (DEPRECATED)
""" """
msg = ("'troubles' is deprecated, " msg = ("'troubles' is deprecated, "
"use 'instabilities'") "use 'instabilities'")
repo.ui.deprecwarn(msg, '4.4') repo.ui.deprecwarn(msg, '4.4')

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 ..