This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4363

log: respect graphshorten on terminal nodes (collapsing o-~ to just o~)
ClosedPublic

Authored by spectral on Aug 23 2018, 4:33 PM.

Details

Reviewers
indygreg
martinvonz
pulkit
Group Reviewers
hg-reviewers
Commits
rHG3c4b2e880273: log: respect graphshorten on terminal nodes (collapsing o-~ to just o~)
Summary

Internally we have a custom template that's inspired by ones that we have seen
in the community. Normally, this looks something like:

o  0834ec17 spectral tip
|  crecord: support x to toggle single, X to toggle a range
o  ee932990 spectral @
|  filemerge: allow specifying $hgeditor as merge-tools.X.executable
@  66f04611 matt_harbison
|  cext: fix truncation warnings in revlog on Windows
o  42cc76d0 matt_harbison
|  cext: fix revlog compiler error on Windows
~
o  bd63ada7 stable boris
|  phases: drop dead code in `newheads`
~

With graphshorten on, and the descriptions of the public nodes hidden, it looks
like this, note that the commits right before the ~ are still "full height":

o  0834ec17 spectral tip
|  crecord: support x to toggle single, X to toggle a range
o  ee932990 spectral @
|  filemerge: allow specifying $hgeditor as merge-tools.X.executable
@  66f04611 matt_harbison
o  42cc76d0 matt_harbison
|
~
o  bd63ada7 stable boris
|
~

This patch makes them look like this, removing the | but keeping the ~:

o  0834ec17 spectral tip
|  crecord: support x to toggle single, X to toggle a range
o  ee932990 spectral @
|  filemerge: allow specifying $hgeditor as merge-tools.X.executable
@  66f04611 matt_harbison
o  42cc76d0 matt_harbison
~
o  bd63ada7 stable boris
~

This originally removed the ~s entirely, but this was determined to be too much
information loss and potentially confusing. This would have looked like the
following (note that the last commit is on a different branch than all of the
ones above it, and they are *not* linearly related):

o  0834ec17 spectral tip
|  crecord: support x to toggle single, X to toggle a range
o  ee932990 spectral @
|  filemerge: allow specifying $hgeditor as merge-tools.X.executable
@  66f04611 matt_harbison
o  42cc76d0 matt_harbison
o  bd63ada7 stable boris

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

spectral created this revision.Aug 23 2018, 4:33 PM
spectral updated this revision to Diff 10539.Aug 23 2018, 4:37 PM
spectral added a reviewer: indygreg.Aug 23 2018, 4:38 PM

Since this touches hg show work test output, I've added indygreg as an explicit reviewer.

martinvonz added a subscriber: martinvonz.Aug 23 2018, 5:32 PM

Can we have a test for this?

mercurial/graphmod.py
301–302

Since it's just the single | line that goes away with this patch, why does minlines change by 2 and not by 1?

martinvonz added a comment.Aug 23 2018, 5:48 PM
In D4363#67012, @martinvonz wrote:

Can we have a test for this?

Oh, sorry, there are updated tests (my browser tab was stale when I reviewed this). That also explains why two minlines changed by 2: the ~ is also removed, which I didn't expect. I think I would like to have the ~ in there. Without them, it may look like two independent branches come in sequence, no?

spectral added a comment.Aug 23 2018, 7:48 PM
In D4363#67015, @martinvonz wrote:
In D4363#67012, @martinvonz wrote:

Can we have a test for this?

Oh, sorry, there are updated tests (my browser tab was stale when I reviewed this). That also explains why two minlines changed by 2: the ~ is also removed, which I didn't expect. I think I would like to have the ~ in there. Without them, it may look like two independent branches come in sequence, no?

Yes, it might. I kinda went back and forth on this, as you probably saw on the FR from our users; I came to the conclusion that those using graphshorten are already losing : vs | distinctions, so seems to already be opting in to potentially misleading graphs. When I realized that hg show work also used graphshorten and this wasn't an explicit choice by the user, I was less sure on this and requested input from indygreg. If we agree that we need the ~ by default, I can easily add it back. Having never used hg show work, doing most of my changes in a repo where we don't really have branches that would cause this to be confusing, and that we have the branch name in the compact log format that I showed in the commit description, I don't think I'm a good judge of the importance of the ~ in these cases :)

martinvonz added a comment.Aug 23 2018, 7:55 PM
In D4363#67016, @spectral wrote:
In D4363#67015, @martinvonz wrote:
In D4363#67012, @martinvonz wrote:

Can we have a test for this?

Oh, sorry, there are updated tests (my browser tab was stale when I reviewed this). That also explains why two minlines changed by 2: the ~ is also removed, which I didn't expect. I think I would like to have the ~ in there. Without them, it may look like two independent branches come in sequence, no?

Yes, it might. I kinda went back and forth on this, as you probably saw on the FR from our users; I came to the conclusion that those using graphshorten are already losing : vs | distinctions, so seems to already be opting in to potentially misleading graphs. When I realized that hg show work also used graphshorten and this wasn't an explicit choice by the user, I was less sure on this and requested input from indygreg. If we agree that we need the ~ by default, I can easily add it back. Having never used hg show work, doing most of my changes in a repo where we don't really have branches that would cause this to be confusing, and that we have the branch name in the compact log format that I showed in the commit description, I don't think I'm a good judge of the importance of the ~ in these cases :)

I feel pretty strongly that making a non-linear graph appear linear is much more misleading than eliding intermediate nodes, so I'd really like the ~ back.

spectral edited the summary of this revision. (Show Details)Aug 23 2018, 8:50 PM
spectral retitled this revision from log: respect graphshorten on terminal nodes (collapsing o-~ to just o) to log: respect graphshorten on terminal nodes (collapsing o-~ to just o~).
spectral updated this revision to Diff 10540.
spectral added a comment.Aug 23 2018, 8:51 PM
In D4363#67017, @martinvonz wrote:
In D4363#67016, @spectral wrote:
In D4363#67015, @martinvonz wrote:
In D4363#67012, @martinvonz wrote:

Can we have a test for this?

Oh, sorry, there are updated tests (my browser tab was stale when I reviewed this). That also explains why two minlines changed by 2: the ~ is also removed, which I didn't expect. I think I would like to have the ~ in there. Without them, it may look like two independent branches come in sequence, no?

Yes, it might. I kinda went back and forth on this, as you probably saw on the FR from our users; I came to the conclusion that those using graphshorten are already losing : vs | distinctions, so seems to already be opting in to potentially misleading graphs. When I realized that hg show work also used graphshorten and this wasn't an explicit choice by the user, I was less sure on this and requested input from indygreg. If we agree that we need the ~ by default, I can easily add it back. Having never used hg show work, doing most of my changes in a repo where we don't really have branches that would cause this to be confusing, and that we have the branch name in the compact log format that I showed in the commit description, I don't think I'm a good judge of the importance of the ~ in these cases :)

I feel pretty strongly that making a non-linear graph appear linear is much more misleading than eliding intermediate nodes, so I'd really like the ~ back.

I tried to find a sane way of determining if we were at the end of the graph and eliding that ~, at least, but couldn't figure it out. Done: all of the ~s are back.

spectral updated this revision to Diff 10541.Aug 23 2018, 9:12 PM
martinvonz accepted this revision.Aug 24 2018, 1:11 PM

LGTM, but I'll let a non-Googler queue it in case it seems controversial to others.

pulkit accepted this revision.Aug 27 2018, 9:42 AM
Closed by commit rHG3c4b2e880273: log: respect graphshorten on terminal nodes (collapsing o-~ to just o~) (authored by spectral). · Explain WhyAug 27 2018, 10:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/graphmod.py (9 lines)
Mtests/test-logexchange.t (2 lines)
Mtests/test-show-work.t (7 lines)

Diff 10585

mercurial/graphmod.py

else: else:
line.extend([' ', ' ']) line.extend([' ', ' '])
# all edges to the right of the current node # all edges to the right of the current node
remainder = ncols - idx - 1 remainder = ncols - idx - 1
if remainder > 0: if remainder > 0:
line.extend(echars[-(remainder * 2):]) line.extend(echars[-(remainder * 2):])
return line return line
def _drawendinglines(lines, extra, edgemap, seen): def _drawendinglines(lines, extra, edgemap, seen, state):
"""Draw ending lines for missing parent edges """Draw ending lines for missing parent edges
None indicates an edge that ends at between this node and the next None indicates an edge that ends at between this node and the next
Replace with a short line ending in ~ and add / lines to any edges to Replace with a short line ending in ~ and add / lines to any edges to
the right. the right.
""" """
if None not in edgemap.values(): if None not in edgemap.values():
return return
# Check for more edges to the right of our ending edges. # Check for more edges to the right of our ending edges.
# We need enough space to draw adjustment lines for these. # We need enough space to draw adjustment lines for these.
edgechars = extra[::2] edgechars = extra[::2]
while edgechars and edgechars[-1] is None: while edgechars and edgechars[-1] is None:
edgechars.pop() edgechars.pop()
shift_size = max((edgechars.count(None) * 2) - 1, 0) shift_size = max((edgechars.count(None) * 2) - 1, 0)
while len(lines) < 3 + shift_size: minlines = 3 if not state['graphshorten'] else 2
while len(lines) < minlines + shift_size:
martinvonzUnsubmitted
Not Done

Since it's just the single | line that goes away with this patch, why does minlines change by 2 and not by 1?

martinvonz: Since it's just the single `|` line that goes away with this patch, why does `minlines` change…
lines.append(extra[:]) lines.append(extra[:])
if shift_size: if shift_size:
empties = [] empties = []
toshift = [] toshift = []
first_empty = extra.index(None) first_empty = extra.index(None)
for i, c in enumerate(extra[first_empty::2], first_empty // 2): for i, c in enumerate(extra[first_empty::2], first_empty // 2):
if c is None: if c is None:
empties.append(i * 2) empties.append(i * 2)
else: else:
toshift.append(i * 2) toshift.append(i * 2)
targets = list(range(first_empty, first_empty + len(toshift) * 2, 2)) targets = list(range(first_empty, first_empty + len(toshift) * 2, 2))
positions = toshift[:] positions = toshift[:]
for line in lines[-shift_size:]: for line in lines[-shift_size:]:
line[first_empty:] = [' '] * (len(line) - first_empty) line[first_empty:] = [' '] * (len(line) - first_empty)
for i in range(len(positions)): for i in range(len(positions)):
pos = positions[i] - 1 pos = positions[i] - 1
positions[i] = max(pos, targets[i]) positions[i] = max(pos, targets[i])
line[pos] = '/' if pos > targets[i] else extra[toshift[i]] line[pos] = '/' if pos > targets[i] else extra[toshift[i]]
map = {1: '|', 2: '~'} map = {1: '|', 2: '~'} if not state['graphshorten'] else {1: '~'}
for i, line in enumerate(lines): for i, line in enumerate(lines):
if None not in line: if None not in line:
continue continue
line[:] = [c or map.get(i, ' ') for c in line] line[:] = [c or map.get(i, ' ') for c in line]
# remove edges that ended # remove edges that ended
remove = [p for p, c in edgemap.items() if c is None] remove = [p for p, c in edgemap.items() if c is None]
for parent in remove: for parent in remove:
# make sure that there are as many graph lines as there are # make sure that there are as many graph lines as there are
# log strings # log strings
extra_interline = echars[:(ncols + coldiff) * 2] extra_interline = echars[:(ncols + coldiff) * 2]
if len(lines) < len(text): if len(lines) < len(text):
while len(lines) < len(text): while len(lines) < len(text):
lines.append(extra_interline[:]) lines.append(extra_interline[:])
_drawendinglines(lines, extra_interline, edgemap, seen) _drawendinglines(lines, extra_interline, edgemap, seen, state)
while len(text) < len(lines): while len(text) < len(lines):
text.append("") text.append("")
if any(len(char) > 1 for char in edgemap.values()): if any(len(char) > 1 for char in edgemap.values()):
# limit drawing an edge to the first or last N lines of the current # limit drawing an edge to the first or last N lines of the current
# section the rest of the edge is drawn like a parent line. # section the rest of the edge is drawn like a parent line.
parent = state['styles'][PARENT][-1:] parent = state['styles'][PARENT][-1:]

tests/test-logexchange.t

$ cat .hg/logexchange/branches $ cat .hg/logexchange/branches
0 0
ec2426147f0e39dbc9cef599b066be6035ce691d\x00default\x00default (esc) ec2426147f0e39dbc9cef599b066be6035ce691d\x00default\x00default (esc)
3e1487808078543b0af6d10dadf5d46943578db0\x00default\x00wat (esc) 3e1487808078543b0af6d10dadf5d46943578db0\x00default\x00wat (esc)
$ hg show work $ hg show work
o 3e14 (wat) (default/wat) added bar o 3e14 (wat) (default/wat) added bar
|
~ ~
@ ec24 (default/default) Added h @ ec24 (default/default) Added h
|
~ ~
$ hg update "default/wat" $ hg update "default/wat"
1 files updated, 0 files merged, 3 files removed, 0 files unresolved 1 files updated, 0 files merged, 3 files removed, 0 files unresolved
$ hg identify $ hg identify
3e1487808078 (wat) tip 3e1487808078 (wat) tip
Making a new server Making a new server

tests/test-show-work.t

o 9f17 commit 0 o 9f17 commit 0
Bumping first draft changeset to public will hide its parent Bumping first draft changeset to public will hide its parent
$ hg phase --public -r 1 $ hg phase --public -r 1
$ hg show work $ hg show work
@ 128c commit 2 @ 128c commit 2
o 181c commit 1 o 181c commit 1
|
~ ~
Multiple DAG heads will be shown Multiple DAG heads will be shown
$ hg -q up -r 1 $ hg -q up -r 1
$ echo 3 > foo $ echo 3 > foo
$ hg commit -m 'commit 3' $ hg commit -m 'commit 3'
created new head created new head
$ hg show work $ hg show work
@ f0ab commit 3 @ f0ab commit 3
| o 128c commit 2 | o 128c commit 2
|/ |/
o 181c commit 1 o 181c commit 1
|
~ ~
Even when wdir is something else Even when wdir is something else
$ hg -q up null $ hg -q up null
$ hg show work $ hg show work
o f0ab commit 3 o f0ab commit 3
| o 128c commit 2 | o 128c commit 2
|/ |/
o 181c commit 1 o 181c commit 1
|
~ ~
Draft child shows public head (multiple heads) Draft child shows public head (multiple heads)
$ hg -q up 0 $ hg -q up 0
$ echo 4 > foo $ echo 4 > foo
$ hg commit -m 'commit 4' $ hg commit -m 'commit 4'
created new head created new head
$ hg commit -m 'commit 4' $ hg commit -m 'commit 4'
$ hg show work $ hg show work
@ f8dd (mybranch) commit 4 @ f8dd (mybranch) commit 4
o 90cf (mybranch) commit 3 o 90cf (mybranch) commit 3
| o 128c commit 2 | o 128c commit 2
|/ |/
o 181c commit 1 o 181c commit 1
|
~ ~
$ cd .. $ cd ..
Bookmark name appears in output Bookmark name appears in output
$ hg init bookmarks $ hg init bookmarks
$ cd bookmarks $ cd bookmarks
$ hg bookmark mybook $ hg bookmark mybook
$ hg show work $ hg show work
@ cac8 (mybook) commit 4 @ cac8 (mybook) commit 4
o f0ab commit 3 o f0ab commit 3
| o 128c (@) commit 2 | o 128c (@) commit 2
|/ |/
o 181c commit 1 o 181c commit 1
|
~ ~
$ cd .. $ cd ..
Tags are rendered Tags are rendered
$ hg init tags $ hg init tags
$ cd tags $ cd tags
$ echo 0 > foo $ echo 0 > foo
$ hg -q commit -A -m 'commit 1' $ hg -q commit -A -m 'commit 1'
$ echo 1 > foo $ echo 1 > foo
$ hg commit -m 'commit 2' $ hg commit -m 'commit 2'
$ hg tag 0.1 $ hg tag 0.1
$ hg phase --public -r . $ hg phase --public -r .
$ echo 2 > foo $ echo 2 > foo
$ hg commit -m 'commit 3' $ hg commit -m 'commit 3'
$ hg tag 0.2 $ hg tag 0.2
$ hg show work $ hg show work
@ 3758 Added tag 0.2 for changeset 6379c25b76f1 @ 3758 Added tag 0.2 for changeset 6379c25b76f1
o 6379 (0.2) commit 3 o 6379 (0.2) commit 3
o a2ad Added tag 0.1 for changeset 6a75536ea0b1 o a2ad Added tag 0.1 for changeset 6a75536ea0b1
|
~ ~
$ cd .. $ cd ..
Multiple names on same changeset render properly Multiple names on same changeset render properly
$ hg init multiplenames $ hg init multiplenames
$ cd multiplenames $ cd multiplenames
> evolution=createmarkers > evolution=createmarkers
> EOF > EOF
$ hg debugobsolete `hg log -r 'desc("commit 2")' -T "{node}"` $ hg debugobsolete `hg log -r 'desc("commit 2")' -T "{node}"`
obsoleted 1 changesets obsoleted 1 changesets
1 new orphan changesets 1 new orphan changesets
$ hg show work --color=debug $ hg show work --color=debug
@ [log.changeset changeset.draft changeset.unstable instability.orphan|32f3] [log.description|commit 3] @ [log.changeset changeset.draft changeset.unstable instability.orphan|32f3] [log.description|commit 3]
x [log.changeset changeset.draft changeset.obsolete|6a75] [log.description|commit 2] x [log.changeset changeset.draft changeset.obsolete|6a75] [log.description|commit 2]
|
~ ~
$ cd .. $ cd ..
Prefix collision on hashes increases shortest node length Prefix collision on hashes increases shortest node length
$ hg init hashcollision $ hg init hashcollision
$ cd hashcollision $ cd hashcollision