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

Pierre-Yves David pierre-yves.david at ens-lyon.org
Thu May 14 00:58:16 CDT 2015



On 05/06/2015 02:00 AM, Yuya Nishihara wrote:
> On Wed, 06 May 2015 00:11:50 -0700, Pierre-Yves David wrote:
>> 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).
>
> Yes. I think "all()" could include any revs, i.e. "null and all()" -> "null".
> I wrote 2de9ee016425 to avoid bug in graphmod.dagwalker(). I don't have a
> strong opinion about how "all()" should handle "null" and "wdir()".
>
>> 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.
>
> Strictly speaking, it isn't new issue. fullreposet was not the same as
> spanset because __and__ doesn't filter out unknown revisions. In fact,
> there was issue4396 caused by fullreposet.__and__.

Okay, this is the original issue with fullereposet ☺

>> 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
>
> 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.

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.

What do you think?

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list