| 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
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,
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.
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,
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)
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,
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)
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 >
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()'