Patchwork rebase: update _rebaseset inside transaction

login
register
mail settings
Submitter Durham Goode
Date Nov. 10, 2016, 11:56 a.m.
Message ID <f3bfa374cea66cd5bf17.1478778981@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/17444/
State Superseded
Headers show

Comments

Durham Goode - Nov. 10, 2016, 11:56 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1478778918 28800
#      Thu Nov 10 03:55:18 2016 -0800
# Node ID f3bfa374cea66cd5bf17645ccb6eef60c4d8a5aa
# Parent  3fd53cc1aad882ac9191d7388885acdbbc2d7103
rebase: update _rebaseset inside transaction

Rebase has the concept of the _rebaseset, which is used to prevent the commits
being rebased from being hidden until after the rebase is complete. Previously,
the _rebaseset was being cleared at the very end of rebase, after the last
transaction had already closed. This meant that the repo filteredrevs cache was
not updated (since no invalidation had been triggered, since we're outside of a
transaction), which meant future changelog reads saw commits as visible that
should've been hidden.

This patch moves the _rebaseset clearing to be inside the last rebase
transaction, so that after the transaction the filteredrevs is appropriately
invalidated and will be up-to-date on the next read.

This showed up in an extension that combines rebase, obsolete, and inhibit, so
I'm not sure how to create a test case in hg core to repro it. But we have a
test case in the extension repo.
Kostia Balytskyi - Nov. 10, 2016, 1:08 p.m.
This looks good to me.


On 11/10/16, 11:56 AM, "Mercurial-devel on behalf of Durham Goode" <mercurial-devel-bounces@mercurial-scm.org on behalf of durham@fb.com> wrote:

    # HG changeset patch
    # User Durham Goode <durham@fb.com>
    # Date 1478778918 28800
    #      Thu Nov 10 03:55:18 2016 -0800
    # Node ID f3bfa374cea66cd5bf17645ccb6eef60c4d8a5aa
    # Parent  3fd53cc1aad882ac9191d7388885acdbbc2d7103
    rebase: update _rebaseset inside transaction
    
    Rebase has the concept of the _rebaseset, which is used to prevent the commits
    being rebased from being hidden until after the rebase is complete. Previously,
    the _rebaseset was being cleared at the very end of rebase, after the last
    transaction had already closed. This meant that the repo filteredrevs cache was
    not updated (since no invalidation had been triggered, since we're outside of a
    transaction), which meant future changelog reads saw commits as visible that
    should've been hidden.
    
    This patch moves the _rebaseset clearing to be inside the last rebase
    transaction, so that after the transaction the filteredrevs is appropriately
    invalidated and will be up-to-date on the next read.
    
    This showed up in an extension that combines rebase, obsolete, and inhibit, so
    I'm not sure how to create a test case in hg core to repro it. But we have a
    test case in the extension repo.
    
    diff --git a/hgext/rebase.py b/hgext/rebase.py
    --- a/hgext/rebase.py
    +++ b/hgext/rebase.py
    @@ -495,6 +495,12 @@ class rebaseruntime(object):
                     if self.activebookmark not in repo._bookmarks:
                         # active bookmark was divergent one and has been deleted
                         self.activebookmark = None
    +            # Clear the _rebaseset as part of the transaction, so
    +            # post-transaction hooks can see the final repo visibility state.
    +            # (We could've moved clearstatus() into the transaction to do this,
    +            # but it contains non-transactional changes, like unlinking files).
    +            _clearrebasesetvisibiliy(repo)
    +
             clearstatus(repo)
             clearcollapsemsg(repo)
     
    _______________________________________________
    Mercurial-devel mailing list
    Mercurial-devel@mercurial-scm.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DQIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Pp-gQYFgs4tKlSFPF5kfCw&m=xBhMhQEBjcvk2_j-7abA2vhcPvMIrWwmaYKktXxNFrE&s=EeMzphKg1_owVFcLHqcurMoyC7PVMQvZB97lXb7W9f8&e=
Pierre-Yves David - Nov. 10, 2016, 1:17 p.m.
On 11/10/2016 11:56 AM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1478778918 28800
> #      Thu Nov 10 03:55:18 2016 -0800
> # Node ID f3bfa374cea66cd5bf17645ccb6eef60c4d8a5aa
> # Parent  3fd53cc1aad882ac9191d7388885acdbbc2d7103
> rebase: update _rebaseset inside transaction
>
> Rebase has the concept of the _rebaseset, which is used to prevent the commits
> being rebased from being hidden until after the rebase is complete. Previously,
> the _rebaseset was being cleared at the very end of rebase, after the last
> transaction had already closed. This meant that the repo filteredrevs cache was
> not updated (since no invalidation had been triggered, since we're outside of a
> transaction), which meant future changelog reads saw commits as visible that
> should've been hidden.
>
> This patch moves the _rebaseset clearing to be inside the last rebase
> transaction, so that after the transaction the filteredrevs is appropriately
> invalidated and will be up-to-date on the next read.
>
> This showed up in an extension that combines rebase, obsolete, and inhibit, so
> I'm not sure how to create a test case in hg core to repro it. But we have a
> test case in the extension repo.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -495,6 +495,12 @@ class rebaseruntime(object):
>                  if self.activebookmark not in repo._bookmarks:
>                      # active bookmark was divergent one and has been deleted
>                      self.activebookmark = None
> +            # Clear the _rebaseset as part of the transaction, so
> +            # post-transaction hooks can see the final repo visibility state.
> +            # (We could've moved clearstatus() into the transaction to do this,
> +            # but it contains non-transactional changes, like unlinking files).
> +            _clearrebasesetvisibiliy(repo)
> +
>          clearstatus(repo)
>          clearcollapsemsg(repo)

The clearing of the "rebased" revision happens a couple of line higher 
(line 489). Would it make more sense to clear the visible set at the 
same time instead of the proposed spot ?

Cheers,
Durham Goode - Nov. 10, 2016, 2:42 p.m.
On 11/10/16 1:17 PM, Pierre-Yves David wrote:
>
>
> On 11/10/2016 11:56 AM, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1478778918 28800
>> #      Thu Nov 10 03:55:18 2016 -0800
>> # Node ID f3bfa374cea66cd5bf17645ccb6eef60c4d8a5aa
>> # Parent  3fd53cc1aad882ac9191d7388885acdbbc2d7103
>> rebase: update _rebaseset inside transaction
>>
>> Rebase has the concept of the _rebaseset, which is used to prevent 
>> the commits
>> being rebased from being hidden until after the rebase is complete. 
>> Previously,
>> the _rebaseset was being cleared at the very end of rebase, after the 
>> last
>> transaction had already closed. This meant that the repo filteredrevs 
>> cache was
>> not updated (since no invalidation had been triggered, since we're 
>> outside of a
>> transaction), which meant future changelog reads saw commits as 
>> visible that
>> should've been hidden.
>>
>> This patch moves the _rebaseset clearing to be inside the last rebase
>> transaction, so that after the transaction the filteredrevs is 
>> appropriately
>> invalidated and will be up-to-date on the next read.
>>
>> This showed up in an extension that combines rebase, obsolete, and 
>> inhibit, so
>> I'm not sure how to create a test case in hg core to repro it. But we 
>> have a
>> test case in the extension repo.
>>
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -495,6 +495,12 @@ class rebaseruntime(object):
>>                  if self.activebookmark not in repo._bookmarks:
>>                      # active bookmark was divergent one and has been 
>> deleted
>>                      self.activebookmark = None
>> +            # Clear the _rebaseset as part of the transaction, so
>> +            # post-transaction hooks can see the final repo 
>> visibility state.
>> +            # (We could've moved clearstatus() into the transaction 
>> to do this,
>> +            # but it contains non-transactional changes, like 
>> unlinking files).
>> +            _clearrebasesetvisibiliy(repo)
>> +
>>          clearstatus(repo)
>>          clearcollapsemsg(repo)
>
> The clearing of the "rebased" revision happens a couple of line higher 
> (line 489). Would it make more sense to clear the visible set at the 
> same time instead of the proposed spot ?
I looked into putting it up there, but didn't for a few reasons:

- I wanted it to happen after the final transaction, since bookmarks can 
also affect visibility.  In the inhibit case, if we clear the rebaseset 
before the bookmark change, then the transaction before the bookmarks 
causes the old nodes to be inhibited since bookmarks exist on them. 
Which means they don't get hidden appropriately after the bookmark 
transaction.  This kinda argues that the whole rebase should be in a 
single transaction, but that's a riskier change to make.
- The clearrebased line is only run if there's no --keep, and we need to 
do this _rebaseset stuff in all cases.
- I tried wrapping clearrebased in the same transaction as the bookmark 
stuff, so all of this happened together, but clearrebased can't be in a 
transaction because it performs a strip if obsolete is not enabled.

This whole area feels like it could use a transaction oriented refactor, 
but that'll have to wait for another day.
Pierre-Yves David - Nov. 10, 2016, 3:06 p.m.
On 11/10/2016 02:42 PM, Durham Goode wrote:
>
>
> On 11/10/16 1:17 PM, Pierre-Yves David wrote:
>>
>>
>> On 11/10/2016 11:56 AM, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1478778918 28800
>>> #      Thu Nov 10 03:55:18 2016 -0800
>>> # Node ID f3bfa374cea66cd5bf17645ccb6eef60c4d8a5aa
>>> # Parent  3fd53cc1aad882ac9191d7388885acdbbc2d7103
>>> rebase: update _rebaseset inside transaction
>>>
>>> Rebase has the concept of the _rebaseset, which is used to prevent
>>> the commits
>>> being rebased from being hidden until after the rebase is complete.
>>> Previously,
>>> the _rebaseset was being cleared at the very end of rebase, after the
>>> last
>>> transaction had already closed. This meant that the repo filteredrevs
>>> cache was
>>> not updated (since no invalidation had been triggered, since we're
>>> outside of a
>>> transaction), which meant future changelog reads saw commits as
>>> visible that
>>> should've been hidden.
>>>
>>> This patch moves the _rebaseset clearing to be inside the last rebase
>>> transaction, so that after the transaction the filteredrevs is
>>> appropriately
>>> invalidated and will be up-to-date on the next read.
>>>
>>> This showed up in an extension that combines rebase, obsolete, and
>>> inhibit, so
>>> I'm not sure how to create a test case in hg core to repro it. But we
>>> have a
>>> test case in the extension repo.
>>>
>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -495,6 +495,12 @@ class rebaseruntime(object):
>>>                  if self.activebookmark not in repo._bookmarks:
>>>                      # active bookmark was divergent one and has been
>>> deleted
>>>                      self.activebookmark = None
>>> +            # Clear the _rebaseset as part of the transaction, so
>>> +            # post-transaction hooks can see the final repo
>>> visibility state.
>>> +            # (We could've moved clearstatus() into the transaction
>>> to do this,
>>> +            # but it contains non-transactional changes, like
>>> unlinking files).
>>> +            _clearrebasesetvisibiliy(repo)
>>> +
>>>          clearstatus(repo)
>>>          clearcollapsemsg(repo)
>>
>> The clearing of the "rebased" revision happens a couple of line higher
>> (line 489). Would it make more sense to clear the visible set at the
>> same time instead of the proposed spot ?
> I looked into putting it up there, but didn't for a few reasons:
>
> - I wanted it to happen after the final transaction, since bookmarks can
> also affect visibility.  In the inhibit case, if we clear the rebaseset
> before the bookmark change, then the transaction before the bookmarks
> causes the old nodes to be inhibited since bookmarks exist on them.
> Which means they don't get hidden appropriately after the bookmark
> transaction.  This kinda argues that the whole rebase should be in a
> single transaction, but that's a riskier change to make.

It looks like it would be more correct to move the bookmark before 
clearing the rebase set anyway. Can you fix that and then group the 
clearing ?

> - The clearrebased line is only run if there's no --keep, and we need to
> do this _rebaseset stuff in all cases.

You new code does not need to be in the conditional, just next to it.

Cheers,

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -495,6 +495,12 @@  class rebaseruntime(object):
                 if self.activebookmark not in repo._bookmarks:
                     # active bookmark was divergent one and has been deleted
                     self.activebookmark = None
+            # Clear the _rebaseset as part of the transaction, so
+            # post-transaction hooks can see the final repo visibility state.
+            # (We could've moved clearstatus() into the transaction to do this,
+            # but it contains non-transactional changes, like unlinking files).
+            _clearrebasesetvisibiliy(repo)
+
         clearstatus(repo)
         clearcollapsemsg(repo)