Patchwork [V2] revset: define successors revset

login
register
mail settings
Submitter Jun Wu
Date July 10, 2017, 6 p.m.
Message ID <c839dcefb603c31236e6.1499709648@x1c>
Download mbox | patch
Permalink /patch/22211/
State Accepted
Headers show

Comments

Jun Wu - July 10, 2017, 6 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1499709400 25200
#      Mon Jul 10 10:56:40 2017 -0700
# Node ID c839dcefb603c31236e6ed2c3d94fedfe4729d33
# Parent  1e872b08a4e93b21e60cba695e4022bf71d4acf4
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r c839dcefb603
revset: define successors revset

This revset returns all successors, including transit nodes and the source
nodes (to be consistent with existing revsets like "ancestors").

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 it seems 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 some flexible indexing
syntax [1] to satisfy people having the depth limit requirement.

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-July/101140.html
Yuya Nishihara - July 11, 2017, 12:42 p.m.
On Mon, 10 Jul 2017 11:00:48 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1499709400 25200
> #      Mon Jul 10 10:56:40 2017 -0700
> # Node ID c839dcefb603c31236e6ed2c3d94fedfe4729d33
> # Parent  1e872b08a4e93b21e60cba695e4022bf71d4acf4
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r c839dcefb603
> revset: define successors revset

Queued, thanks.
Matt Harbison - July 12, 2017, 4:25 a.m.
On Mon, 10 Jul 2017 14:00:48 -0400, Jun Wu <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1499709400 25200
> #      Mon Jul 10 10:56:40 2017 -0700
> # Node ID c839dcefb603c31236e6ed2c3d94fedfe4729d33
> # Parent  1e872b08a4e93b21e60cba695e4022bf71d4acf4
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r  
> c839dcefb603
> revset: define successors revset
>
> This revset returns all successors, including transit nodes and the  
> source
> nodes (to be consistent with existing revsets like "ancestors").
>
> 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 it seems 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 some flexible indexing
> syntax [1] to satisfy people having the depth limit requirement.
>
> [1]:  
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-July/101140.html

This will be more of an issue with precursors(), which I assume will be  
defined the same way for consistency.  I tend to use the following to make  
sure that conflicts were resolved properly after rebasing:

	$ hg extdiff --patch --hidden -r "precursors($rev)" -r "$rev"

It can be especially handy in a loop when a series is rebased and several  
commits need to be reviewed.  Depth=1 probably solves that.  I only  
mention it to provide an actual use case.  Hopefully this remains working  
one way or the other in 4.3.
Jun Wu - July 12, 2017, 4:54 a.m.
Excerpts from Matt Harbison's message of 2017-07-12 00:25:47 -0400:
> This will be more of an issue with precursors(), which I assume will be  
> defined the same way for consistency.  I tend to use the following to make  
> sure that conflicts were resolved properly after rebasing:
> 
>     $ hg extdiff --patch --hidden -r "precursors($rev)" -r "$rev"
> 
> It can be especially handy in a loop when a series is rebased and several  
> commits need to be reviewed.  Depth=1 probably solves that.  I only  
> mention it to provide an actual use case.  Hopefully this remains working  
> one way or the other in 4.3.

Yes, I was aware of the use-case. We also have internal users wanting to
compare with a previous version.

Note: we have decided to use "predecessor" instead of "precursors" [1].
A "predecessors" revset is planned. However, I think the current obsstore
API need some change to support depth better. Since I'm also planning on new
(hash-preserving) obsstore format and API, I don't want to spend too much
time on things only working for the format obsstore.

The successors revset is mainly to be used together with my rebase work,
where you can use:

  hg rebase -s 'unstable()-obsolete()' -d 'max(successors(max(roots(ALLSRC) & ::SRC)^)-obsolete()-divergent())'

to basically achieve what "evolve --all" can do today, even with more
flexibility like you can move the destination approximately to a head:

  hg rebase -s 'unstable()-obsolete()' -d 'max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete()-divergent())::)'

(I'd wish that feature to be available in 4.3, but that's just a wish)

Predecessor revset is not needed for that so it's less prioritized.

[1]: https://www.mercurial-scm.org/wiki/CEDVocabulary
Jun Wu - July 12, 2017, 4:57 a.m.
Excerpts from Jun Wu's message of 2017-07-11 21:54:36 -0700:
> Excerpts from Matt Harbison's message of 2017-07-12 00:25:47 -0400:
> > This will be more of an issue with precursors(), which I assume will be  
> > defined the same way for consistency.  I tend to use the following to make  
> > sure that conflicts were resolved properly after rebasing:
> > 
> >     $ hg extdiff --patch --hidden -r "precursors($rev)" -r "$rev"
> > 
> > It can be especially handy in a loop when a series is rebased and several  
> > commits need to be reviewed.  Depth=1 probably solves that.  I only  
> > mention it to provide an actual use case.  Hopefully this remains working  
> > one way or the other in 4.3.
> 
> Yes, I was aware of the use-case. We also have internal users wanting to
> compare with a previous version.
> 
> Note: we have decided to use "predecessor" instead of "precursors" [1].
> A "predecessors" revset is planned. However, I think the current obsstore
> API need some change to support depth better. Since I'm also planning on new
> (hash-preserving) obsstore format and API, I don't want to spend too much
> time on things only working for the format obsstore.

correction: s/format/current/

> 
> The successors revset is mainly to be used together with my rebase work,
> where you can use:
> 
>   hg rebase -s 'unstable()-obsolete()' -d 'max(successors(max(roots(ALLSRC) & ::SRC)^)-obsolete()-divergent())'

correction: s/rebase -s/rebase -r/, so does the command below.

> to basically achieve what "evolve --all" can do today, even with more
> flexibility like you can move the destination approximately to a head:
> 
>   hg rebase -s 'unstable()-obsolete()' -d 'max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete()-divergent())::)'
> 
> (I'd wish that feature to be available in 4.3, but that's just a wish)
> 
> Predecessor revset is not needed for that so it's less prioritized.
> 
> [1]: https://www.mercurial-scm.org/wiki/CEDVocabulary

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 set, including the given set themselves"""
+    s = getset(repo, fullreposet(repo), x)
+    f = lambda nodes: obsutil.allsuccessors(repo.obsstore, nodes)
+    d = _mapbynodefunc(repo, s, f)
+    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,55 @@  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(Z)' -T '{desc}\n'
+  Z
+
+  $ hg log -r 'successors(F)' -T '{desc}\n'
+  F
+  G
+
+  $ hg tag --remove --local C D E F G
+
+  $ hg log -r 'successors(B)' -T '{desc}\n'
+  B
+  D
+  E
+  G
+
+  $ hg log -r 'successors(B)' -T '{desc}\n' --hidden
+  B
+  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'
+  A
+  Z
+  B
+
+  $ hg log -r 'successors(B+A)-divergent()-obsolete()' -T '{desc}\n'
+  Z