Patchwork revset: for x^2, do not take null as a valid p2 revision

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 14, 2016, 3:35 p.m.
Message ID <18ba0036dd81db9e45fd.1476459326@mimosa>
Download mbox | patch
Permalink /patch/17085/
State Accepted
Headers show

Comments

Yuya Nishihara - Oct. 14, 2016, 3:35 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1476455580 -32400
#      Fri Oct 14 23:33:00 2016 +0900
# Node ID 18ba0036dd81db9e45fd1a450c7f207f1b89f22c
# Parent  5cb830801855dbb63e98b948e355bc995d295bf3
revset: for x^2, do not take null as a valid p2 revision

Since we don't count null p2 revision as a parent, x^2 should never return
null even if null is explicitly populated.
Pierre-Yves David - Oct. 14, 2016, 4:42 p.m.
On 10/14/2016 05:35 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1476455580 -32400
> #      Fri Oct 14 23:33:00 2016 +0900
> # Node ID 18ba0036dd81db9e45fd1a450c7f207f1b89f22c
> # Parent  5cb830801855dbb63e98b948e355bc995d295bf3
> revset: for x^2, do not take null as a valid p2 revision
>
> Since we don't count null p2 revision as a parent, x^2 should never return
> null even if null is explicitly populated.
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -1620,7 +1620,7 @@ def parentspec(repo, subset, x, n, order
>              ps.add(cl.parentrevs(r)[0])
>          elif n == 2:
>              parents = cl.parentrevs(r)
> -            if len(parents) > 1:
> +            if parents[1] != node.nullrev:

Is there any chance that p1 is nullid and p2 is not?

Also, how does this behave if r is a root (p1 and p2 is nullid).

Should we instead filter the parents list ?::

   parents = [p for p in cl.parentrevs(r) if p != nullrev]

>                  ps.add(parents[1])
>      return subset & ps
>
> diff --git a/tests/test-revset.t b/tests/test-revset.t
> --- a/tests/test-revset.t
> +++ b/tests/test-revset.t
> @@ -2750,6 +2750,7 @@ parentrevspec
>    5
>    $ log 'merge()^2'
>    4
> +  $ log '(not merge())^2'
>    $ log 'merge()^^'
>    3
>    $ log 'merge()^1^'
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - Oct. 15, 2016, 7:06 a.m.
On Fri, 14 Oct 2016 18:42:12 +0200, Pierre-Yves David wrote:
> On 10/14/2016 05:35 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1476455580 -32400
> > #      Fri Oct 14 23:33:00 2016 +0900
> > # Node ID 18ba0036dd81db9e45fd1a450c7f207f1b89f22c
> > # Parent  5cb830801855dbb63e98b948e355bc995d295bf3
> > revset: for x^2, do not take null as a valid p2 revision
> >
> > Since we don't count null p2 revision as a parent, x^2 should never return
> > null even if null is explicitly populated.
> >
> > diff --git a/mercurial/revset.py b/mercurial/revset.py
> > --- a/mercurial/revset.py
> > +++ b/mercurial/revset.py
> > @@ -1620,7 +1620,7 @@ def parentspec(repo, subset, x, n, order
> >              ps.add(cl.parentrevs(r)[0])
> >          elif n == 2:
> >              parents = cl.parentrevs(r)
> > -            if len(parents) > 1:
> > +            if parents[1] != node.nullrev:
> 
> Is there any chance that p1 is nullid and p2 is not?

No.

> Also, how does this behave if r is a root (p1 and p2 is nullid).
> 
> Should we instead filter the parents list ?::
> 
>    parents = [p for p in cl.parentrevs(r) if p != nullrev]

IMHO, that's undecided problem. It doesn't make sense to take p2=null as
a valid relation because otherwise all revisions must be rooted to null (b),
which doesn't agree with how we draw a graph including null (a).

  (a) -1 - 0 - 1 ...

  (b) -1 = 0 - 1
       \      /
        ------

But we could say '0^1' (or 'null:tip & 0^1') is null per the graph (a). So
I fixed only 'x^2 -> null', which is clearly wrong.
Pierre-Yves David - Oct. 16, 2016, 12:20 p.m.
On 10/15/2016 09:06 AM, Yuya Nishihara wrote:
> On Fri, 14 Oct 2016 18:42:12 +0200, Pierre-Yves David wrote:
>> On 10/14/2016 05:35 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1476455580 -32400
>>> #      Fri Oct 14 23:33:00 2016 +0900
>>> # Node ID 18ba0036dd81db9e45fd1a450c7f207f1b89f22c
>>> # Parent  5cb830801855dbb63e98b948e355bc995d295bf3
>>> revset: for x^2, do not take null as a valid p2 revision
>>>
>>> Since we don't count null p2 revision as a parent, x^2 should never return
>>> null even if null is explicitly populated.

I've pushed this as a net improvement.

>>> diff --git a/mercurial/revset.py b/mercurial/revset.py
>>> --- a/mercurial/revset.py
>>> +++ b/mercurial/revset.py
>>> @@ -1620,7 +1620,7 @@ def parentspec(repo, subset, x, n, order
>>>              ps.add(cl.parentrevs(r)[0])
>>>          elif n == 2:
>>>              parents = cl.parentrevs(r)
>>> -            if len(parents) > 1:
>>> +            if parents[1] != node.nullrev:
>>
>> Is there any chance that p1 is nullid and p2 is not?
>
> No.
>
>> Also, how does this behave if r is a root (p1 and p2 is nullid).
>>
>> Should we instead filter the parents list ?::
>>
>>    parents = [p for p in cl.parentrevs(r) if p != nullrev]
>
> IMHO, that's undecided problem. It doesn't make sense to take p2=null as
> a valid relation because otherwise all revisions must be rooted to null (b),
> which doesn't agree with how we draw a graph including null (a).
>
>   (a) -1 - 0 - 1 ...
>
>   (b) -1 = 0 - 1
>        \      /
>         ------
>
> But we could say '0^1' (or 'null:tip & 0^1') is null per the graph (a). So
> I fixed only 'x^2 -> null', which is clearly wrong.

I remember a discussion about having the special revision excluded 
unless something else in the revset make a direct reference to it. But 
this is a discussion for after this cycle.

Cheers,

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1620,7 +1620,7 @@  def parentspec(repo, subset, x, n, order
             ps.add(cl.parentrevs(r)[0])
         elif n == 2:
             parents = cl.parentrevs(r)
-            if len(parents) > 1:
+            if parents[1] != node.nullrev:
                 ps.add(parents[1])
     return subset & ps
 
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -2750,6 +2750,7 @@  parentrevspec
   5
   $ log 'merge()^2'
   4
+  $ log '(not merge())^2'
   $ log 'merge()^^'
   3
   $ log 'merge()^1^'