Patchwork [3,of,3] revset: drop factory that promotes spanset to fullreposet

login
register
mail settings
Submitter Yuya Nishihara
Date May 5, 2015, 1:05 a.m.
Message ID <20150505100511.0dee35e97bca51ddfb61c29f@tcha.org>
Download mbox | patch
Permalink /patch/8890/
State Changes Requested
Headers show

Comments

Yuya Nishihara - May 5, 2015, 1:05 a.m.
On Mon, 04 May 2015 17:19:35 -0700, Pierre-Yves David wrote:
> On 02/10/2015 07:50 AM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1420728195 -32400
> > #      Thu Jan 08 23:43:15 2015 +0900
> > # Node ID 9962a866325681d8e4523ea30edd3e2ed8343f98
> > # Parent  f04a70f7f3a11b5c66dc739cdf6bcf57d59183ff
> > revset: drop factory that promotes spanset to fullreposet
> 
> This overlooked the 'all()' revset as a place were spanset was used 
> (making combinaison with involving all() less efficients as they 
> should). We cannot just fix 'all()' to used 'fullreposet' because 
> fullreposet is now doing working copy magic too. This lead to these two 
> test failure when tried. Yuya, can I get you to look at this?
> 
> --- /home/marmoute/mercurial-testing/tests/test-glog.t
> +++ /home/marmoute/mercurial-testing/tests/test-glog.t.err
> @@ -2370,9 +2370,9 @@
>     $ hg log -G -r 'all()' | tail -6
>     |
>     o  changeset:   0:f8035bb17114
> -     user:        test
> -     date:        Thu Jan 01 00:00:00 1970 +0000
> -     summary:     add a
> -
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     add a
> +  |

Does this solve the efficiency problem?

It backs out 2de9ee016425 and wraps fullreposet at match() instead. It
assumes that the optimization provided by fullreposet is necessary while
calculating revset.
Yuya Nishihara - May 14, 2015, 11:01 p.m.
On Wed, 13 May 2015 22:58:16 -0700, Pierre-Yves David wrote:
> On 05/06/2015 02:00 AM, Yuya Nishihara wrote:
> > On Wed, 06 May 2015 00:11:50 -0700, Pierre-Yves David wrote:
> >> On related topic, one what behavior did we settle regarding usage of
> >> "null" in revset? I cannot find anything related to this discussion in
> >> mercurial/revset.py
> >
> > Not sure if it's settled. My starting point was to fix buggy handling of
> > "null" in revset.
> >
> > http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75134/focus=75148
> 
> We should probably discuss this, decide on a behavior, document it and 
> stick to it. I cannot access the gmane thread for some reason :-/. Do 
> you have another pointer to this messages?
> 
> My current opinions is:
> 
> - null should work when explicitly referenced
> - null should not survive combination (and) with anything.

I agree with the bullet point 1, but my opinion for 2 is:

- null should work just like other revisions once appeared

> In practice this means
> 
> 1) The following include null in the result
> - "null"
> - "null + 2"
> - "null::" (probably)
> - "::null" (probably)
> 
> 2) the following does not includes it
> - "null and (::2)"
> - "null and date(-9001)"
> - "null and all()"
> (replaces null by any of the variant in (1))
>
> Same should probably apply to the "working directory" revset.
> "all()" could maybe take and argument to accept null.

- "null and (:2)" -> ()       # because :2 = 0:2
- "null and (::2)" -> (null,)
- "null and date(-100000)" -> (null,)
- "null and all()" -> ()      # if all() = 0:tip
                   or (null)  # if all() = everything in sets (like "and true")
- "wdir() and draft()" -> (wdir,)

Regards,
Pierre-Yves David - May 18, 2015, 3:12 a.m.
On 05/14/2015 04:01 PM, Yuya Nishihara wrote:
> On Wed, 13 May 2015 22:58:16 -0700, Pierre-Yves David wrote:
>> On 05/06/2015 02:00 AM, Yuya Nishihara wrote:
>>> On Wed, 06 May 2015 00:11:50 -0700, Pierre-Yves David wrote:
>>>> On related topic, one what behavior did we settle regarding usage of
>>>> "null" in revset? I cannot find anything related to this discussion in
>>>> mercurial/revset.py
>>>
>>> Not sure if it's settled. My starting point was to fix buggy handling of
>>> "null" in revset.
>>>
>>> http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75134/focus=75148
>>
>> We should probably discuss this, decide on a behavior, document it and
>> stick to it. I cannot access the gmane thread for some reason :-/. Do
>> you have another pointer to this messages?
>>
>> My current opinions is:
>>
>> - null should work when explicitly referenced
>> - null should not survive combination (and) with anything.
>
> I agree with the bullet point 1, but my opinion for 2 is:
>
> - null should work just like other revisions once appeared
>
>> In practice this means
>>
>> 1) The following include null in the result
>> - "null"
>> - "null + 2"
>> - "null::" (probably)
>> - "::null" (probably)
>>
>> 2) the following does not includes it
>> - "null and (::2)"
>> - "null and date(-9001)"
>> - "null and all()"
>> (replaces null by any of the variant in (1))
>>
>> Same should probably apply to the "working directory" revset.
>> "all()" could maybe take and argument to accept null.
>
> - "null and (:2)" -> ()       # because :2 = 0:2
> - "null and (::2)" -> (null,)
> - "null and date(-100000)" -> (null,)
> - "null and all()" -> ()      # if all() = 0:tip
>                     or (null)  # if all() = everything in sets (like "and true")
> - "wdir() and draft()" -> (wdir,)

So, there does not seems to be an obvious consensus and the question 
seems to be complicated. I think we need to take a global look at this 
issue to ensure we have something consistent in 3.5. This is especially 
important if we want to push the 'wdir()' forward.

I think we need to start a wiki page at this point. The wiki page would 
list:

- a set of relevant example usecases. With possible behavior.
- sorts -all- existing revset is a set of relevant categories taking in 
account how they are affected by null/wdir and if their implementation 
would need rework to support them.

Yuya, do you have time/willingness to build such wiki page?

On the implementation front, revset are currently not ready for a 
consistent support of null/wdir. We would probably need a special flag 
to force specific inclusive "mode" for most revset predicate.
Yuya Nishihara - May 18, 2015, 1:52 p.m.
On Sun, 17 May 2015 20:12:50 -0700, Pierre-Yves David wrote:
> On 05/14/2015 04:01 PM, Yuya Nishihara wrote:
> > On Wed, 13 May 2015 22:58:16 -0700, Pierre-Yves David wrote:
> >> On 05/06/2015 02:00 AM, Yuya Nishihara wrote:
> >>> On Wed, 06 May 2015 00:11:50 -0700, Pierre-Yves David wrote:
> >>>> On related topic, one what behavior did we settle regarding usage of
> >>>> "null" in revset? I cannot find anything related to this discussion in
> >>>> mercurial/revset.py
> >>>
> >>> Not sure if it's settled. My starting point was to fix buggy handling of
> >>> "null" in revset.
> >>>
> >>> http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75134/focus=75148
> >>
> >> We should probably discuss this, decide on a behavior, document it and
> >> stick to it. I cannot access the gmane thread for some reason :-/. Do
> >> you have another pointer to this messages?
> >>
> >> My current opinions is:
> >>
> >> - null should work when explicitly referenced
> >> - null should not survive combination (and) with anything.
> >
> > I agree with the bullet point 1, but my opinion for 2 is:
> >
> > - null should work just like other revisions once appeared
> >
> >> In practice this means
> >>
> >> 1) The following include null in the result
> >> - "null"
> >> - "null + 2"
> >> - "null::" (probably)
> >> - "::null" (probably)
> >>
> >> 2) the following does not includes it
> >> - "null and (::2)"
> >> - "null and date(-9001)"
> >> - "null and all()"
> >> (replaces null by any of the variant in (1))
> >>
> >> Same should probably apply to the "working directory" revset.
> >> "all()" could maybe take and argument to accept null.
> >
> > - "null and (:2)" -> ()       # because :2 = 0:2
> > - "null and (::2)" -> (null,)
> > - "null and date(-100000)" -> (null,)
> > - "null and all()" -> ()      # if all() = 0:tip
> >                     or (null)  # if all() = everything in sets (like "and true")
> > - "wdir() and draft()" -> (wdir,)
> 
> So, there does not seems to be an obvious consensus and the question 
> seems to be complicated. I think we need to take a global look at this 
> issue to ensure we have something consistent in 3.5. This is especially 
> important if we want to push the 'wdir()' forward.
> 
> I think we need to start a wiki page at this point. The wiki page would 
> list:
> 
> - a set of relevant example usecases. With possible behavior.
> - sorts -all- existing revset is a set of relevant categories taking in 
> account how they are affected by null/wdir and if their implementation 
> would need rework to support them.
> 
> Yuya, do you have time/willingness to build such wiki page?

will do.

> On the implementation front, revset are currently not ready for a
> consistent support of null/wdir. We would probably need a special flag
> to force specific inclusive "mode" for most revset predicate.

Perhaps your suggestion, "null should not survive", assumes a flag-based
implementation. I have another idea to handle null/wdir just like a normal
revision, in which "null should not survive" wouldn't go well.

Regards,
Pierre-Yves David - May 18, 2015, 3:43 p.m.
On 05/18/2015 08:52 AM, Yuya Nishihara wrote:
> On Sun, 17 May 2015 20:12:50 -0700, Pierre-Yves David wrote:
>> On 05/14/2015 04:01 PM, Yuya Nishihara wrote:
>>> On Wed, 13 May 2015 22:58:16 -0700, Pierre-Yves David wrote:
>>>> On 05/06/2015 02:00 AM, Yuya Nishihara wrote:
>>>>> On Wed, 06 May 2015 00:11:50 -0700, Pierre-Yves David wrote:
>>>>>> On related topic, one what behavior did we settle regarding usage of
>>>>>> "null" in revset? I cannot find anything related to this discussion in
>>>>>> mercurial/revset.py
>>>>>
>>>>> Not sure if it's settled. My starting point was to fix buggy handling of
>>>>> "null" in revset.
>>>>>
>>>>> http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75134/focus=75148
>>>>
>>>> We should probably discuss this, decide on a behavior, document it and
>>>> stick to it. I cannot access the gmane thread for some reason :-/. Do
>>>> you have another pointer to this messages?
>>>>
>>>> My current opinions is:
>>>>
>>>> - null should work when explicitly referenced
>>>> - null should not survive combination (and) with anything.
>>>
>>> I agree with the bullet point 1, but my opinion for 2 is:
>>>
>>> - null should work just like other revisions once appeared
>>>
>>>> In practice this means
>>>>
>>>> 1) The following include null in the result
>>>> - "null"
>>>> - "null + 2"
>>>> - "null::" (probably)
>>>> - "::null" (probably)
>>>>
>>>> 2) the following does not includes it
>>>> - "null and (::2)"
>>>> - "null and date(-9001)"
>>>> - "null and all()"
>>>> (replaces null by any of the variant in (1))
>>>>
>>>> Same should probably apply to the "working directory" revset.
>>>> "all()" could maybe take and argument to accept null.
>>>
>>> - "null and (:2)" -> ()       # because :2 = 0:2
>>> - "null and (::2)" -> (null,)
>>> - "null and date(-100000)" -> (null,)
>>> - "null and all()" -> ()      # if all() = 0:tip
>>>                      or (null)  # if all() = everything in sets (like "and true")
>>> - "wdir() and draft()" -> (wdir,)
>>
>> So, there does not seems to be an obvious consensus and the question
>> seems to be complicated. I think we need to take a global look at this
>> issue to ensure we have something consistent in 3.5. This is especially
>> important if we want to push the 'wdir()' forward.
>>
>> I think we need to start a wiki page at this point. The wiki page would
>> list:
>>
>> - a set of relevant example usecases. With possible behavior.
>> - sorts -all- existing revset is a set of relevant categories taking in
>> account how they are affected by null/wdir and if their implementation
>> would need rework to support them.
>>
>> Yuya, do you have time/willingness to build such wiki page?
>
> will do.
>
>> On the implementation front, revset are currently not ready for a
>> consistent support of null/wdir. We would probably need a special flag
>> to force specific inclusive "mode" for most revset predicate.
>
> Perhaps your suggestion, "null should not survive", assumes a flag-based
> implementation. I have another idea to handle null/wdir just like a normal
> revision, in which "null should not survive" wouldn't go well.

I'm assuming multiple revset will have to special case them (like the 
one walking the graph or the one that just reuse an internal set)
Yuya Nishihara - June 3, 2015, 2:37 p.m.
On Sun, 17 May 2015 20:12:50 -0700, Pierre-Yves David wrote:
> So, there does not seems to be an obvious consensus and the question
> seems to be complicated. I think we need to take a global look at this
> issue to ensure we have something consistent in 3.5. This is especially
> important if we want to push the 'wdir()' forward.
> 
> I think we need to start a wiki page at this point. The wiki page would
> list:
> 
> - a set of relevant example usecases. With possible behavior.
> - sorts -all- existing revset is a set of relevant categories taking in
> account how they are affected by null/wdir and if their implementation
> would need rework to support them.

I've updated the wiki page to show the current status of null revision in
revset.

https://mercurial.selenic.com/wiki/RevsetVirtualRevisionPlan

It doesn't list the status of wdir(), but I believe wdir() will crash in most
cases because of None.

Regards,
Pierre-Yves David - June 3, 2015, 4:05 p.m.
On 06/03/2015 07:37 AM, Yuya Nishihara wrote:
> On Sun, 17 May 2015 20:12:50 -0700, Pierre-Yves David wrote:
>> So, there does not seems to be an obvious consensus and the question
>> seems to be complicated. I think we need to take a global look at this
>> issue to ensure we have something consistent in 3.5. This is especially
>> important if we want to push the 'wdir()' forward.
>>
>> I think we need to start a wiki page at this point. The wiki page would
>> list:
>>
>> - a set of relevant example usecases. With possible behavior.
>> - sorts -all- existing revset is a set of relevant categories taking in
>> account how they are affected by null/wdir and if their implementation
>> would need rework to support them.
>
> I've updated the wiki page to show the current status of null revision in
> revset.
>
> https://mercurial.selenic.com/wiki/RevsetVirtualRevisionPlan
>
> It doesn't list the status of wdir(), but I believe wdir() will crash in most
> cases because of None.

And I'm sure None is used as 'no value' in multiple case so this might 
be a pain to get it working (expecially in the extension world)
Martin von Zweigbergk - June 3, 2015, 4:06 p.m.
On Wed, Jun 3, 2015 at 7:42 AM Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 17 May 2015 20:12:50 -0700, Pierre-Yves David wrote:
> > So, there does not seems to be an obvious consensus and the question
> > seems to be complicated. I think we need to take a global look at this
> > issue to ensure we have something consistent in 3.5. This is especially
> > important if we want to push the 'wdir()' forward.
> >
> > I think we need to start a wiki page at this point. The wiki page would
> > list:
> >
> > - a set of relevant example usecases. With possible behavior.
> > - sorts -all- existing revset is a set of relevant categories taking in
> > account how they are affected by null/wdir and if their implementation
> > would need rework to support them.
>
> I've updated the wiki page to show the current status of null revision in
> revset.
>
> https://mercurial.selenic.com/wiki/RevsetVirtualRevisionPlan


I haven't been watching the conversion very closely, but I just want to say
the wiki looks great. Thanks!


>
>
> It doesn't list the status of wdir(), but I believe wdir() will crash in
> most
> cases because of None.
>
> Regards,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - June 5, 2015, 2:01 p.m.
On Wed, 03 Jun 2015 09:05:10 -0700, Pierre-Yves David wrote:
> On 06/03/2015 07:37 AM, Yuya Nishihara wrote:
> > On Sun, 17 May 2015 20:12:50 -0700, Pierre-Yves David wrote:
> >> So, there does not seems to be an obvious consensus and the question
> >> seems to be complicated. I think we need to take a global look at this
> >> issue to ensure we have something consistent in 3.5. This is especially
> >> important if we want to push the 'wdir()' forward.
> >>
> >> I think we need to start a wiki page at this point. The wiki page would
> >> list:
> >>
> >> - a set of relevant example usecases. With possible behavior.
> >> - sorts -all- existing revset is a set of relevant categories taking in
> >> account how they are affected by null/wdir and if their implementation
> >> would need rework to support them.
> >
> > I've updated the wiki page to show the current status of null revision in
> > revset.
> >
> > https://mercurial.selenic.com/wiki/RevsetVirtualRevisionPlan
> >
> > It doesn't list the status of wdir(), but I believe wdir() will crash in most
> > cases because of None.
> 
> And I'm sure None is used as 'no value' in multiple case so this might
> be a pain to get it working (expecially in the extension world)

Yes. Because revset heavily depends on numeric revisions, None shouldn't appear
in set. My plan for the None issue is to map 'wdir()' to 'len(repo)'.

  wdir() -> len(repo)
  repo[len(repo)] -> wctx
  # canonical form is 'None'
  wctx.rev() -> None
  repo[None] -> wctx

A downside of this design is that we have to be careful of using ctx.rev().

  revs = repo.revs(...)
  r = revs.first()
  r in revs  # ok
  ctx = repo[r]
  ctx.rev() in revs  # fails if ctx is a workingctx

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -996,7 +996,7 @@  def getall(repo, subset, x):
     """
     # i18n: "all" is a keyword
     getargs(x, 0, 0, _("all takes no arguments"))
-    return subset & spanset(repo)  # drop "null" if any
+    return subset
 
 def grep(repo, subset, x):
     """``grep(regex)``
@@ -2522,6 +2522,9 @@  def match(ui, spec, repo=None):
             result = getset(repo, subset, tree)
         else:
             result = getset(repo, baseset(subset), tree)
+        if isinstance(result, fullreposet):
+            # do not expose fullreposet because its __contains__ causes problem
+            result &= spanset(repo)
         return result
     return mfunc
 
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -600,9 +600,6 @@  Test null revision
   -1
   $ log 'min(null:)'
   -1
-  $ log 'tip:null and all()' | tail -2
-  1
-  0
 
 Test working-directory revision
   $ hg debugrevspec 'wdir()'