Patchwork [4,of,4] bundle2: no longer use 'retractboundary' in updatephases

login
register
mail settings
Submitter Boris Feld
Date July 12, 2017, 3:39 p.m.
Message ID <002ca6e17d6b53123c5a.1499873946@FB>
Download mbox | patch
Permalink /patch/22266/
State Accepted
Headers show

Comments

Boris Feld - July 12, 2017, 3:39 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1499742361 -7200
#      Tue Jul 11 05:06:01 2017 +0200
# Node ID 002ca6e17d6b53123c5ab4a4a46fb3f5d453e072
# Parent  5a2c6c050e33e4f2a571247f5d1e150a3ef1d295
# EXP-Topic tr.changes.phases
bundle2: no longer use 'retractboundary' in updatephases

The new 'phase-heads' forced all added node to secret before advancing the
boundary to work around the fact changesets were added as draft by default.
This is no longer necessary since the changegroup part can now use the
'targetphase' parameter.

Not doing this retract boundary call has a couple of advantages:

* This makes implementing phases change tracking in the transaction much
  simpler since retract boundary can become a rare case.

* Bundling secret changesets is not the norm. Exchange never does that and
  even for strip, the use-case is not common.Skipping the retract boundary
  will avoid useless work here.

* Sending phase update on push can be simplified since we can rely on the
  behavior of 'cg.apply' for most of it.
  This means less phases update send for example.

* We no longer needs to track and use the addednodes during unbundling. This
  make it possible to have multiple 'changegroup' and 'phase-heads' parts in the
  same bundle without them interfering with each others.

The new part has not been part of any release yet so we do not offer backward
compatibility yet. It is important to update this semantic before the 4.3
freeze happens.
Sean Farley - July 13, 2017, 3:29 a.m.
Boris Feld <boris.feld@octobus.net> writes:

> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1499742361 -7200
> #      Tue Jul 11 05:06:01 2017 +0200
> # Node ID 002ca6e17d6b53123c5ab4a4a46fb3f5d453e072
> # Parent  5a2c6c050e33e4f2a571247f5d1e150a3ef1d295
> # EXP-Topic tr.changes.phases
> bundle2: no longer use 'retractboundary' in updatephases
>
> The new 'phase-heads' forced all added node to secret before advancing the
> boundary to work around the fact changesets were added as draft by default.
> This is no longer necessary since the changegroup part can now use the
> 'targetphase' parameter.
>
> Not doing this retract boundary call has a couple of advantages:
>
> * This makes implementing phases change tracking in the transaction much
>   simpler since retract boundary can become a rare case.
>
> * Bundling secret changesets is not the norm. Exchange never does that and
>   even for strip, the use-case is not common.Skipping the retract boundary
>   will avoid useless work here.
>
> * Sending phase update on push can be simplified since we can rely on the
>   behavior of 'cg.apply' for most of it.
>   This means less phases update send for example.
>
> * We no longer needs to track and use the addednodes during unbundling. This
>   make it possible to have multiple 'changegroup' and 'phase-heads' parts in the
>   same bundle without them interfering with each others.
>
> The new part has not been part of any release yet so we do not offer backward
> compatibility yet. It is important to update this semantic before the 4.3
> freeze happens.

Nice, I like where this is going. If I understand correctly, we are no
longer assuming bundles are draft, correct?

This will open the door so that we can track phase change in a
transaction (and I can thereby put that information into a hook) as
well, right?

If so, that's awesome and I'll queue these tomorrow if there's no
objection.
via Mercurial-devel - July 13, 2017, 7:22 a.m.
On Wed, Jul 12, 2017 at 8:29 PM, Sean Farley <sean@farley.io> wrote:
>
> Boris Feld <boris.feld@octobus.net> writes:
>
>> # HG changeset patch
>> # User Boris Feld <boris.feld@octobus.net>
>> # Date 1499742361 -7200
>> #      Tue Jul 11 05:06:01 2017 +0200
>> # Node ID 002ca6e17d6b53123c5ab4a4a46fb3f5d453e072
>> # Parent  5a2c6c050e33e4f2a571247f5d1e150a3ef1d295
>> # EXP-Topic tr.changes.phases
>> bundle2: no longer use 'retractboundary' in updatephases
>>
>> The new 'phase-heads' forced all added node to secret before advancing the
>> boundary to work around the fact changesets were added as draft by default.
>> This is no longer necessary since the changegroup part can now use the
>> 'targetphase' parameter.
>>
>> Not doing this retract boundary call has a couple of advantages:
>>
>> * This makes implementing phases change tracking in the transaction much
>>   simpler since retract boundary can become a rare case.
>>
>> * Bundling secret changesets is not the norm. Exchange never does that and
>>   even for strip, the use-case is not common.Skipping the retract boundary
>>   will avoid useless work here.
>>
>> * Sending phase update on push can be simplified since we can rely on the
>>   behavior of 'cg.apply' for most of it.
>>   This means less phases update send for example.
>>
>> * We no longer needs to track and use the addednodes during unbundling. This
>>   make it possible to have multiple 'changegroup' and 'phase-heads' parts in the
>>   same bundle without them interfering with each others.
>>
>> The new part has not been part of any release yet so we do not offer backward
>> compatibility yet. It is important to update this semantic before the 4.3
>> freeze happens.
>
> Nice, I like where this is going. If I understand correctly, we are no
> longer assuming bundles are draft, correct?
>
> This will open the door so that we can track phase change in a
> transaction (and I can thereby put that information into a hook) as
> well, right?
>
> If so, that's awesome and I'll queue these tomorrow if there's no
> objection.

I feel like we should be able to replace the "targetphase=secret"
parameter by a "dontupdatephasesatall=true" parameter that will be set
when there's a phase-heads part in the bundle. Then there would be
only one phase update. I also feel like changegroup.apply() updating
phases should only be for legacy reasons, kind of how like phases and
bookmarks are exchanged via separate requests if they are not in the
bundle.

>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - July 13, 2017, 4:45 p.m.
On Thu, Jul 13, 2017 at 12:22 AM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Wed, Jul 12, 2017 at 8:29 PM, Sean Farley <sean@farley.io> wrote:
>>
>> Boris Feld <boris.feld@octobus.net> writes:
>>
>>> # HG changeset patch
>>> # User Boris Feld <boris.feld@octobus.net>
>>> # Date 1499742361 -7200
>>> #      Tue Jul 11 05:06:01 2017 +0200
>>> # Node ID 002ca6e17d6b53123c5ab4a4a46fb3f5d453e072
>>> # Parent  5a2c6c050e33e4f2a571247f5d1e150a3ef1d295
>>> # EXP-Topic tr.changes.phases
>>> bundle2: no longer use 'retractboundary' in updatephases
>>>
>>> The new 'phase-heads' forced all added node to secret before advancing the
>>> boundary to work around the fact changesets were added as draft by default.
>>> This is no longer necessary since the changegroup part can now use the
>>> 'targetphase' parameter.
>>>
>>> Not doing this retract boundary call has a couple of advantages:
>>>
>>> * This makes implementing phases change tracking in the transaction much
>>>   simpler since retract boundary can become a rare case.
>>>
>>> * Bundling secret changesets is not the norm. Exchange never does that and
>>>   even for strip, the use-case is not common.Skipping the retract boundary
>>>   will avoid useless work here.
>>>
>>> * Sending phase update on push can be simplified since we can rely on the
>>>   behavior of 'cg.apply' for most of it.
>>>   This means less phases update send for example.
>>>
>>> * We no longer needs to track and use the addednodes during unbundling. This
>>>   make it possible to have multiple 'changegroup' and 'phase-heads' parts in the
>>>   same bundle without them interfering with each others.
>>>
>>> The new part has not been part of any release yet so we do not offer backward
>>> compatibility yet. It is important to update this semantic before the 4.3
>>> freeze happens.
>>
>> Nice, I like where this is going. If I understand correctly, we are no
>> longer assuming bundles are draft, correct?
>>
>> This will open the door so that we can track phase change in a
>> transaction (and I can thereby put that information into a hook) as
>> well, right?
>>
>> If so, that's awesome and I'll queue these tomorrow if there's no
>> objection.
>
> I feel like we should be able to replace the "targetphase=secret"
> parameter by a "dontupdatephasesatall=true" parameter that will be set
> when there's a phase-heads part in the bundle. Then there would be
> only one phase update. I also feel like changegroup.apply() updating
> phases should only be for legacy reasons, kind of how like phases and
> bookmarks are exchanged via separate requests if they are not in the
> bundle.

I think I understand this better now and I agree with the patches.
Queued, thanks!

One thing that helped me understand the issues was to think about
multiple changegroups and/or multiple phase-heads in a bundle (as I
now see you also mentioned above). If there are multiple of each and
we skipped updating phases completely when processing the changegroup
parts, when applying the second phase-heads part, it would retract
phase boundaries of all the added nodes, possibly undoing the phase
advancement of a previous phase-heads part. If we want to not update
phases when processing the changegroup part, we would thus need to
pair the phase-heads parts up with the changegroups parts, which would
be really awkward.


>
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
Sean Farley - July 13, 2017, 5:47 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Thu, Jul 13, 2017 at 12:22 AM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>> On Wed, Jul 12, 2017 at 8:29 PM, Sean Farley <sean@farley.io> wrote:
>>>
>>> Boris Feld <boris.feld@octobus.net> writes:
>>>
>>>> # HG changeset patch
>>>> # User Boris Feld <boris.feld@octobus.net>
>>>> # Date 1499742361 -7200
>>>> #      Tue Jul 11 05:06:01 2017 +0200
>>>> # Node ID 002ca6e17d6b53123c5ab4a4a46fb3f5d453e072
>>>> # Parent  5a2c6c050e33e4f2a571247f5d1e150a3ef1d295
>>>> # EXP-Topic tr.changes.phases
>>>> bundle2: no longer use 'retractboundary' in updatephases
>>>>
>>>> The new 'phase-heads' forced all added node to secret before advancing the
>>>> boundary to work around the fact changesets were added as draft by default.
>>>> This is no longer necessary since the changegroup part can now use the
>>>> 'targetphase' parameter.
>>>>
>>>> Not doing this retract boundary call has a couple of advantages:
>>>>
>>>> * This makes implementing phases change tracking in the transaction much
>>>>   simpler since retract boundary can become a rare case.
>>>>
>>>> * Bundling secret changesets is not the norm. Exchange never does that and
>>>>   even for strip, the use-case is not common.Skipping the retract boundary
>>>>   will avoid useless work here.
>>>>
>>>> * Sending phase update on push can be simplified since we can rely on the
>>>>   behavior of 'cg.apply' for most of it.
>>>>   This means less phases update send for example.
>>>>
>>>> * We no longer needs to track and use the addednodes during unbundling. This
>>>>   make it possible to have multiple 'changegroup' and 'phase-heads' parts in the
>>>>   same bundle without them interfering with each others.
>>>>
>>>> The new part has not been part of any release yet so we do not offer backward
>>>> compatibility yet. It is important to update this semantic before the 4.3
>>>> freeze happens.
>>>
>>> Nice, I like where this is going. If I understand correctly, we are no
>>> longer assuming bundles are draft, correct?
>>>
>>> This will open the door so that we can track phase change in a
>>> transaction (and I can thereby put that information into a hook) as
>>> well, right?
>>>
>>> If so, that's awesome and I'll queue these tomorrow if there's no
>>> objection.
>>
>> I feel like we should be able to replace the "targetphase=secret"
>> parameter by a "dontupdatephasesatall=true" parameter that will be set
>> when there's a phase-heads part in the bundle. Then there would be
>> only one phase update. I also feel like changegroup.apply() updating
>> phases should only be for legacy reasons, kind of how like phases and
>> bookmarks are exchanged via separate requests if they are not in the
>> bundle.
>
> I think I understand this better now and I agree with the patches.
> Queued, thanks!
>
> One thing that helped me understand the issues was to think about
> multiple changegroups and/or multiple phase-heads in a bundle (as I
> now see you also mentioned above). If there are multiple of each and
> we skipped updating phases completely when processing the changegroup
> parts, when applying the second phase-heads part, it would retract
> phase boundaries of all the added nodes, possibly undoing the phase
> advancement of a previous phase-heads part. If we want to not update
> phases when processing the changegroup part, we would thus need to
> pair the phase-heads parts up with the changegroups parts, which would
> be really awkward.

Ah, that's a nice summary! Thanks, Martin!

Patch

diff -r 5a2c6c050e33 -r 002ca6e17d6b mercurial/phases.py
--- a/mercurial/phases.py	Tue Jul 11 05:12:03 2017 +0200
+++ b/mercurial/phases.py	Tue Jul 11 05:06:01 2017 +0200
@@ -448,10 +448,6 @@ 
 
 def updatephases(repo, tr, headsbyphase, addednodes):
     """Updates the repo with the given phase heads"""
-    # First make all the added revisions secret because changegroup.apply()
-    # currently sets the phase to draft.
-    retractboundary(repo, tr, secret, addednodes)
-
     # Now advance phase boundaries of all but secret phase
     for phase in allphases[:-1]:
         advanceboundary(repo, tr, phase, headsbyphase[phase])