Patchwork [5,of,9] revset: abort the query when there is no revision for "rev()" (BC)

login
register
mail settings
Submitter Katsunori FUJIWARA
Date March 29, 2015, 6:34 p.m.
Message ID <f9e9783d4f10af2974d7.1427654067@juju>
Download mbox | patch
Permalink /patch/8358/
State Changes Requested
Headers show

Comments

Katsunori FUJIWARA - March 29, 2015, 6:34 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1427653752 -32400
#      Mon Mar 30 03:29:12 2015 +0900
# Node ID f9e9783d4f10af2974d77dbfaacd217f24fd3035
# Parent  efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
revset: abort the query when there is no revision for "rev()" (BC)

Before this patch, "REV" and "rev(REV)" differ from each other:

    target      REV          rev(REV)
    ----------- ------------ ------------
    known       pass         pass
    hidden      abort (*1)   pass (=> empty set)
    unknown     abort        pass (=> empty set)

    (*1) abort with "use --hidden" hint

For equivalence between them, "rev(REV)" should abort the query for
hidden or unknown revisions, even though this may break backward
compatibility for existing tools, which expect that "rev()" doesn't
abort the query in any cases.

This patch uses "context.findbyrevnum()" (via repo) to abort if the
target revision doesn't exist or isn't visible.
Yuya Nishihara - March 30, 2015, 5:05 a.m.
On Mon, 30 Mar 2015 03:34:27 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1427653752 -32400
> #      Mon Mar 30 03:29:12 2015 +0900
> # Node ID f9e9783d4f10af2974d77dbfaacd217f24fd3035
> # Parent  efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
> revset: abort the query when there is no revision for "rev()" (BC)
> 
> Before this patch, "REV" and "rev(REV)" differ from each other:
> 
>     target      REV          rev(REV)
>     ----------- ------------ ------------
>     known       pass         pass
>     hidden      abort (*1)   pass (=> empty set)
>     unknown     abort        pass (=> empty set)
> 
>     (*1) abort with "use --hidden" hint
> 
> For equivalence between them, "rev(REV)" should abort the query for
> hidden or unknown revisions, even though this may break backward
> compatibility for existing tools, which expect that "rev()" doesn't
> abort the query in any cases.

I thought about it before, but because rev() didn't raise error for 3 years,
I'm afraid that there would be some tools that rely on the existing behavior.

Regards,
Katsunori FUJIWARA - March 30, 2015, 4:31 p.m.
At Mon, 30 Mar 2015 14:05:45 +0900,
Yuya Nishihara wrote:
> 
> On Mon, 30 Mar 2015 03:34:27 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1427653752 -32400
> > #      Mon Mar 30 03:29:12 2015 +0900
> > # Node ID f9e9783d4f10af2974d77dbfaacd217f24fd3035
> > # Parent  efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
> > revset: abort the query when there is no revision for "rev()" (BC)
> > 
> > Before this patch, "REV" and "rev(REV)" differ from each other:
> > 
> >     target      REV          rev(REV)
> >     ----------- ------------ ------------
> >     known       pass         pass
> >     hidden      abort (*1)   pass (=> empty set)
> >     unknown     abort        pass (=> empty set)
> > 
> >     (*1) abort with "use --hidden" hint
> > 
> > For equivalence between them, "rev(REV)" should abort the query for
> > hidden or unknown revisions, even though this may break backward
> > compatibility for existing tools, which expect that "rev()" doesn't
> > abort the query in any cases.
> 
> I thought about it before, but because rev() didn't raise error for 3 years,
> I'm afraid that there would be some tools that rely on the existing behavior.

What about introducing new predicates "revnum()" (and "hashid()") or
so for strict behavior ? (should we make "rev()"/"id()" deprecate, too ?)

> Regards,
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Augie Fackler - March 30, 2015, 6:19 p.m.
On Tue, Mar 31, 2015 at 01:31:52AM +0900, FUJIWARA Katsunori wrote:
> At Mon, 30 Mar 2015 14:05:45 +0900,
> Yuya Nishihara wrote:
> >
> > On Mon, 30 Mar 2015 03:34:27 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1427653752 -32400
> > > #      Mon Mar 30 03:29:12 2015 +0900
> > > # Node ID f9e9783d4f10af2974d77dbfaacd217f24fd3035
> > > # Parent  efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
> > > revset: abort the query when there is no revision for "rev()" (BC)
> > >
> > > Before this patch, "REV" and "rev(REV)" differ from each other:
> > >
> > >     target      REV          rev(REV)
> > >     ----------- ------------ ------------
> > >     known       pass         pass
> > >     hidden      abort (*1)   pass (=> empty set)
> > >     unknown     abort        pass (=> empty set)
> > >
> > >     (*1) abort with "use --hidden" hint
> > >
> > > For equivalence between them, "rev(REV)" should abort the query for
> > > hidden or unknown revisions, even though this may break backward
> > > compatibility for existing tools, which expect that "rev()" doesn't
> > > abort the query in any cases.
> >
> > I thought about it before, but because rev() didn't raise error for 3 years,
> > I'm afraid that there would be some tools that rely on the existing behavior.
>
> What about introducing new predicates "revnum()" (and "hashid()") or
> so for strict behavior ? (should we make "rev()"/"id()" deprecate, too ?)

I'd be in favor of something like that iff we can't declare the
no-args behavior of rev() and id() to be bugs, which would be my
strong preference. cc'ing mpm so he can weigh in.

>
> > Regards,
> >
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - March 31, 2015, 12:08 p.m.
On Mon, 2015-03-30 at 14:19 -0400, Augie Fackler wrote:
> On Tue, Mar 31, 2015 at 01:31:52AM +0900, FUJIWARA Katsunori wrote:
> > At Mon, 30 Mar 2015 14:05:45 +0900,
> > Yuya Nishihara wrote:
> > >
> > > On Mon, 30 Mar 2015 03:34:27 +0900, FUJIWARA Katsunori wrote:
> > > > # HG changeset patch
> > > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > > # Date 1427653752 -32400
> > > > #      Mon Mar 30 03:29:12 2015 +0900
> > > > # Node ID f9e9783d4f10af2974d77dbfaacd217f24fd3035
> > > > # Parent  efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
> > > > revset: abort the query when there is no revision for "rev()" (BC)
> > > >
> > > > Before this patch, "REV" and "rev(REV)" differ from each other:
> > > >
> > > >     target      REV          rev(REV)
> > > >     ----------- ------------ ------------
> > > >     known       pass         pass
> > > >     hidden      abort (*1)   pass (=> empty set)
> > > >     unknown     abort        pass (=> empty set)
> > > >
> > > >     (*1) abort with "use --hidden" hint
> > > >
> > > > For equivalence between them, "rev(REV)" should abort the query for
> > > > hidden or unknown revisions, even though this may break backward
> > > > compatibility for existing tools, which expect that "rev()" doesn't
> > > > abort the query in any cases.
> > >
> > > I thought about it before, but because rev() didn't raise error for 3 years,
> > > I'm afraid that there would be some tools that rely on the existing behavior.
> >
> > What about introducing new predicates "revnum()" (and "hashid()") or
> > so for strict behavior ? (should we make "rev()"/"id()" deprecate, too ?)
> 
> I'd be in favor of something like that iff we can't declare the
> no-args behavior of rev() and id() to be bugs, which would be my
> strong preference. cc'ing mpm so he can weigh in.

"no-args behavior"?

We seem to be preparing to change the behavior with explicitly naming
hidden changesets, but no one wants to tell me about it.
Augie Fackler - March 31, 2015, 12:38 p.m.
On Tue, Mar 31, 2015 at 8:08 AM, Matt Mackall <mpm@selenic.com> wrote:
> On Mon, 2015-03-30 at 14:19 -0400, Augie Fackler wrote:
>> On Tue, Mar 31, 2015 at 01:31:52AM +0900, FUJIWARA Katsunori wrote:
>> > At Mon, 30 Mar 2015 14:05:45 +0900,
>> > Yuya Nishihara wrote:
>> > >
>> > > On Mon, 30 Mar 2015 03:34:27 +0900, FUJIWARA Katsunori wrote:
>> > > > # HG changeset patch
>> > > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>> > > > # Date 1427653752 -32400
>> > > > #      Mon Mar 30 03:29:12 2015 +0900
>> > > > # Node ID f9e9783d4f10af2974d77dbfaacd217f24fd3035
>> > > > # Parent  efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
>> > > > revset: abort the query when there is no revision for "rev()" (BC)
>> > > >
>> > > > Before this patch, "REV" and "rev(REV)" differ from each other:
>> > > >
>> > > >     target      REV          rev(REV)
>> > > >     ----------- ------------ ------------
>> > > >     known       pass         pass
>> > > >     hidden      abort (*1)   pass (=> empty set)
>> > > >     unknown     abort        pass (=> empty set)
>> > > >
>> > > >     (*1) abort with "use --hidden" hint
>> > > >
>> > > > For equivalence between them, "rev(REV)" should abort the query for
>> > > > hidden or unknown revisions, even though this may break backward
>> > > > compatibility for existing tools, which expect that "rev()" doesn't
>> > > > abort the query in any cases.
>> > >
>> > > I thought about it before, but because rev() didn't raise error for 3 years,
>> > > I'm afraid that there would be some tools that rely on the existing behavior.
>> >
>> > What about introducing new predicates "revnum()" (and "hashid()") or
>> > so for strict behavior ? (should we make "rev()"/"id()" deprecate, too ?)
>>
>> I'd be in favor of something like that iff we can't declare the
>> no-args behavior of rev() and id() to be bugs, which would be my
>> strong preference. cc'ing mpm so he can weigh in.
>
> "no-args behavior"?

I worded things poorly, but also completely misread the table above. I
had (wrongly) thought this was making it so that no-arguments-passed
rev() and id() would be saner by returning errors, but it's actually
about an argument being passed that's somehow invalid being allowed to
slip by.

I'm no longer a fan of where this series is going, both because it
seems wrong and because I can't fathom a usecase.

>
> We seem to be preparing to change the behavior with explicitly naming
> hidden changesets, but no one wants to tell me about it.

Huh? (I'm legitimately confused here, but I also haven't read -devel
since work yesterday. If there's some sort of plan about that, I'm
unaware of it.)
Matt Mackall - March 31, 2015, 12:42 p.m.
On Tue, 2015-03-31 at 08:38 -0400, Augie Fackler wrote:
> On Tue, Mar 31, 2015 at 8:08 AM, Matt Mackall <mpm@selenic.com> wrote:
> > On Mon, 2015-03-30 at 14:19 -0400, Augie Fackler wrote:
> >> On Tue, Mar 31, 2015 at 01:31:52AM +0900, FUJIWARA Katsunori wrote:
> >> > At Mon, 30 Mar 2015 14:05:45 +0900,
> >> > Yuya Nishihara wrote:
> >> > >
> >> > > On Mon, 30 Mar 2015 03:34:27 +0900, FUJIWARA Katsunori wrote:
> >> > > > # HG changeset patch
> >> > > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> >> > > > # Date 1427653752 -32400
> >> > > > #      Mon Mar 30 03:29:12 2015 +0900
> >> > > > # Node ID f9e9783d4f10af2974d77dbfaacd217f24fd3035
> >> > > > # Parent  efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
> >> > > > revset: abort the query when there is no revision for "rev()" (BC)
> >> > > >
> >> > > > Before this patch, "REV" and "rev(REV)" differ from each other:
> >> > > >
> >> > > >     target      REV          rev(REV)
> >> > > >     ----------- ------------ ------------
> >> > > >     known       pass         pass
> >> > > >     hidden      abort (*1)   pass (=> empty set)
> >> > > >     unknown     abort        pass (=> empty set)
> >> > > >
> >> > > >     (*1) abort with "use --hidden" hint
> >> > > >
> >> > > > For equivalence between them, "rev(REV)" should abort the query for
> >> > > > hidden or unknown revisions, even though this may break backward
> >> > > > compatibility for existing tools, which expect that "rev()" doesn't
> >> > > > abort the query in any cases.
> >> > >
> >> > > I thought about it before, but because rev() didn't raise error for 3 years,
> >> > > I'm afraid that there would be some tools that rely on the existing behavior.
> >> >
> >> > What about introducing new predicates "revnum()" (and "hashid()") or
> >> > so for strict behavior ? (should we make "rev()"/"id()" deprecate, too ?)
> >>
> >> I'd be in favor of something like that iff we can't declare the
> >> no-args behavior of rev() and id() to be bugs, which would be my
> >> strong preference. cc'ing mpm so he can weigh in.
> >
> > "no-args behavior"?
> 
> I worded things poorly, but also completely misread the table above. I
> had (wrongly) thought this was making it so that no-arguments-passed
> rev() and id() would be saner by returning errors, but it's actually
> about an argument being passed that's somehow invalid being allowed to
> slip by.
> 
> I'm no longer a fan of where this series is going, both because it
> seems wrong and because I can't fathom a usecase.
> 
> >
> > We seem to be preparing to change the behavior with explicitly naming
> > hidden changesets, but no one wants to tell me about it.
> 
> Huh? (I'm legitimately confused here, but I also haven't read -devel
> since work yesterday. If there's some sort of plan about that, I'm
> unaware of it.)

There's work underway to make:

hg log -r <somethinghidden>

..just work without needing --hidden, since it was specified explicitly.
I've been lobbying for this for a while. So far the plan seems to only
be to work on hashes though.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1540,6 +1540,10 @@  def removes(repo, subset, x):
 def rev(repo, subset, x):
     """``rev(number)``
     Revision with the given numeric identifier.
+
+    This predicate aborts the query, if the specified number doesn't
+    refer any (visible) existing revision. Use this with ``present()``
+    to continue the query even in such case.
     """
     # i18n: "rev" is a keyword
     l = getargs(x, 1, 1, _("rev requires one argument"))
@@ -1549,8 +1553,9 @@  def rev(repo, subset, x):
     except (TypeError, ValueError):
         # i18n: "rev" is a keyword
         raise error.ParseError(_("rev expects a number"))
-    if l not in repo.changelog and l != node.nullrev:
-        return baseset()
+
+    repo.findbyrevnum(l, abort=True)
+
     return subset & baseset([l])
 
 def matching(repo, subset, x):
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -186,7 +186,14 @@  check that various commands work well wi
   (use --hidden to access hidden revisions)
   [255]
   $ hg debugrevspec 'rev(6)'
+  abort: unknown revision '6'!
+  [255]
   $ hg debugrevspec 'rev(4)'
+  abort: hidden revision '4'!
+  (use --hidden to access hidden revisions)
+  [255]
+  $ hg debugrevspec --hidden 'rev(4)'
+  4
   $ hg debugrevspec 'null'
   -1
 
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -543,6 +543,8 @@  Test the order of operations
 
 Test explicit numeric revision
   $ log 'rev(-2)'
+  abort: unknown revision '-2'!
+  [255]
   $ log 'rev(-1)'
   -1
   $ log 'rev(0)'
@@ -550,6 +552,9 @@  Test explicit numeric revision
   $ log 'rev(9)'
   9
   $ log 'rev(10)'
+  abort: unknown revision '10'!
+  [255]
+  $ log 'present(rev(10))'
   $ log 'rev(tip)'
   hg: parse error: rev expects a number
   [255]