Patchwork [STABLE] singlehead: making config item a bool again

login
register
mail settings
Submitter Georges Racinet
Date Nov. 20, 2019, 6:23 p.m.
Message ID <c557e15674ea6adfe1b0.1574274206@purity.tombe.racinet.fr>
Download mbox | patch
Permalink /patch/43375/
State Superseded
Headers show

Comments

Georges Racinet - Nov. 20, 2019, 6:23 p.m.
# HG changeset patch
# User Georges Racinet <georges.racinet@octobus.net>
# Date 1574273222 -3600
#      Wed Nov 20 19:07:02 2019 +0100
# Branch stable
# Node ID c557e15674ea6adfe1b034e5b4af1e26bca850dd
# Parent  ca3dca416f8d5863ca6f5a4a6a6bb835dcd5feeb
# EXP-Topic single_head_is_boolean
singlehead: making config item a bool again

with the use of `configsuboptions`, the main config item has become
a string (unless it's just the default value).

This makes it in particular hard to override in a cascade of HGRC files,
as we do in Heptapod to re-allow multiple heads on specific repositories
while the default behaviour is to forbid them. The added test case reflects
that use-case
Yuya Nishihara - Nov. 21, 2019, 12:38 p.m.
On Wed, 20 Nov 2019 19:23:26 +0100, Georges Racinet wrote:
> # HG changeset patch
> # User Georges Racinet <georges.racinet@octobus.net>
> # Date 1574273222 -3600
> #      Wed Nov 20 19:07:02 2019 +0100
> # Branch stable
> # Node ID c557e15674ea6adfe1b034e5b4af1e26bca850dd
> # Parent  ca3dca416f8d5863ca6f5a4a6a6bb835dcd5feeb
> # EXP-Topic single_head_is_boolean
> singlehead: making config item a bool again
> 
> with the use of `configsuboptions`, the main config item has become
> a string (unless it's just the default value).
> 
> This makes it in particular hard to override in a cascade of HGRC files,
> as we do in Heptapod to re-allow multiple heads on specific repositories
> while the default behaviour is to forbid them. The added test case reflects
> that use-case
> 
> diff -r ca3dca416f8d -r c557e15674ea mercurial/localrepo.py
> --- a/mercurial/localrepo.py	Tue Nov 05 21:35:19 2019 +0900
> +++ b/mercurial/localrepo.py	Wed Nov 20 19:07:02 2019 +0100
> @@ -2090,6 +2090,8 @@
>                  b'experimental', b'single-head-per-branch'
>              )
>              singlehead, singleheadsub = r
> +            if singlehead is not None and not isinstance(singlehead, bool):
> +                singlehead = stringutil.parsebool(singlehead)

Maybe it's better to use ui.configbool() to check if the config value can
be parsed as bool.

  singlehead, singleheadsub = ui.configsuboptions(...)
  singlehead = ui.configbool(...)
Georges Racinet - Nov. 21, 2019, 12:50 p.m.
On 11/21/19 1:38 PM, Yuya Nishihara wrote:
> On Wed, 20 Nov 2019 19:23:26 +0100, Georges Racinet wrote:
>> # HG changeset patch
>> # User Georges Racinet <georges.racinet@octobus.net>
>> # Date 1574273222 -3600
>> #      Wed Nov 20 19:07:02 2019 +0100
>> # Branch stable
>> # Node ID c557e15674ea6adfe1b034e5b4af1e26bca850dd
>> # Parent  ca3dca416f8d5863ca6f5a4a6a6bb835dcd5feeb
>> # EXP-Topic single_head_is_boolean
>> singlehead: making config item a bool again
>>
>> with the use of `configsuboptions`, the main config item has become
>> a string (unless it's just the default value).
>>
>> This makes it in particular hard to override in a cascade of HGRC files,
>> as we do in Heptapod to re-allow multiple heads on specific repositories
>> while the default behaviour is to forbid them. The added test case reflects
>> that use-case
>>
>> diff -r ca3dca416f8d -r c557e15674ea mercurial/localrepo.py
>> --- a/mercurial/localrepo.py	Tue Nov 05 21:35:19 2019 +0900
>> +++ b/mercurial/localrepo.py	Wed Nov 20 19:07:02 2019 +0100
>> @@ -2090,6 +2090,8 @@
>>                   b'experimental', b'single-head-per-branch'
>>               )
>>               singlehead, singleheadsub = r
>> +            if singlehead is not None and not isinstance(singlehead, bool):
>> +                singlehead = stringutil.parsebool(singlehead)
Thanks for having looked at this
> Maybe it's better to use ui.configbool() to check if the config value can
> be parsed as bool.
>
>    singlehead, singleheadsub = ui.configsuboptions(...)
>    singlehead = ui.configbool(...)

I wasn't sure whether it'd be a bit slower because of redoing partly 
what `configsuboptions` had already done.

Then why not make explicit that we don't want the first value given by 
`configsuboptions`,, then :

_, singleheadsub = ui.configsuboptions(...)         (or even 
singleheadsub = ui.configsuboptions(...)[1])

singlehead = ui.configbool(...)

I'll amend and resubmit according to your prefererence
Yuya Nishihara - Nov. 21, 2019, 1:12 p.m.
On Thu, 21 Nov 2019 13:50:56 +0100, Georges Racinet wrote:
> On 11/21/19 1:38 PM, Yuya Nishihara wrote:
> > On Wed, 20 Nov 2019 19:23:26 +0100, Georges Racinet wrote:
> >> # HG changeset patch
> >> # User Georges Racinet <georges.racinet@octobus.net>
> >> # Date 1574273222 -3600
> >> #      Wed Nov 20 19:07:02 2019 +0100
> >> # Branch stable
> >> # Node ID c557e15674ea6adfe1b034e5b4af1e26bca850dd
> >> # Parent  ca3dca416f8d5863ca6f5a4a6a6bb835dcd5feeb
> >> # EXP-Topic single_head_is_boolean
> >> singlehead: making config item a bool again
> >>
> >> with the use of `configsuboptions`, the main config item has become
> >> a string (unless it's just the default value).
> >>
> >> This makes it in particular hard to override in a cascade of HGRC files,
> >> as we do in Heptapod to re-allow multiple heads on specific repositories
> >> while the default behaviour is to forbid them. The added test case reflects
> >> that use-case
> >>
> >> diff -r ca3dca416f8d -r c557e15674ea mercurial/localrepo.py
> >> --- a/mercurial/localrepo.py	Tue Nov 05 21:35:19 2019 +0900
> >> +++ b/mercurial/localrepo.py	Wed Nov 20 19:07:02 2019 +0100
> >> @@ -2090,6 +2090,8 @@
> >>                   b'experimental', b'single-head-per-branch'
> >>               )
> >>               singlehead, singleheadsub = r
> >> +            if singlehead is not None and not isinstance(singlehead, bool):
> >> +                singlehead = stringutil.parsebool(singlehead)
> Thanks for having looked at this
> > Maybe it's better to use ui.configbool() to check if the config value can
> > be parsed as bool.
> >
> >    singlehead, singleheadsub = ui.configsuboptions(...)
> >    singlehead = ui.configbool(...)
> 
> I wasn't sure whether it'd be a bit slower because of redoing partly 
> what `configsuboptions` had already done.
> 
> Then why not make explicit that we don't want the first value given by 
> `configsuboptions`,, then :
> 
> _, singleheadsub = ui.configsuboptions(...)         (or even 
> singleheadsub = ui.configsuboptions(...)[1])
> 
> singlehead = ui.configbool(...)
> 
> I'll amend and resubmit according to your prefererence

I have no preference, but we should avoid using _ because it is i18n._().
Georges Racinet - Nov. 21, 2019, 1:14 p.m.
On 11/21/19 2:12 PM, Yuya Nishihara wrote:
> On Thu, 21 Nov 2019 13:50:56 +0100, Georges Racinet wrote:
>> On 11/21/19 1:38 PM, Yuya Nishihara wrote:
>>> On Wed, 20 Nov 2019 19:23:26 +0100, Georges Racinet wrote:
>>>> # HG changeset patch
>>>> # User Georges Racinet <georges.racinet@octobus.net>
>>>> # Date 1574273222 -3600
>>>> #      Wed Nov 20 19:07:02 2019 +0100
>>>> # Branch stable
>>>> # Node ID c557e15674ea6adfe1b034e5b4af1e26bca850dd
>>>> # Parent  ca3dca416f8d5863ca6f5a4a6a6bb835dcd5feeb
>>>> # EXP-Topic single_head_is_boolean
>>>> singlehead: making config item a bool again
>>>>
>>>> with the use of `configsuboptions`, the main config item has become
>>>> a string (unless it's just the default value).
>>>>
>>>> This makes it in particular hard to override in a cascade of HGRC files,
>>>> as we do in Heptapod to re-allow multiple heads on specific repositories
>>>> while the default behaviour is to forbid them. The added test case reflects
>>>> that use-case
>>>>
>>>> diff -r ca3dca416f8d -r c557e15674ea mercurial/localrepo.py
>>>> --- a/mercurial/localrepo.py	Tue Nov 05 21:35:19 2019 +0900
>>>> +++ b/mercurial/localrepo.py	Wed Nov 20 19:07:02 2019 +0100
>>>> @@ -2090,6 +2090,8 @@
>>>>                    b'experimental', b'single-head-per-branch'
>>>>                )
>>>>                singlehead, singleheadsub = r
>>>> +            if singlehead is not None and not isinstance(singlehead, bool):
>>>> +                singlehead = stringutil.parsebool(singlehead)
>> Thanks for having looked at this
>>> Maybe it's better to use ui.configbool() to check if the config value can
>>> be parsed as bool.
>>>
>>>     singlehead, singleheadsub = ui.configsuboptions(...)
>>>     singlehead = ui.configbool(...)
>> I wasn't sure whether it'd be a bit slower because of redoing partly
>> what `configsuboptions` had already done.
>>
>> Then why not make explicit that we don't want the first value given by
>> `configsuboptions`,, then :
>>
>> _, singleheadsub = ui.configsuboptions(...)         (or even
>> singleheadsub = ui.configsuboptions(...)[1])
>>
>> singlehead = ui.configbool(...)
>>
>> I'll amend and resubmit according to your prefererence
> I have no preference, but we should avoid using _ because it is i18n._().
Ah yes, right, I keep forgetting it.
Pierre-Yves David - Nov. 21, 2019, 1:43 p.m.
On 11/21/19 1:50 PM, Georges Racinet wrote:
> On 11/21/19 1:38 PM, Yuya Nishihara wrote:
>> On Wed, 20 Nov 2019 19:23:26 +0100, Georges Racinet wrote:
>>> # HG changeset patch
>>> # User Georges Racinet <georges.racinet@octobus.net>
>>> # Date 1574273222 -3600
>>> #      Wed Nov 20 19:07:02 2019 +0100
>>> # Branch stable
>>> # Node ID c557e15674ea6adfe1b034e5b4af1e26bca850dd
>>> # Parent  ca3dca416f8d5863ca6f5a4a6a6bb835dcd5feeb
>>> # EXP-Topic single_head_is_boolean
>>> singlehead: making config item a bool again
>>>
>>> with the use of `configsuboptions`, the main config item has become
>>> a string (unless it's just the default value).
>>>
>>> This makes it in particular hard to override in a cascade of HGRC files,
>>> as we do in Heptapod to re-allow multiple heads on specific repositories
>>> while the default behaviour is to forbid them. The added test case 
>>> reflects
>>> that use-case
>>>
>>> diff -r ca3dca416f8d -r c557e15674ea mercurial/localrepo.py
>>> --- a/mercurial/localrepo.py    Tue Nov 05 21:35:19 2019 +0900
>>> +++ b/mercurial/localrepo.py    Wed Nov 20 19:07:02 2019 +0100
>>> @@ -2090,6 +2090,8 @@
>>>                   b'experimental', b'single-head-per-branch'
>>>               )
>>>               singlehead, singleheadsub = r
>>> +            if singlehead is not None and not isinstance(singlehead, 
>>> bool):
>>> +                singlehead = stringutil.parsebool(singlehead)
> Thanks for having looked at this
>> Maybe it's better to use ui.configbool() to check if the config value can
>> be parsed as bool.
>>
>>    singlehead, singleheadsub = ui.configsuboptions(...)
>>    singlehead = ui.configbool(...)
> 
> I wasn't sure whether it'd be a bit slower because of redoing partly 
> what `configsuboptions` had already done.

I don't think the performance is a concern here (otherwise there are a 
lot of more important slowdown in that code)

> 
> Then why not make explicit that we don't want the first value given by 
> `configsuboptions`,, then :
> 
> _, singleheadsub = ui.configsuboptions(...)         (or even 
> singleheadsub = ui.configsuboptions(...)[1])
> 
> singlehead = ui.configbool(...)
> 
> I'll amend and resubmit according to your prefererence
>

Patch

diff -r ca3dca416f8d -r c557e15674ea mercurial/localrepo.py
--- a/mercurial/localrepo.py	Tue Nov 05 21:35:19 2019 +0900
+++ b/mercurial/localrepo.py	Wed Nov 20 19:07:02 2019 +0100
@@ -2090,6 +2090,8 @@ 
                 b'experimental', b'single-head-per-branch'
             )
             singlehead, singleheadsub = r
+            if singlehead is not None and not isinstance(singlehead, bool):
+                singlehead = stringutil.parsebool(singlehead)
             if singlehead:
                 accountclosed = singleheadsub.get(
                     b"account-closed-heads", False
diff -r ca3dca416f8d -r c557e15674ea tests/test-single-head.t
--- a/tests/test-single-head.t	Tue Nov 05 21:35:19 2019 +0900
+++ b/tests/test-single-head.t	Wed Nov 20 19:07:02 2019 +0100
@@ -259,3 +259,28 @@ 
   abort: rejecting multiple heads on branch "branch_A"
   (3 heads: 49003e504178 5254bcccab93 42b9fe70a3c1)
   [255]
+
+
+Test that config can be overriden as the boolean it is
+------------------------------------------------------
+
+  $ cat <<EOF >> $TESTTMP/single-head-server/.hg/hgrc
+  > [experimental]
+  > single-head-per-branch = no
+  > EOF
+
+let's make a new, non closed head, but because of previous test, we'll also
+push c_aL0 and c_aM0.
+
+  $ hg out
+  $ hg up 'desc("c_aG0")'
+  0 files updated, 0 files merged, 2 files removed, 0 files unresolved
+  $ mkcommit c_aN0
+  created new head
+  $ hg push -f
+  pushing to $TESTTMP/single-head-server
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 3 changesets with 3 changes to 3 files