Patchwork bundle2: don't assume ordering of heads checked after push

login
register
mail settings
Submitter Mads Kiilerich
Date June 1, 2016, 7:41 p.m.
Message ID <8e8f644bb2b3ff59af75.1464810096@madski>
Download mbox | patch
Permalink /patch/15324/
State Accepted
Headers show

Comments

Mads Kiilerich - June 1, 2016, 7:41 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1464810052 -7200
#      Wed Jun 01 21:40:52 2016 +0200
# Branch stable
# Node ID 8e8f644bb2b3ff59af75d28056fd88481aca7a01
# Parent  a9764ab80e11bcf6a37255db7dd079011f767c6c
bundle2: don't assume ordering of heads checked after push

Usually, the heads will have the same ordering in handlecheckheads. Insisting
on the same ordering is however an unnecessary constraint that in some custom
cases can cause pushes to fail even though the actual heads didn't change. This
caused production issues for us in combination with the current version of
https://bitbucket.org/Unity-Technologies/hgwebcachingproxy/ .
Mads Kiilerich - June 1, 2016, 7:44 p.m.
On 06/01/2016 09:41 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1464810052 -7200
> #      Wed Jun 01 21:40:52 2016 +0200
> # Branch stable
> # Node ID 8e8f644bb2b3ff59af75d28056fd88481aca7a01
> # Parent  a9764ab80e11bcf6a37255db7dd079011f767c6c
> bundle2: don't assume ordering of heads checked after push

I forgot the [stable] tag.

> Usually, the heads will have the same ordering in handlecheckheads. Insisting
> on the same ordering is however an unnecessary constraint that in some custom
> cases can cause pushes to fail even though the actual heads didn't change. This
> caused production issues for us in combination with the current version of
> https://bitbucket.org/Unity-Technologies/hgwebcachingproxy/ .
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1453,7 +1453,7 @@ def handlecheckheads(op, inpart):
>       # Trigger a transaction so that we are guaranteed to have the lock now.
>       if op.ui.configbool('experimental', 'bundle2lazylocking'):
>           op.gettransaction()
> -    if heads != op.repo.heads():
> +    if sorted(heads) != sorted(op.repo.heads()):

This could use set() instead of sorted() - I don't know which should be 
preferred.

>           raise error.PushRaced('repository changed while pushing - '
>                                 'please try again')
>   


/Mads
Pierre-Yves David - June 2, 2016, 12:36 a.m.
On 06/01/2016 09:44 PM, Mads Kiilerich wrote:
> On 06/01/2016 09:41 PM, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1464810052 -7200
>> #      Wed Jun 01 21:40:52 2016 +0200
>> # Branch stable
>> # Node ID 8e8f644bb2b3ff59af75d28056fd88481aca7a01
>> # Parent  a9764ab80e11bcf6a37255db7dd079011f767c6c
>> bundle2: don't assume ordering of heads checked after push
> 
> I forgot the [stable] tag.
> 
>> Usually, the heads will have the same ordering in handlecheckheads.
>> Insisting
>> on the same ordering is however an unnecessary constraint that in some
>> custom
>> cases can cause pushes to fail even though the actual heads didn't
>> change. This
>> caused production issues for us in combination with the current
>> version of
>> https://bitbucket.org/Unity-Technologies/hgwebcachingproxy/ .
>>
>> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>> --- a/mercurial/bundle2.py
>> +++ b/mercurial/bundle2.py
>> @@ -1453,7 +1453,7 @@ def handlecheckheads(op, inpart):
>>       # Trigger a transaction so that we are guaranteed to have the
>> lock now.
>>       if op.ui.configbool('experimental', 'bundle2lazylocking'):
>>           op.gettransaction()
>> -    if heads != op.repo.heads():
>> +    if sorted(heads) != sorted(op.repo.heads()):
> 
> This could use set() instead of sorted() - I don't know which should be
> preferred.

I slightly suspect set to be better, sorted will create a new object
anyway and the comparison for set might be better? But this is a wild
guess on my side.
Henrik Stuart - June 3, 2016, 6:25 a.m.
On 01-06-2016 21:44, Mads Kiilerich wrote:
> On 06/01/2016 09:41 PM, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1464810052 -7200
>> #      Wed Jun 01 21:40:52 2016 +0200
>> # Branch stable
>> # Node ID 8e8f644bb2b3ff59af75d28056fd88481aca7a01
>> # Parent  a9764ab80e11bcf6a37255db7dd079011f767c6c
>> bundle2: don't assume ordering of heads checked after push
>
> I forgot the [stable] tag.
>
>> Usually, the heads will have the same ordering in handlecheckheads.
>> Insisting
>> on the same ordering is however an unnecessary constraint that in some
>> custom
>> cases can cause pushes to fail even though the actual heads didn't
>> change. This
>> caused production issues for us in combination with the current
>> version of
>> https://bitbucket.org/Unity-Technologies/hgwebcachingproxy/ .
>>
>> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>> --- a/mercurial/bundle2.py
>> +++ b/mercurial/bundle2.py
>> @@ -1453,7 +1453,7 @@ def handlecheckheads(op, inpart):
>>       # Trigger a transaction so that we are guaranteed to have the
>> lock now.
>>       if op.ui.configbool('experimental', 'bundle2lazylocking'):
>>           op.gettransaction()
>> -    if heads != op.repo.heads():
>> +    if sorted(heads) != sorted(op.repo.heads()):
>
> This could use set() instead of sorted() - I don't know which should be
> preferred.
>
>>           raise error.PushRaced('repository changed while pushing - '
>>                                 'please try again')
>>

Assuming that the local ordering of heads for a given bundle is the same 
as the current local ordering of heads of a repository is a regression 
from bundle1 that did this:

heads = repo.heads()
heads_hash = util.sha1(''.join(sorted(heads))).digest()

The sort is, by the way, over the changeset hashes, not the internal 
revision numbers like it is with bundle2.

In related notes, it seems a bit too conservative that we check that 
/all/ the heads have stayed the same - it is only the heads that we 
depend upon for the new heads being pushed that need to stay the same, 
but that should be addressed separately from this fix (and obviously not 
on stable).
Augie Fackler - June 4, 2016, 2:36 a.m.
On Wed, Jun 01, 2016 at 09:41:36PM +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1464810052 -7200
> #      Wed Jun 01 21:40:52 2016 +0200
> # Branch stable
> # Node ID 8e8f644bb2b3ff59af75d28056fd88481aca7a01
> # Parent  a9764ab80e11bcf6a37255db7dd079011f767c6c
> bundle2: don't assume ordering of heads checked after push

Queued for stable, thanks.

>
> Usually, the heads will have the same ordering in handlecheckheads. Insisting
> on the same ordering is however an unnecessary constraint that in some custom
> cases can cause pushes to fail even though the actual heads didn't change. This
> caused production issues for us in combination with the current version of
> https://bitbucket.org/Unity-Technologies/hgwebcachingproxy/ .
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1453,7 +1453,7 @@ def handlecheckheads(op, inpart):
>      # Trigger a transaction so that we are guaranteed to have the lock now.
>      if op.ui.configbool('experimental', 'bundle2lazylocking'):
>          op.gettransaction()
> -    if heads != op.repo.heads():
> +    if sorted(heads) != sorted(op.repo.heads()):
>          raise error.PushRaced('repository changed while pushing - '
>                                'please try again')
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1453,7 +1453,7 @@  def handlecheckheads(op, inpart):
     # Trigger a transaction so that we are guaranteed to have the lock now.
     if op.ui.configbool('experimental', 'bundle2lazylocking'):
         op.gettransaction()
-    if heads != op.repo.heads():
+    if sorted(heads) != sorted(op.repo.heads()):
         raise error.PushRaced('repository changed while pushing - '
                               'please try again')