Patchwork revset: define successors revset

login
register
mail settings
Submitter Jun Wu
Date July 9, 2017, 1:24 a.m.
Message ID <94977b4b9787d8709bc3.1499563449@x1c>
Download mbox | patch
Permalink /patch/22152/
State Superseded
Headers show

Comments

Jun Wu - July 9, 2017, 1:24 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1499563232 25200
#      Sat Jul 08 18:20:32 2017 -0700
# Node ID 94977b4b9787d8709bc3ef7e4ae5ae5410961e5a
# Parent  4672db164c986da4442bd864cd044512d975c3f2
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 94977b4b9787
revset: define successors revset

This revset returns all successors, including transit nodes.

To filter out transit nodes, use `successors(X)-obsolete()`.
To filter out divergent case, use `successors(X)-divergent()-obsolete()`.

The revset could be useful to define rebase destination, like:
`max(successors(BASE)-divergent()-obsolete())`. The `max` is to deal with
splits.

There are other implementations where `successors` returns just one level of
successors, and `allsuccessors` returns everything. I think `successors`
returning all successors by default is more user friendly. We have seen
cases in production where people use 1-level `successors` while they really
want `allsuccessors`. So I think it's better to just have one single revset
returning all successors by default to avoid user errors.

In the future we might want to add `depth` keyword argument to it and for
other revsets like `ancestors` etc. Or even build fancy indexing syntax [1]
to satisfy people having the level limit requirement.

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-July/101140.html
Yuya Nishihara - July 10, 2017, 12:59 p.m.
On Sat, 8 Jul 2017 18:24:09 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1499563232 25200
> #      Sat Jul 08 18:20:32 2017 -0700
> # Node ID 94977b4b9787d8709bc3ef7e4ae5ae5410961e5a
> # Parent  4672db164c986da4442bd864cd044512d975c3f2
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 94977b4b9787
> revset: define successors revset
> 
> This revset returns all successors, including transit nodes.
> 
> To filter out transit nodes, use `successors(X)-obsolete()`.
> To filter out divergent case, use `successors(X)-divergent()-obsolete()`.
> 
> The revset could be useful to define rebase destination, like:
> `max(successors(BASE)-divergent()-obsolete())`. The `max` is to deal with
> splits.
> 
> There are other implementations where `successors` returns just one level of
> successors, and `allsuccessors` returns everything. I think `successors`
> returning all successors by default is more user friendly. We have seen
> cases in production where people use 1-level `successors` while they really
> want `allsuccessors`. So I think it's better to just have one single revset
> returning all successors by default to avoid user errors.

Sounds good.

> In the future we might want to add `depth` keyword argument to it and for
> other revsets like `ancestors` etc. Or even build fancy indexing syntax [1]
> to satisfy people having the level limit requirement.

[...]

> +@predicate('successors(set)', safe=True)
> +def successors(repo, subset, x):
> +    """All successors for given set"""
> +    s = getset(repo, fullreposet(repo), x)
> +    f = lambda nodes: obsutil.allsuccessors(repo.obsstore, nodes)
> +    d = _mapbynodefunc(repo, s, f) - s
> +    return subset & d

If we'll eventually add a depth parameter, I think successors(set) should
include the given set itself for consistency with ancesotrs()/descendants().
Jun Wu - July 10, 2017, 5:07 p.m.
Excerpts from Yuya Nishihara's message of 2017-07-10 21:59:46 +0900:
> If we'll eventually add a depth parameter, I think successors(set) should
> include the given set itself for consistency with ancesotrs()/descendants().

Good point. I was not very comfortable with "- s" here and removing it
should be fine, since the user probably wants "- obsolete()" which removes
"s". Will send V2 to make it consistent with "ancestors".

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -20,4 +20,5 @@  from . import (
     node,
     obsolete as obsmod,
+    obsutil,
     pathutil,
     phases,
@@ -1827,4 +1828,26 @@  def subrepo(repo, subset, x):
     return subset.filter(matches, condrepr=('<subrepo %r>', pat))
 
+def _mapbynodefunc(repo, s, f):
+    """(repo, smartset, [node] -> [node]) -> smartset
+
+    Helper method to map a smartset to another smartset given a function only
+    talking about nodes. Handles converting between rev numbers and nodes, and
+    filtering.
+    """
+    cl = repo.unfiltered().changelog
+    torev = cl.rev
+    tonode = cl.node
+    nodemap = cl.nodemap
+    result = set(torev(n) for n in f(tonode(r) for r in s) if n in nodemap)
+    return smartset.baseset(result - repo.changelog.filteredrevs)
+
+@predicate('successors(set)', safe=True)
+def successors(repo, subset, x):
+    """All successors for given set"""
+    s = getset(repo, fullreposet(repo), x)
+    f = lambda nodes: obsutil.allsuccessors(repo.obsstore, nodes)
+    d = _mapbynodefunc(repo, s, f) - s
+    return subset & d
+
 def _substringmatcher(pattern, casesensitive=True):
     kind, pattern, matcher = util.stringmatcher(pattern,
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -21,4 +21,5 @@ 
   $ cat >> $HGRCPATH << EOF
   > [extensions]
+  > drawdag=$TESTDIR/drawdag.py
   > testrevset=$TESTTMP/testrevset.py
   > EOF
@@ -4284,2 +4285,46 @@  Test repo.anyrevs with customized revset
 
   $ cd ..
+
+Test obsstore related revsets
+
+  $ hg init repo1
+  $ cd repo1
+  $ cat <<EOF >> .hg/hgrc
+  > [experimental]
+  > evolution = createmarkers
+  > EOF
+
+  $ hg debugdrawdag <<'EOS'
+  >        F G
+  >        |/    # split: B -> E, F
+  > B C D  E     # amend: B -> C -> D
+  >  \|/   |     # amend: F -> G
+  >   A    A  Z  # amend: A -> Z
+  > EOS
+
+  $ hg log -r 'successors(F)' -T '{desc}\n'
+  G
+
+  $ for i in C D E F G; do
+  >   hg tag --remove --local $i
+  > done
+
+  $ hg log -r 'successors(B)' -T '{desc}\n'
+  D
+  E
+  G
+
+  $ hg log -r 'successors(B)' -T '{desc}\n' --hidden
+  C
+  D
+  E
+  F
+  G
+
+  $ hg log -r 'successors(B)-obsolete()' -T '{desc}\n' --hidden
+  D
+  E
+  G
+
+  $ hg log -r 'successors(B+A)-divergent()' -T '{desc}\n'
+  Z