Patchwork [6,of,6] check-concurrency: expose the feature as 'server.check-concurrency'

login
register
mail settings
Submitter Pierre-Yves David
Date June 4, 2017, 2:49 p.m.
Message ID <c58a31911d96c8fbb083.1496587774@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/21173/
State Accepted
Headers show

Comments

Pierre-Yves David - June 4, 2017, 2:49 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1495923158 -7200
#      Sun May 28 00:12:38 2017 +0200
# Node ID c58a31911d96c8fbb083388e55bf22cc473cc7b5
# Parent  ae88951457de93c7f6286d449672b0b9d20c57f1
# EXP-Topic pushrace
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r c58a31911d96
check-concurrency: expose the feature as 'server.check-concurrency'

We move the feature to a proper configuration and document it. The config goes
in the 'server' section because it feels like something the server owner would
want to decide. We pick and open field because it seems likely that other
checking levels will emerge in the future. (eg: server like the mozilla-try
server will likely wants a "none" value)
Yuya Nishihara - June 7, 2017, 2:29 p.m.
On Sun, 04 Jun 2017 15:49:34 +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1495923158 -7200
> #      Sun May 28 00:12:38 2017 +0200
> # Node ID c58a31911d96c8fbb083388e55bf22cc473cc7b5
> # Parent  ae88951457de93c7f6286d449672b0b9d20c57f1
> # EXP-Topic pushrace
> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r c58a31911d96
> check-concurrency: expose the feature as 'server.check-concurrency'

> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -1668,6 +1668,16 @@ Controls generic server settings.
>      are highly recommended. Partial clones will still be allowed.
>      (default: False)
>  
> +``check-concurrency``
> +    Level of allowed race condition between two pushing client.
> +    - 'strict': push is abort if another client touched the repository
> +      while the push was preparing. (default)
> +    - 'related': push is only aborted if it affects head that got also
> +      affected while the push was preparing.
> +
> +    This requires compatible client (version 4.3 and later). Old client will
> +    use 'strict'.

I feel "race" is better than "concurrency" since I think "concurrency" doesn't
have any negative meaning. But obviously I'm not the right person to review
this patch. :)
Augie Fackler - June 7, 2017, 6:52 p.m.
> On Jun 7, 2017, at 10:29, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Sun, 04 Jun 2017 15:49:34 +0100, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>> # Date 1495923158 -7200
>> #      Sun May 28 00:12:38 2017 +0200
>> # Node ID c58a31911d96c8fbb083388e55bf22cc473cc7b5
>> # Parent  ae88951457de93c7f6286d449672b0b9d20c57f1
>> # EXP-Topic pushrace
>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r c58a31911d96
>> check-concurrency: expose the feature as 'server.check-concurrency'
> 
>> --- a/mercurial/help/config.txt
>> +++ b/mercurial/help/config.txt
>> @@ -1668,6 +1668,16 @@ Controls generic server settings.
>>     are highly recommended. Partial clones will still be allowed.
>>     (default: False)
>> 
>> +``check-concurrency``
>> +    Level of allowed race condition between two pushing client.
>> +    - 'strict': push is abort if another client touched the repository
>> +      while the push was preparing. (default)
>> +    - 'related': push is only aborted if it affects head that got also
>> +      affected while the push was preparing.
>> +
>> +    This requires compatible client (version 4.3 and later). Old client will
>> +    use 'strict'.
> 
> I feel "race" is better than "concurrency" since I think "concurrency" doesn't
> have any negative meaning. But obviously I'm not the right person to review
> this patch. :)

This is tricky. race condition implies to my brain that it's always bad, which isn't necessarily the right feeling here. I might call it concurrent-push-mode or something (since it's only for pushes, and it's not strictly races either I guess)?

Naming is hard. :(

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - June 8, 2017, 3:58 p.m.
On Wed, 7 Jun 2017 14:52:18 -0400, Augie Fackler wrote:
> > On Jun 7, 2017, at 10:29, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Sun, 04 Jun 2017 15:49:34 +0100, Pierre-Yves David wrote:
> >> # HG changeset patch
> >> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> >> # Date 1495923158 -7200
> >> #      Sun May 28 00:12:38 2017 +0200
> >> # Node ID c58a31911d96c8fbb083388e55bf22cc473cc7b5
> >> # Parent  ae88951457de93c7f6286d449672b0b9d20c57f1
> >> # EXP-Topic pushrace
> >> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
> >> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r c58a31911d96
> >> check-concurrency: expose the feature as 'server.check-concurrency'
> > 
> >> --- a/mercurial/help/config.txt
> >> +++ b/mercurial/help/config.txt
> >> @@ -1668,6 +1668,16 @@ Controls generic server settings.
> >>     are highly recommended. Partial clones will still be allowed.
> >>     (default: False)
> >> 
> >> +``check-concurrency``
> >> +    Level of allowed race condition between two pushing client.
> >> +    - 'strict': push is abort if another client touched the repository
> >> +      while the push was preparing. (default)
> >> +    - 'related': push is only aborted if it affects head that got also
> >> +      affected while the push was preparing.
> >> +
> >> +    This requires compatible client (version 4.3 and later). Old client will
> >> +    use 'strict'.
> > 
> > I feel "race" is better than "concurrency" since I think "concurrency" doesn't
> > have any negative meaning. But obviously I'm not the right person to review
> > this patch. :)
> 
> This is tricky. race condition implies to my brain that it's always bad, which isn't necessarily the right feeling here.

Maybe I take the current level as check-race=paranoid.

> I might call it concurrent-push-mode or something (since it's only for pushes, and it's not strictly races either I guess)?

Sounds good.
Pierre-Yves David - June 9, 2017, 1:29 p.m.
On 06/08/2017 04:58 PM, Yuya Nishihara wrote:
> On Wed, 7 Jun 2017 14:52:18 -0400, Augie Fackler wrote:
>>> On Jun 7, 2017, at 10:29, Yuya Nishihara <yuya@tcha.org> wrote:
>>> On Sun, 04 Jun 2017 15:49:34 +0100, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>> # Date 1495923158 -7200
>>>> #      Sun May 28 00:12:38 2017 +0200
>>>> # Node ID c58a31911d96c8fbb083388e55bf22cc473cc7b5
>>>> # Parent  ae88951457de93c7f6286d449672b0b9d20c57f1
>>>> # EXP-Topic pushrace
>>>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r c58a31911d96
>>>> check-concurrency: expose the feature as 'server.check-concurrency'
>>>
>>>> --- a/mercurial/help/config.txt
>>>> +++ b/mercurial/help/config.txt
>>>> @@ -1668,6 +1668,16 @@ Controls generic server settings.
>>>>     are highly recommended. Partial clones will still be allowed.
>>>>     (default: False)
>>>>
>>>> +``check-concurrency``
>>>> +    Level of allowed race condition between two pushing client.
>>>> +    - 'strict': push is abort if another client touched the repository
>>>> +      while the push was preparing. (default)
>>>> +    - 'related': push is only aborted if it affects head that got also
>>>> +      affected while the push was preparing.
>>>> +
>>>> +    This requires compatible client (version 4.3 and later). Old client will
>>>> +    use 'strict'.
>>>
>>> I feel "race" is better than "concurrency" since I think "concurrency" doesn't
>>> have any negative meaning. But obviously I'm not the right person to review
>>> this patch. :)
>>
>> This is tricky. race condition implies to my brain that it's always bad, which isn't necessarily the right feeling here.
>
> Maybe I take the current level as check-race=paranoid.

To me, paranoid also has negative connotation, while the current mode is 
perfectly fine and even needed if people do extra checking logic during 
push.

>> I might call it concurrent-push-mode or something (since it's only for pushes, and it's not strictly races either I guess)?

"concurrent-push-mode" is not bad. What about the value?

concurrent-push-mode=strict  # current behavior, refuse any concurrency
concurrent-push-mode= unrelated  # allow push on unrelated heads
Augie Fackler - June 9, 2017, 2:02 p.m.
On Jun 9, 2017 09:29, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:



On 06/08/2017 04:58 PM, Yuya Nishihara wrote:

> On Wed, 7 Jun 2017 14:52:18 -0400, Augie Fackler wrote:
>
>> On Jun 7, 2017, at 10:29, Yuya Nishihara <yuya@tcha.org> wrote:
>>> On Sun, 04 Jun 2017 15:49:34 +0100, Pierre-Yves David wrote:
>>>
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>> # Date 1495923158 -7200
>>>> #      Sun May 28 00:12:38 2017 +0200
>>>> # Node ID c58a31911d96c8fbb083388e55bf22cc473cc7b5
>>>> # Parent  ae88951457de93c7f6286d449672b0b9d20c57f1
>>>> # EXP-Topic pushrace
>>>> # Available At https://www.mercurial-scm.org/
>>>> repo/users/marmoute/mercurial/
>>>> #              hg pull https://www.mercurial-scm.org/
>>>> repo/users/marmoute/mercurial/ -r c58a31911d96
>>>> check-concurrency: expose the feature as 'server.check-concurrency'
>>>>
>>>
>>> --- a/mercurial/help/config.txt
>>>> +++ b/mercurial/help/config.txt
>>>> @@ -1668,6 +1668,16 @@ Controls generic server settings.
>>>>     are highly recommended. Partial clones will still be allowed.
>>>>     (default: False)
>>>>
>>>> +``check-concurrency``
>>>> +    Level of allowed race condition between two pushing client.
>>>> +    - 'strict': push is abort if another client touched the repository
>>>> +      while the push was preparing. (default)
>>>> +    - 'related': push is only aborted if it affects head that got also
>>>> +      affected while the push was preparing.
>>>> +
>>>> +    This requires compatible client (version 4.3 and later). Old
>>>> client will
>>>> +    use 'strict'.
>>>>
>>>
>>> I feel "race" is better than "concurrency" since I think "concurrency"
>>> doesn't
>>> have any negative meaning. But obviously I'm not the right person to
>>> review
>>> this patch. :)
>>>
>>
>> This is tricky. race condition implies to my brain that it's always bad,
>> which isn't necessarily the right feeling here.
>>
>
> Maybe I take the current level as check-race=paranoid.
>

To me, paranoid also has negative connotation, while the current mode is
perfectly fine and even needed if people do extra checking logic during
push.


I might call it concurrent-push-mode or something (since it's only for
>> pushes, and it's not strictly races either I guess)?
>>
>
"concurrent-push-mode" is not bad. What about the value?

concurrent-push-mode=strict  # current behavior, refuse any concurrency
concurrent-push-mode= unrelated  # allow push on unrelated heads


These seem good to me.
Pierre-Yves David - June 11, 2017, 8:48 p.m.
On 06/09/2017 04:02 PM, Augie Fackler wrote:
> On Jun 9, 2017 09:29, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
> wrote:
>
>
>
> On 06/08/2017 04:58 PM, Yuya Nishihara wrote:
>
>> On Wed, 7 Jun 2017 14:52:18 -0400, Augie Fackler wrote:
>>
>>> On Jun 7, 2017, at 10:29, Yuya Nishihara <yuya@tcha.org> wrote:
>>>> On Sun, 04 Jun 2017 15:49:34 +0100, Pierre-Yves David wrote:
>>>>
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>>> # Date 1495923158 -7200
>>>>> #      Sun May 28 00:12:38 2017 +0200
>>>>> # Node ID c58a31911d96c8fbb083388e55bf22cc473cc7b5
>>>>> # Parent  ae88951457de93c7f6286d449672b0b9d20c57f1
>>>>> # EXP-Topic pushrace
>>>>> # Available At https://www.mercurial-scm.org/
>>>>> repo/users/marmoute/mercurial/
>>>>> #              hg pull https://www.mercurial-scm.org/
>>>>> repo/users/marmoute/mercurial/ -r c58a31911d96
>>>>> check-concurrency: expose the feature as 'server.check-concurrency'
>>>>>
>>>>
>>>> --- a/mercurial/help/config.txt
>>>>> +++ b/mercurial/help/config.txt
>>>>> @@ -1668,6 +1668,16 @@ Controls generic server settings.
>>>>>     are highly recommended. Partial clones will still be allowed.
>>>>>     (default: False)
>>>>>
>>>>> +``check-concurrency``
>>>>> +    Level of allowed race condition between two pushing client.
>>>>> +    - 'strict': push is abort if another client touched the repository
>>>>> +      while the push was preparing. (default)
>>>>> +    - 'related': push is only aborted if it affects head that got also
>>>>> +      affected while the push was preparing.
>>>>> +
>>>>> +    This requires compatible client (version 4.3 and later). Old
>>>>> client will
>>>>> +    use 'strict'.
>>>>>
>>>>
>>>> I feel "race" is better than "concurrency" since I think "concurrency"
>>>> doesn't
>>>> have any negative meaning. But obviously I'm not the right person to
>>>> review
>>>> this patch. :)
>>>>
>>>
>>> This is tricky. race condition implies to my brain that it's always bad,
>>> which isn't necessarily the right feeling here.
>>>
>>
>> Maybe I take the current level as check-race=paranoid.
>>
>
> To me, paranoid also has negative connotation, while the current mode is
> perfectly fine and even needed if people do extra checking logic during
> push.
>
>
> I might call it concurrent-push-mode or something (since it's only for
>>> pushes, and it's not strictly races either I guess)?
>>>
>>
> "concurrent-push-mode" is not bad. What about the value?
>
> concurrent-push-mode=strict  # current behavior, refuse any concurrency
> concurrent-push-mode= unrelated  # allow push on unrelated heads

I slept over this before sending an updated patch and I think there is 
small bit still missing. The latest proposal is a bit ambiguous in term 
of value meaning.

The initial proposal had a "verb" and an "object":

   check-concurrency=stric # "check" is "strict"
   check-concurrency=related # "check" if "related".

The latest one is a bit less clear, since we kinda lost the
"verb".

   concurrent-push-mode=strict # "mode" is "strict" (okay)
   concurrent-push-mode=related # "mode" is "related" what does this mean?

Maybe we should switch the "logic" of the "value":

   concurrent-push-mode=related # "unrelated" push can be concurrent

Or makes it more verbose:

   concurrent-push-mode=allow-unrelated

(or)

   concurrent-push-mode=deny-related

What do you all think?

Cheers,
Augie Fackler - June 13, 2017, 10:47 p.m.
> On Jun 11, 2017, at 16:48, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> I slept over this before sending an updated patch and I think there is small bit still missing. The latest proposal is a bit ambiguous in term of value meaning.
> 
> The initial proposal had a "verb" and an "object":
> 
>  check-concurrency=stric # "check" is "strict"
>  check-concurrency=related # "check" if "related".
> 
> The latest one is a bit less clear, since we kinda lost the
> "verb".
> 
>  concurrent-push-mode=strict # "mode" is "strict" (okay)
>  concurrent-push-mode=related # "mode" is "related" what does this mean?
> 
> Maybe we should switch the "logic" of the "value":
> 
>  concurrent-push-mode=related # "unrelated" push can be concurrent
> 
> Or makes it more verbose:
> 
>  concurrent-push-mode=allow-unrelated
> 
> (or)
> 
>  concurrent-push-mode=deny-related
> 
> What do you all think?

That seems like an improvement to me.

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1323,7 +1323,7 @@  def getrepocaps(repo, allowpushback=Fals
         caps['obsmarkers'] = supportedformat
     if allowpushback:
         caps['pushback'] = ()
-    if not repo.ui.configbool('experimental', 'checkheads-strict', True):
+    if repo.ui.config('server', 'check-concurrency', 'strict') == 'related':
         caps['checkheads'] = ('related',)
     return caps
 
diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -1668,6 +1668,16 @@  Controls generic server settings.
     are highly recommended. Partial clones will still be allowed.
     (default: False)
 
+``check-concurrency``
+    Level of allowed race condition between two pushing client.
+    - 'strict': push is abort if another client touched the repository
+      while the push was preparing. (default)
+    - 'related': push is only aborted if it affects head that got also
+      affected while the push was preparing.
+
+    This requires compatible client (version 4.3 and later). Old client will
+    use 'strict'.
+
 ``validate``
     Whether to validate the completeness of pushed changesets by
     checking that all new file revisions specified in manifests are
diff --git a/tests/test-push-race.t b/tests/test-push-race.t
--- a/tests/test-push-race.t
+++ b/tests/test-push-race.t
@@ -111,8 +111,8 @@  We tests multiple cases:
 #if unrelated
 
   $ cat >> $HGRCPATH << EOF
-  > [experimental]
-  > checkheads-strict = no
+  > [server]
+  > check-concurrency = related
   > EOF
 
 #endif