| 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
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,
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
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
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.
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.)
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]
# 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.