| 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
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
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.
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).
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')