Patchwork [3,of,3,RFC] revset: add an 'all' argument to ancestor() to return

login
register
mail settings
Submitter Sean Farley
Date May 27, 2018, 11:48 a.m.
Message ID <9fa3f81f4685ca73393f.1527421698@1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa>
Download mbox | patch
Permalink /patch/31888/
State New
Headers show

Comments

Sean Farley - May 27, 2018, 11:48 a.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1527357855 -7200
#      Sat May 26 20:04:15 2018 +0200
# Node ID 9fa3f81f4685ca73393f57253f2f05a0d758c022
# Parent  000e9442997b1c61ae02a27e657ffb34d170502b
# EXP-Topic gca-revset
revset: add an 'all' argument to ancestor() to return

Currently, I'm not sure if this should be an option or a new revset
method.
Sean Farley - May 27, 2018, noon
Sean Farley <sean@farley.io> writes:

> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1527357855 -7200
> #      Sat May 26 20:04:15 2018 +0200
> # Node ID 9fa3f81f4685ca73393f57253f2f05a0d758c022
> # Parent  000e9442997b1c61ae02a27e657ffb34d170502b
> # EXP-Topic gca-revset
> revset: add an 'all' argument to ancestor() to return
>
> Currently, I'm not sure if this should be an option or a new revset
> method.
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> index de543df..ad88cd2 100644
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -299,25 +299,42 @@ def adds(repo, subset, x):
>      """
>      # i18n: "adds" is a keyword
>      pat = getstring(x, _("adds requires a pattern"))
>      return checkstatus(repo, subset, pat, 1)
>  
> -@predicate('ancestor(*changeset)', safe=True, weight=0.5)
> +@predicate('ancestor(*changeset[, all])', safe=True, weight=0.5)
>  def ancestor(repo, subset, x):
>      """A greatest common ancestor of the changesets.
>  
>      Accepts 0 or more changesets.
>      Will return empty list when passed no args.
>      Greatest common ancestor of a single changeset is that changeset.
> +
> +    If `all` is passed in, then all greatest common ancestors are returned
> +    (e.g. consesus bid merge finding multiple common ancestors from criss cross
> +    merges).
> +
>      """
>      # i18n: "ancestor" is a keyword
>      l = getlist(x)
>      rl = fullreposet(repo)
>      anc = None
> +    allcommon = False
> +
> +    args = getargsdict(x, 'ancestor', 'set all')
> +    if 'all' in args:
> +        allcommon = getboolean(args['all'], _("all expects a boolean"))
>  
>      # (getset(repo, rl, i) for i in l) generates a list of lists
>      for revs in (getset(repo, rl, i) for i in l):
> +        if allcommon and len(revs) > 1:
> +            # for now error if more than two
> +            if len(revs) > 2:
> +                msg = _("currently returning all greatest common ancestors "
> +                        "only works two revs")
> +                raise error.Abort(msg)

I found this a bit awkward, so comments are welcomed. To fix the
limitation of only two revs being passed, we'd need to refactor some of
the code in changelog to return that information.
Yuya Nishihara - May 28, 2018, 12:27 p.m.
On Sun, 27 May 2018 13:48:18 +0200, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1527357855 -7200
> #      Sat May 26 20:04:15 2018 +0200
> # Node ID 9fa3f81f4685ca73393f57253f2f05a0d758c022
> # Parent  000e9442997b1c61ae02a27e657ffb34d170502b
> # EXP-Topic gca-revset
> revset: add an 'all' argument to ancestor() to return
> 
> Currently, I'm not sure if this should be an option or a new revset
> method.

Maybe this can be an optimized path for 'heads(::x, ::y)'?
commonancestorsheads() is documented as such.

> +    args = getargsdict(x, 'ancestor', 'set all')

'*set all' because it takes a variable number of arguments.

> +        if allcommon and len(revs) > 1:
> +            # for now error if more than two
> +            if len(revs) > 2:
> +                msg = _("currently returning all greatest common ancestors "
> +                        "only works two revs")
> +                raise error.Abort(msg)
> +            return repo[revs.first()].commonancestors(repo[revs.last()])

Missed 'subset &' filtering.
Yuya Nishihara - May 28, 2018, 1:07 p.m.
On Mon, 28 May 2018 21:27:40 +0900, Yuya Nishihara wrote:
> On Sun, 27 May 2018 13:48:18 +0200, Sean Farley wrote:
> > # HG changeset patch
> > # User Sean Farley <sean@farley.io>
> > # Date 1527357855 -7200
> > #      Sat May 26 20:04:15 2018 +0200
> > # Node ID 9fa3f81f4685ca73393f57253f2f05a0d758c022
> > # Parent  000e9442997b1c61ae02a27e657ffb34d170502b
> > # EXP-Topic gca-revset
> > revset: add an 'all' argument to ancestor() to return
> > 
> > Currently, I'm not sure if this should be an option or a new revset
> > method.
> 
> Maybe this can be an optimized path for 'heads(::x, ::y)'?
> commonancestorsheads() is documented as such.

To be clear, I thought this should be a new revset function since 'all' could
also mean "all ancestors." And I noticed that it could be a private function
'_ancestorheads(x, y)' rewritten from 'heads(::x, ::y)'.
Sean Farley - May 29, 2018, 2:24 a.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Mon, 28 May 2018 21:27:40 +0900, Yuya Nishihara wrote:
>> On Sun, 27 May 2018 13:48:18 +0200, Sean Farley wrote:
>> > # HG changeset patch
>> > # User Sean Farley <sean@farley.io>
>> > # Date 1527357855 -7200
>> > #      Sat May 26 20:04:15 2018 +0200
>> > # Node ID 9fa3f81f4685ca73393f57253f2f05a0d758c022
>> > # Parent  000e9442997b1c61ae02a27e657ffb34d170502b
>> > # EXP-Topic gca-revset
>> > revset: add an 'all' argument to ancestor() to return
>> >
>> > Currently, I'm not sure if this should be an option or a new revset
>> > method.
>>
>> Maybe this can be an optimized path for 'heads(::x, ::y)'?
>> commonancestorsheads() is documented as such.
>
> To be clear, I thought this should be a new revset function since 'all' could
> also mean "all ancestors." And I noticed that it could be a private function
> '_ancestorheads(x, y)' rewritten from 'heads(::x, ::y)'.

Yeah, I think I agree that it should be a new revset; thanks. I'm not
quite sure what optimization you mean for heads(::x, ::y), though?
Yuya Nishihara - May 29, 2018, 11:34 a.m.
On Mon, 28 May 2018 19:24:08 -0700, Sean Farley wrote:
> > On Mon, 28 May 2018 21:27:40 +0900, Yuya Nishihara wrote:
> >> On Sun, 27 May 2018 13:48:18 +0200, Sean Farley wrote:
> >> > # HG changeset patch
> >> > # User Sean Farley <sean@farley.io>
> >> > # Date 1527357855 -7200
> >> > #      Sat May 26 20:04:15 2018 +0200
> >> > # Node ID 9fa3f81f4685ca73393f57253f2f05a0d758c022
> >> > # Parent  000e9442997b1c61ae02a27e657ffb34d170502b
> >> > # EXP-Topic gca-revset
> >> > revset: add an 'all' argument to ancestor() to return
> >> >
> >> > Currently, I'm not sure if this should be an option or a new revset
> >> > method.
> >>
> >> Maybe this can be an optimized path for 'heads(::x, ::y)'?
> >> commonancestorsheads() is documented as such.
> >
> > To be clear, I thought this should be a new revset function since 'all' could
> > also mean "all ancestors." And I noticed that it could be a private function
> > '_ancestorheads(x, y)' rewritten from 'heads(::x, ::y)'.
> 
> Yeah, I think I agree that it should be a new revset; thanks. I'm not
> quite sure what optimization you mean for heads(::x, ::y), though?

My point is the proposed function can be expressed as a combination of
existing functions:

  'heads(::x, ::y)'  (when 'x' and 'y' have at most one element each)

so we wouldn't have to add new public function.

That said, it isn't easy to determine if 'heads(::x, ::y)' can be rewritten
to '_commonancestorsheads(x, y)' while parsing because 'x'/'y' may return
more than one revisions. So we'll probably end up adding new function,
'heads(commonancestors(set))' or 'commonancestorsheads(set)'.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
index de543df..ad88cd2 100644
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -299,25 +299,42 @@  def adds(repo, subset, x):
     """
     # i18n: "adds" is a keyword
     pat = getstring(x, _("adds requires a pattern"))
     return checkstatus(repo, subset, pat, 1)
 
-@predicate('ancestor(*changeset)', safe=True, weight=0.5)
+@predicate('ancestor(*changeset[, all])', safe=True, weight=0.5)
 def ancestor(repo, subset, x):
     """A greatest common ancestor of the changesets.
 
     Accepts 0 or more changesets.
     Will return empty list when passed no args.
     Greatest common ancestor of a single changeset is that changeset.
+
+    If `all` is passed in, then all greatest common ancestors are returned
+    (e.g. consesus bid merge finding multiple common ancestors from criss cross
+    merges).
+
     """
     # i18n: "ancestor" is a keyword
     l = getlist(x)
     rl = fullreposet(repo)
     anc = None
+    allcommon = False
+
+    args = getargsdict(x, 'ancestor', 'set all')
+    if 'all' in args:
+        allcommon = getboolean(args['all'], _("all expects a boolean"))
 
     # (getset(repo, rl, i) for i in l) generates a list of lists
     for revs in (getset(repo, rl, i) for i in l):
+        if allcommon and len(revs) > 1:
+            # for now error if more than two
+            if len(revs) > 2:
+                msg = _("currently returning all greatest common ancestors "
+                        "only works two revs")
+                raise error.Abort(msg)
+            return repo[revs.first()].commonancestors(repo[revs.last()])
         for r in revs:
             if anc is None:
                 anc = repo[r]
             else:
                 anc = anc.ancestor(repo[r])
diff --git a/tests/test-merge-criss-cross.t b/tests/test-merge-criss-cross.t
index 4901da2..f79a63d 100644
--- a/tests/test-merge-criss-cross.t
+++ b/tests/test-merge-criss-cross.t
@@ -159,10 +159,25 @@  Criss cross merging
   merging f2
   3 files updated, 0 files merged, 0 files removed, 1 files unresolved
   use 'hg resolve' to retry unresolved file merges or 'hg merge --abort' to abandon
   [1]
 
+Test the greatest common ancestor returning multiple changesets
+
+  $ hg log -r 'ancestor(head(), all=True)'
+  changeset:   1:0f6b37dbe527
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     1 first change f1
+  
+  changeset:   2:d1d156401c1b
+  parent:      0:40494bf2444c
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     2 first change f2
+  
+
 Redo merge with merge.preferancestor="*" to enable bid merge
 
   $ rm f*
   $ hg up -qC .
   $ hg merge -v --debug --tool internal:dump 5 --config merge.preferancestor="*"