Patchwork dagop: raise ProgrammingError if stopdepth<=0

login
register
mail settings
Submitter via Mercurial-devel
Date June 24, 2017, 5:48 a.m.
Message ID <d654eefeefdd3df493fa.1498283303@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/21662/
State Accepted
Headers show

Comments

via Mercurial-devel - June 24, 2017, 5:48 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1498281322 25200
#      Fri Jun 23 22:15:22 2017 -0700
# Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
# Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
dagop: raise ProgrammingError if stopdepth<=0

revset.py should never send such a value.
Yuya Nishihara - June 24, 2017, 6:03 a.m.
On Fri, 23 Jun 2017 22:48:23 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1498281322 25200
> #      Fri Jun 23 22:15:22 2017 -0700
> # Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
> # Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
> dagop: raise ProgrammingError if stopdepth<=0
> 
> revset.py should never send such a value.
> 
> diff --git a/mercurial/dagop.py b/mercurial/dagop.py
> --- a/mercurial/dagop.py
> +++ b/mercurial/dagop.py
> @@ -33,7 +33,7 @@
>      if stopdepth is None:
>          stopdepth = _maxlogdepth
>      if stopdepth <= 0:
> -        return
> +        raise error.ProgrammingError('negative stopdepth')

I think stopdepth = 0 is valid on API level, so I want to change the condition
to stopdepth < 0. I'll update it in flight if you agree.
via Mercurial-devel - June 24, 2017, 6:06 a.m.
On Fri, Jun 23, 2017 at 11:03 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Fri, 23 Jun 2017 22:48:23 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1498281322 25200
>> #      Fri Jun 23 22:15:22 2017 -0700
>> # Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
>> # Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
>> dagop: raise ProgrammingError if stopdepth<=0
>>
>> revset.py should never send such a value.
>>
>> diff --git a/mercurial/dagop.py b/mercurial/dagop.py
>> --- a/mercurial/dagop.py
>> +++ b/mercurial/dagop.py
>> @@ -33,7 +33,7 @@
>>      if stopdepth is None:
>>          stopdepth = _maxlogdepth
>>      if stopdepth <= 0:
>> -        return
>> +        raise error.ProgrammingError('negative stopdepth')
>
> I think stopdepth = 0 is valid on API level, so I want to change the condition
> to stopdepth < 0. I'll update it in flight if you agree.

Weren't you passing in "stopdepth=(n + 1)" where n was a non-negative
number? I.e. n must be >= 0, so stopdepth > 0.
Yuya Nishihara - June 24, 2017, 6:12 a.m.
On Fri, 23 Jun 2017 23:06:19 -0700, Martin von Zweigbergk wrote:
> On Fri, Jun 23, 2017 at 11:03 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Fri, 23 Jun 2017 22:48:23 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1498281322 25200
> >> #      Fri Jun 23 22:15:22 2017 -0700
> >> # Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
> >> # Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
> >> dagop: raise ProgrammingError if stopdepth<=0
> >>
> >> revset.py should never send such a value.
> >>
> >> diff --git a/mercurial/dagop.py b/mercurial/dagop.py
> >> --- a/mercurial/dagop.py
> >> +++ b/mercurial/dagop.py
> >> @@ -33,7 +33,7 @@
> >>      if stopdepth is None:
> >>          stopdepth = _maxlogdepth
> >>      if stopdepth <= 0:
> >> -        return
> >> +        raise error.ProgrammingError('negative stopdepth')
> >
> > I think stopdepth = 0 is valid on API level, so I want to change the condition
> > to stopdepth < 0. I'll update it in flight if you agree.
> 
> Weren't you passing in "stopdepth=(n + 1)" where n was a non-negative
> number? I.e. n must be >= 0, so stopdepth > 0.

Yes. There's no caller that passes stopdepth=0. I mean
revancestors(..., stopdepth=0) => {} isn't an invalid use of the API.
via Mercurial-devel - June 24, 2017, 6:16 a.m.
On Fri, Jun 23, 2017 at 11:12 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Fri, 23 Jun 2017 23:06:19 -0700, Martin von Zweigbergk wrote:
>> On Fri, Jun 23, 2017 at 11:03 PM, Yuya Nishihara <yuya@tcha.org> wrote:
>> > On Fri, 23 Jun 2017 22:48:23 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>> >> # HG changeset patch
>> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> # Date 1498281322 25200
>> >> #      Fri Jun 23 22:15:22 2017 -0700
>> >> # Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
>> >> # Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
>> >> dagop: raise ProgrammingError if stopdepth<=0
>> >>
>> >> revset.py should never send such a value.
>> >>
>> >> diff --git a/mercurial/dagop.py b/mercurial/dagop.py
>> >> --- a/mercurial/dagop.py
>> >> +++ b/mercurial/dagop.py
>> >> @@ -33,7 +33,7 @@
>> >>      if stopdepth is None:
>> >>          stopdepth = _maxlogdepth
>> >>      if stopdepth <= 0:
>> >> -        return
>> >> +        raise error.ProgrammingError('negative stopdepth')
>> >
>> > I think stopdepth = 0 is valid on API level, so I want to change the condition
>> > to stopdepth < 0. I'll update it in flight if you agree.
>>
>> Weren't you passing in "stopdepth=(n + 1)" where n was a non-negative
>> number? I.e. n must be >= 0, so stopdepth > 0.
>
> Yes. There's no caller that passes stopdepth=0. I mean
> revancestors(..., stopdepth=0) => {} isn't an invalid use of the API. Oh, I see what you're saying. It would just no yield any matches, right? So it would be:

if stopdepth == 0:
  return
if stopdepth < 0:
  raise ProgrammingError()

Did I get that right?
Yuya Nishihara - June 24, 2017, 6:22 a.m.
On Fri, 23 Jun 2017 23:16:17 -0700, Martin von Zweigbergk wrote:
> On Fri, Jun 23, 2017 at 11:12 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Fri, 23 Jun 2017 23:06:19 -0700, Martin von Zweigbergk wrote:
> >> On Fri, Jun 23, 2017 at 11:03 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> >> > On Fri, 23 Jun 2017 22:48:23 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> >> >> # HG changeset patch
> >> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> >> # Date 1498281322 25200
> >> >> #      Fri Jun 23 22:15:22 2017 -0700
> >> >> # Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
> >> >> # Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
> >> >> dagop: raise ProgrammingError if stopdepth<=0
> >> >>
> >> >> revset.py should never send such a value.
> >> >>
> >> >> diff --git a/mercurial/dagop.py b/mercurial/dagop.py
> >> >> --- a/mercurial/dagop.py
> >> >> +++ b/mercurial/dagop.py
> >> >> @@ -33,7 +33,7 @@
> >> >>      if stopdepth is None:
> >> >>          stopdepth = _maxlogdepth
> >> >>      if stopdepth <= 0:
> >> >> -        return
> >> >> +        raise error.ProgrammingError('negative stopdepth')
> >> >
> >> > I think stopdepth = 0 is valid on API level, so I want to change the condition
> >> > to stopdepth < 0. I'll update it in flight if you agree.
> >>
> >> Weren't you passing in "stopdepth=(n + 1)" where n was a non-negative
> >> number? I.e. n must be >= 0, so stopdepth > 0.
> >
> > Yes. There's no caller that passes stopdepth=0. I mean
> > revancestors(..., stopdepth=0) => {} isn't an invalid use of the API. Oh, I see what you're saying. It would just no yield any matches, right? So it would be:
> 
> if stopdepth == 0:
>   return
> if stopdepth < 0:
>   raise ProgrammingError()
> 
> Did I get that right?

Yeah, that's what I have in mind.
via Mercurial-devel - June 24, 2017, 6:25 a.m.
On Fri, Jun 23, 2017 at 11:22 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Fri, 23 Jun 2017 23:16:17 -0700, Martin von Zweigbergk wrote:
>> On Fri, Jun 23, 2017 at 11:12 PM, Yuya Nishihara <yuya@tcha.org> wrote:
>> > On Fri, 23 Jun 2017 23:06:19 -0700, Martin von Zweigbergk wrote:
>> >> On Fri, Jun 23, 2017 at 11:03 PM, Yuya Nishihara <yuya@tcha.org> wrote:
>> >> > On Fri, 23 Jun 2017 22:48:23 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>> >> >> # HG changeset patch
>> >> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> >> # Date 1498281322 25200
>> >> >> #      Fri Jun 23 22:15:22 2017 -0700
>> >> >> # Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
>> >> >> # Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
>> >> >> dagop: raise ProgrammingError if stopdepth<=0
>> >> >>
>> >> >> revset.py should never send such a value.
>> >> >>
>> >> >> diff --git a/mercurial/dagop.py b/mercurial/dagop.py
>> >> >> --- a/mercurial/dagop.py
>> >> >> +++ b/mercurial/dagop.py
>> >> >> @@ -33,7 +33,7 @@
>> >> >>      if stopdepth is None:
>> >> >>          stopdepth = _maxlogdepth
>> >> >>      if stopdepth <= 0:
>> >> >> -        return
>> >> >> +        raise error.ProgrammingError('negative stopdepth')
>> >> >
>> >> > I think stopdepth = 0 is valid on API level, so I want to change the condition
>> >> > to stopdepth < 0. I'll update it in flight if you agree.
>> >>
>> >> Weren't you passing in "stopdepth=(n + 1)" where n was a non-negative
>> >> number? I.e. n must be >= 0, so stopdepth > 0.
>> >
>> > Yes. There's no caller that passes stopdepth=0. I mean
>> > revancestors(..., stopdepth=0) => {} isn't an invalid use of the API. Oh, I see what you're saying. It would just no yield any matches, right? So it would be:
>>
>> if stopdepth == 0:
>>   return
>> if stopdepth < 0:
>>   raise ProgrammingError()
>>
>> Did I get that right?
>
> Yeah, that's what I have in mind.

Sounds good! Feel free to restructure that as you please.
Yuya Nishihara - June 24, 2017, 6:36 a.m.
On Fri, 23 Jun 2017 23:25:18 -0700, Martin von Zweigbergk wrote:
> On Fri, Jun 23, 2017 at 11:22 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Fri, 23 Jun 2017 23:16:17 -0700, Martin von Zweigbergk wrote:
> >> On Fri, Jun 23, 2017 at 11:12 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> >> > On Fri, 23 Jun 2017 23:06:19 -0700, Martin von Zweigbergk wrote:
> >> >> On Fri, Jun 23, 2017 at 11:03 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> >> >> > On Fri, 23 Jun 2017 22:48:23 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> >> >> >> # HG changeset patch
> >> >> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> >> >> # Date 1498281322 25200
> >> >> >> #      Fri Jun 23 22:15:22 2017 -0700
> >> >> >> # Node ID d654eefeefdd3df493fa777cbcfe423f17d9d500
> >> >> >> # Parent  fac9941bd542e3621082b157fb2f3aff09cfb1b7
> >> >> >> dagop: raise ProgrammingError if stopdepth<=0
> >> >> >>
> >> >> >> revset.py should never send such a value.
> >> >> >>
> >> >> >> diff --git a/mercurial/dagop.py b/mercurial/dagop.py
> >> >> >> --- a/mercurial/dagop.py
> >> >> >> +++ b/mercurial/dagop.py
> >> >> >> @@ -33,7 +33,7 @@
> >> >> >>      if stopdepth is None:
> >> >> >>          stopdepth = _maxlogdepth
> >> >> >>      if stopdepth <= 0:
> >> >> >> -        return
> >> >> >> +        raise error.ProgrammingError('negative stopdepth')
> >> >> >
> >> >> > I think stopdepth = 0 is valid on API level, so I want to change the condition
> >> >> > to stopdepth < 0. I'll update it in flight if you agree.
> >> >>
> >> >> Weren't you passing in "stopdepth=(n + 1)" where n was a non-negative
> >> >> number? I.e. n must be >= 0, so stopdepth > 0.
> >> >
> >> > Yes. There's no caller that passes stopdepth=0. I mean
> >> > revancestors(..., stopdepth=0) => {} isn't an invalid use of the API. Oh, I see what you're saying. It would just no yield any matches, right? So it would be:
> >>
> >> if stopdepth == 0:
> >>   return
> >> if stopdepth < 0:
> >>   raise ProgrammingError()
> >>
> >> Did I get that right?
> >
> > Yeah, that's what I have in mind.
> 
> Sounds good! Feel free to restructure that as you please.

Queued with that change, thanks.

Patch

diff --git a/mercurial/dagop.py b/mercurial/dagop.py
--- a/mercurial/dagop.py
+++ b/mercurial/dagop.py
@@ -33,7 +33,7 @@ 
     if stopdepth is None:
         stopdepth = _maxlogdepth
     if stopdepth <= 0:
-        return
+        raise error.ProgrammingError('negative stopdepth')
 
     cl = repo.changelog