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

[Pierre-Yves David]

On 05/04/2015 06:05 PM, Yuya Nishihara wrote:
> 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 at 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.

It seems to fix it. but it regress stuff related to "null" handling (And 
probably working directory handling) (eg "null and all()" would now 
return nullid).

This bring my attention on the "new" behavior of fullreposet. 
fullreposet was initially designed as an efficient replacement for 
`spanset(repo)` and it is used as such in multiple place in the code. 
However your change in d2de20e1451f turn it into a yes-card to allow 
"null" through (and also workingdirectory now?). I'm not sure we want 
that relaxed approach everywhere. Especially because:

1) It makes fullreposet ≠ spanset breaking the initial assumption
2) It makes fullreposet non returnable to user code.

Could we put back fullreposet to the old behavior and have an extra 
subclass especially dedicated to be the initial set? One would accept 
null and working directory when it makes sense.

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

-- 
Pierre-Yves David