Patchwork pushkey: use False/True for return values from push functions

login
register
mail settings
Submitter via Mercurial-devel
Date June 13, 2017, 3:53 p.m.
Message ID <984cdd0844fecb6c56d5.1497369192@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/21360/
State Accepted
Headers show

Comments

via Mercurial-devel - June 13, 2017, 3:53 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1497310557 25200
#      Mon Jun 12 16:35:57 2017 -0700
# Node ID 984cdd0844fecb6c56d570236b03c999c4d485cf
# Parent  f40eec7af04416521543b284fc6fa5365dbef611
pushkey: use False/True for return values from push functions

It was particularly unclear in phases.pushphase() whether the 0/1
returned were the 0/1 for public/draft phase or for False/True
Sean Farley - June 13, 2017, 6:54 p.m.
Martin von Zweigbergk via Mercurial-devel
<mercurial-devel@mercurial-scm.org> writes:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1497310557 25200
> #      Mon Jun 12 16:35:57 2017 -0700
> # Node ID 984cdd0844fecb6c56d570236b03c999c4d485cf
> # Parent  f40eec7af04416521543b284fc6fa5365dbef611
> pushkey: use False/True for return values from push functions
>
> It was particularly unclear in phases.pushphase() whether the 0/1
> returned were the 0/1 for public/draft phase or for False/True

Looks good to me; queued!
Pierre-Yves David - June 14, 2017, 11:15 a.m.
On 06/13/2017 08:54 PM, Sean Farley wrote:
> Martin von Zweigbergk via Mercurial-devel
> <mercurial-devel@mercurial-scm.org> writes:
>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1497310557 25200
>> #      Mon Jun 12 16:35:57 2017 -0700
>> # Node ID 984cdd0844fecb6c56d570236b03c999c4d485cf
>> # Parent  f40eec7af04416521543b284fc6fa5365dbef611
>> pushkey: use False/True for return values from push functions
>>
>> It was particularly unclear in phases.pushphase() whether the 0/1
>> returned were the 0/1 for public/draft phase or for False/True
>
> Looks good to me; queued!

I'm not sure this is a good idea, the pushkey return treated as an 
integer everywhere in the code, including the bit handling phases. Using 
"True/False" works because they are also integers but that does not feel 
like a good idea to me.

code using phases should use the explicit name (phases.public, 
phases.draft) and I think we should rather make sure of this than using 
explicit integer in the integer space.

(I do not have a too strong opinion about that)

Cheers
Yuya Nishihara - June 14, 2017, 12:52 p.m.
On Wed, 14 Jun 2017 13:15:22 +0200, Pierre-Yves David wrote:
> On 06/13/2017 08:54 PM, Sean Farley wrote:
> > Martin von Zweigbergk via Mercurial-devel
> > <mercurial-devel@mercurial-scm.org> writes:
> >
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1497310557 25200
> >> #      Mon Jun 12 16:35:57 2017 -0700
> >> # Node ID 984cdd0844fecb6c56d570236b03c999c4d485cf
> >> # Parent  f40eec7af04416521543b284fc6fa5365dbef611
> >> pushkey: use False/True for return values from push functions
> >>
> >> It was particularly unclear in phases.pushphase() whether the 0/1
> >> returned were the 0/1 for public/draft phase or for False/True
> >
> > Looks good to me; queued!
> 
> I'm not sure this is a good idea, the pushkey return treated as an 
> integer everywhere in the code, including the bit handling phases. Using 
> "True/False" works because they are also integers but that does not feel 
> like a good idea to me.

Agreed. It seems these return values derive from the wireproto layer.
via Mercurial-devel - June 14, 2017, 4:07 p.m.
On Wed, Jun 14, 2017 at 5:52 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Wed, 14 Jun 2017 13:15:22 +0200, Pierre-Yves David wrote:
>> On 06/13/2017 08:54 PM, Sean Farley wrote:
>> > Martin von Zweigbergk via Mercurial-devel
>> > <mercurial-devel@mercurial-scm.org> writes:
>> >
>> >> # HG changeset patch
>> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> # Date 1497310557 25200
>> >> #      Mon Jun 12 16:35:57 2017 -0700
>> >> # Node ID 984cdd0844fecb6c56d570236b03c999c4d485cf
>> >> # Parent  f40eec7af04416521543b284fc6fa5365dbef611
>> >> pushkey: use False/True for return values from push functions
>> >>
>> >> It was particularly unclear in phases.pushphase() whether the 0/1
>> >> returned were the 0/1 for public/draft phase or for False/True
>> >
>> > Looks good to me; queued!
>>
>> I'm not sure this is a good idea, the pushkey return treated as an
>> integer everywhere in the code, including the bit handling phases. Using
>> "True/False" works because they are also integers but that does not feel
>> like a good idea to me.
>
> Agreed. It seems these return values derive from the wireproto layer.

For both of you: Where specifically?
Augie Fackler - June 14, 2017, 7:04 p.m.
> On Jun 14, 2017, at 12:07, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> 
> On Wed, Jun 14, 2017 at 5:52 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>> On Wed, 14 Jun 2017 13:15:22 +0200, Pierre-Yves David wrote:
>>> On 06/13/2017 08:54 PM, Sean Farley wrote:
>>>> Martin von Zweigbergk via Mercurial-devel
>>>> <mercurial-devel@mercurial-scm.org> writes:
>>>> 
>>>>> # HG changeset patch
>>>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>>>> # Date 1497310557 25200
>>>>> #      Mon Jun 12 16:35:57 2017 -0700
>>>>> # Node ID 984cdd0844fecb6c56d570236b03c999c4d485cf
>>>>> # Parent  f40eec7af04416521543b284fc6fa5365dbef611
>>>>> pushkey: use False/True for return values from push functions
>>>>> 
>>>>> It was particularly unclear in phases.pushphase() whether the 0/1
>>>>> returned were the 0/1 for public/draft phase or for False/True
>>>> 
>>>> Looks good to me; queued!
>>> 
>>> I'm not sure this is a good idea, the pushkey return treated as an
>>> integer everywhere in the code, including the bit handling phases. Using
>>> "True/False" works because they are also integers but that does not feel
>>> like a good idea to me.
>> 
>> Agreed. It seems these return values derive from the wireproto layer.
> 
> For both of you: Where specifically?

from wireproto.py:

@wireprotocommand('pushkey', 'namespace key old new')
def pushkey(repo, proto, namespace, key, old, new):
[...]
    r = repo.pushkey(encoding.tolocal(namespace), encoding.tolocal(key),
                     encoding.tolocal(old), new)
    return '%s\n' % int(r)

But I think this patch is fine. I'm going to mark it as accepted, and if we end up worried about this we can back it out. :)

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - June 14, 2017, 10:57 p.m.
On Wed, Jun 14, 2017 at 12:04 PM, Augie Fackler <raf@durin42.com> wrote:
>
>> On Jun 14, 2017, at 12:07, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>>
>> On Wed, Jun 14, 2017 at 5:52 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>>> On Wed, 14 Jun 2017 13:15:22 +0200, Pierre-Yves David wrote:
>>>> On 06/13/2017 08:54 PM, Sean Farley wrote:
>>>>> Martin von Zweigbergk via Mercurial-devel
>>>>> <mercurial-devel@mercurial-scm.org> writes:
>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>>>>> # Date 1497310557 25200
>>>>>> #      Mon Jun 12 16:35:57 2017 -0700
>>>>>> # Node ID 984cdd0844fecb6c56d570236b03c999c4d485cf
>>>>>> # Parent  f40eec7af04416521543b284fc6fa5365dbef611
>>>>>> pushkey: use False/True for return values from push functions
>>>>>>
>>>>>> It was particularly unclear in phases.pushphase() whether the 0/1
>>>>>> returned were the 0/1 for public/draft phase or for False/True
>>>>>
>>>>> Looks good to me; queued!
>>>>
>>>> I'm not sure this is a good idea, the pushkey return treated as an
>>>> integer everywhere in the code, including the bit handling phases. Using
>>>> "True/False" works because they are also integers but that does not feel
>>>> like a good idea to me.
>>>
>>> Agreed. It seems these return values derive from the wireproto layer.
>>
>> For both of you: Where specifically?
>
> from wireproto.py:
>
> @wireprotocommand('pushkey', 'namespace key old new')
> def pushkey(repo, proto, namespace, key, old, new):
> [...]
>     r = repo.pushkey(encoding.tolocal(namespace), encoding.tolocal(key),
>                      encoding.tolocal(old), new)
>     return '%s\n' % int(r)

Ah, there it is. Thanks. I did look, but I couldn't find it.

>
> But I think this patch is fine. I'm going to mark it as accepted, and if we end up worried about this we can back it out. :)

Yeah, seems safe since it's explicitly converted to int. If we do end
up backing it out, we should also update bookmarks.py because that
already use False/True (that's how I learned that those were valid
values).

>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -944,10 +944,10 @@ 
     """Push markers over pushkey"""
     if not key.startswith('dump'):
         repo.ui.warn(_('unknown key: %r') % key)
-        return 0
+        return False
     if old:
         repo.ui.warn(_('unexpected old value for %r') % key)
-        return 0
+        return False
     data = util.b85decode(new)
     lock = repo.lock()
     try:
@@ -956,7 +956,7 @@ 
             repo.obsstore.mergemarkers(tr, data)
             repo.invalidatevolatilesets()
             tr.close()
-            return 1
+            return True
         finally:
             tr.release()
     finally:
diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -423,12 +423,12 @@ 
         if currentphase == oldphase and newphase < oldphase:
             with repo.transaction('pushkey-phase') as tr:
                 advanceboundary(repo, tr, newphase, [bin(nhex)])
-            return 1
+            return True
         elif currentphase == newphase:
             # raced, but got correct result
-            return 1
+            return True
         else:
-            return 0
+            return False
 
 def analyzeremotephases(repo, subset, roots):
     """Compute phases heads and root in a subset of node from root dict