This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6331

log: add config for making `hg log -G` always topo-sorted
ClosedPublic

Authored by martinvonz on May 1 2019, 1:22 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG604c086ddde6: log: add config for making `hg log -G` always topo-sorted
rHGa39f8aa64cef: log: add config for making `hg log -G` always topo-sorted
Summary

I (and everyone else at Google) have an log alias that adds graph mode
and templating. I have another one that builds on the first and also
restricts the set of revisions to only show those I'm most likely to
care about. This second alias also adds topological sorting. I still
sometimes use the first one. When I do, it very often bothers me that
it's not topologically sorted (branches are interleaved). This patch
adds a config option for always using topological sorting with graph
log.

The revision set is sorted eagerly, which seems like a bad idea, but
it doesn't seem to make a big difference in the hg repo (150ms). I
initially tried to instead wrap the user's revset in sort(...,topo),
but that seemed much harder.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.May 1 2019, 1:22 PM
yuja added a subscriber: yuja.May 14 2019, 8:35 AM

+def _maybetoposort(repo, revs, opts):
+ if opts.get('graph') and repo.ui.configbool('experimental', 'log.topo'):
+ revs = dagop.toposort(revs, repo.changelog.parentrevs)
+ # TODO: try to iterate the set lazily
+ revs = revset.baseset(list(revs))
+ return revs
+
def getrevs(repo, pats, opts):

"""Return (revs, differ) where revs is a smartset

@@ -727,7 +734,7 @@

limit = getlimit(opts)
revs = _initialrevs(repo, opts)
if not revs:
  • return smartset.baseset(), None

+ return _maybetoposort(repo, smartset.baseset(), opts), None

Nit: no need to sort an empty set.

@@ -756,7 +763,7 @@

differ = changesetdiffer()
differ._makefilematcher = filematcher
  • return revs, differ

+ return _maybetoposort(repo, revs, opts), differ

Maybe this can be merged with the if opts.get('graph') block above.

# I think "and opts.get('rev')" can be removed since revs.isdescending()
# would be true if --follow without --rev.
if opts.get('graph'):
    if experimental.log.topo:
        if not topo yet:
            toposort(revs)
    elif not (revs.isdescending() or revs.istopo()):
        revs.sort(reverse=True)
marmoute added a subscriber: marmoute.May 14 2019, 10:00 AM

There used to be a experimental.graph-group-branches (that tortoise-hg
still use) that got removed at some point. Did you check the rational
behind the removal ?

If we reintroduce the option, I would be in favor of defining a path to
a non-experimental version of it.

martinvonz added a comment.May 14 2019, 11:54 AM
In D6331#92621, @marmoute wrote:

There used to be a experimental.graph-group-branches (that tortoise-hg
still use) that got removed at some point. Did you check the rational
behind the removal ?

I didn't remember that option, so no. It was removed in 2188f170f5b6 (revset: add new topographical sort, 2016-06-13). All the rationale I could find is right there in the subject line (i.e. "add new topographical sort"). Was there more to it that you can remember?

If we reintroduce the option, I would be in favor of defining a path to
a non-experimental version of it.

I'd be fine with simply not marking it experimental from the beginning even though we know that it's not as fast/lazy as it should be.

martinvonz added a comment.May 14 2019, 12:34 PM
In D6331#92619, @yuja wrote:

+def _maybetoposort(repo, revs, opts):
+ if opts.get('graph') and repo.ui.configbool('experimental', 'log.topo'):
+ revs = dagop.toposort(revs, repo.changelog.parentrevs)
+ # TODO: try to iterate the set lazily
+ revs = revset.baseset(list(revs))
+ return revs
+
def getrevs(repo, pats, opts):

"""Return (revs, differ) where revs is a smartset

@@ -727,7 +734,7 @@

limit = getlimit(opts)
revs = _initialrevs(repo, opts)
if not revs:
  • return smartset.baseset(), None

+ return _maybetoposort(repo, smartset.baseset(), opts), None

Nit: no need to sort an empty set.

Heh, true. I'l fix that.

@@ -756,7 +763,7 @@

differ = changesetdiffer()
differ._makefilematcher = filematcher
  • return revs, differ

+ return _maybetoposort(repo, revs, opts), differ

Maybe this can be merged with the if opts.get('graph') block above.

# I think "and opts.get('rev')" can be removed since revs.isdescending()
# would be true if --follow without --rev.
if opts.get('graph'):
    if experimental.log.topo:
        if not topo yet:
            toposort(revs)
    elif not (revs.isdescending() or revs.istopo()):
        revs.sort(reverse=True)

Good idea, that looks much better.

martinvonz updated this revision to Diff 15076.May 14 2019, 1:47 PM
pulkit accepted this revision.May 15 2019, 2:00 PM
Closed by commit rHGa39f8aa64cef: log: add config for making `hg log -G` always topo-sorted (authored by martinvonz). · Explain WhyMay 15 2019, 2:31 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 15076

mercurial/configitems.py

alias=[('experimental', 'effect-flags')] alias=[('experimental', 'effect-flags')]
) )
coreconfigitem('experimental', 'evolution.exchange', coreconfigitem('experimental', 'evolution.exchange',
default=None, default=None,
) )
coreconfigitem('experimental', 'evolution.bundle-obsmarker', coreconfigitem('experimental', 'evolution.bundle-obsmarker',
default=False, default=False,
) )
coreconfigitem('experimental', 'log.topo',
default=False,
)
coreconfigitem('experimental', 'evolution.report-instabilities', coreconfigitem('experimental', 'evolution.report-instabilities',
default=True, default=True,
) )
coreconfigitem('experimental', 'evolution.track-operation', coreconfigitem('experimental', 'evolution.track-operation',
default=True, default=True,
) )
# repo-level config to exclude a revset visibility # repo-level config to exclude a revset visibility
# #

mercurial/logcmdutil.py

if filematcher is None: if filematcher is None:
def filematcher(ctx): def filematcher(ctx):
return match return match
expr = _makerevset(repo, match, pats, slowpath, opts) expr = _makerevset(repo, match, pats, slowpath, opts)
if opts.get('graph'): if opts.get('graph'):
# User-specified revs might be unsorted, but don't sort before # User-specified revs might be unsorted, but don't sort before
# _makerevset because it might depend on the order of revs # _makerevset because it might depend on the order of revs
if not (revs.isdescending() or revs.istopo()): if repo.ui.configbool('experimental', 'log.topo'):
if not revs.istopo():
revs = dagop.toposort(revs, repo.changelog.parentrevs)
# TODO: try to iterate the set lazily
revs = revset.baseset(list(revs))
elif not (revs.isdescending() or revs.istopo()):
revs.sort(reverse=True) revs.sort(reverse=True)
if expr: if expr:
matcher = revset.match(None, expr) matcher = revset.match(None, expr)
revs = matcher(repo, revs) revs = matcher(repo, revs)
if limit is not None: if limit is not None:
revs = revs.slice(0, limit) revs = revs.slice(0, limit)
differ = changesetdiffer() differ = changesetdiffer()

relnotes/next

== New Features == == New Features ==
* New config `commands.commit.post-status` shows status after successful * New config `commands.commit.post-status` shows status after successful
commit. commit.
== New Experimental Features ==
* New config `experimental.log.topo` makes `hg log -G` use
topological sorting. This is especially useful for aliases since it
lets the alias accept an `-r` option while still using topological
sorting with or without the `-r` (unlike if you use the `sort(...,
topo)` revset).
== Bug Fixes == == Bug Fixes ==
== Backwards Compatibility Changes == == Backwards Compatibility Changes ==
== Internal API Changes == == Internal API Changes ==
* Matchers are no longer iterable. Use `match.files()` instead. * Matchers are no longer iterable. Use `match.files()` instead.

tests/test-glog-topological.t

| o 3 | o 3
| | | |
| o 2 | o 2
| | | |
| o 1 | o 1
|/ |/
o 0 o 0
Topological sort can be turned on via config
$ cat >> $HGRCPATH << EOF
> [experimental]
> log.topo=true
> EOF
$ hg log -G
o 8
|
o 3
|
o 2
|
o 1
|
| o 7
| |
| o 6
| |
| o 5
| |
| o 4
|/
o 0
Does not affect non-graph log
$ hg log -T '{rev}\n'
8
7
6
5
4
3
2
1
0