Patchwork [2,of,2,V2] rebase: update _rebaseset inside transaction

login
register
mail settings
Submitter Durham Goode
Date Nov. 10, 2016, 5:27 p.m.
Message ID <839aa67de1a6436dfc1a.1478798875@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/17450/
State Changes Requested
Headers show

Comments

Durham Goode - Nov. 10, 2016, 5:27 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1478798531 28800
#      Thu Nov 10 09:22:11 2016 -0800
# Node ID 839aa67de1a6436dfc1ae29265d0bfb0c1ce3cb7
# Parent  03bc2afdfa26ad5ab057a28ac037524407ca5a92
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.
Pierre-Yves David - Nov. 19, 2016, 10:25 a.m.
On 11/10/2016 06:27 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1478798531 28800
> #      Thu Nov 10 09:22:11 2016 -0800
> # Node ID 839aa67de1a6436dfc1ae29265d0bfb0c1ce3cb7
> # Parent  03bc2afdfa26ad5ab057a28ac037524407ca5a92
> rebase: update _rebaseset inside transaction

I've taken patch 1, I'm not sure about the approach of patch 2, see below.

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

It looks like the main issue here is that the filteredrevs is not 
invalidated. The '_clearrebasesetvisibiliy' function should just clear 
call that cache invalidation and that would probably be enough. Using a 
new dedicated transaction where nothing happen for cache invalidation 
purpose seems a bit odd.
Durham Goode - Nov. 28, 2016, 5:46 p.m.
On 11/19/16 2:25 AM, Pierre-Yves David wrote:
> On 11/10/2016 06:27 PM, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1478798531 28800
>> #      Thu Nov 10 09:22:11 2016 -0800
>> # Node ID 839aa67de1a6436dfc1ae29265d0bfb0c1ce3cb7
>> # Parent  03bc2afdfa26ad5ab057a28ac037524407ca5a92
>> rebase: update _rebaseset inside transaction
>
> I've taken patch 1, I'm not sure about the approach of patch 2, see 
> below.
>
>> 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.
>
> It looks like the main issue here is that the filteredrevs is not 
> invalidated. The '_clearrebasesetvisibiliy' function should just clear 
> call that cache invalidation and that would probably be enough. Using 
> a new dedicated transaction where nothing happen for cache 
> invalidation purpose seems a bit odd.
>
I feel like business logic code, like rebase, should have as little 
awareness of cache lifetimes as possible.  Hard coding the clearing of 
the filteredrevs would mean that anything else that depends on 
_rebaseset would not be invalidated unless it was also touched in 
_clearrebasesetvisibility.  It just seems like tangling the consumer and 
provider of this data.

But it's not a big enough issue for me to push on.  I'll resend this 
patch with it just clearing the cache directly.
Pierre-Yves David - Dec. 3, 2016, 3:39 a.m.
On 11/28/2016 06:46 PM, Durham Goode wrote:
> On 11/19/16 2:25 AM, Pierre-Yves David wrote:
>> On 11/10/2016 06:27 PM, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1478798531 28800
>>> #      Thu Nov 10 09:22:11 2016 -0800
>>> # Node ID 839aa67de1a6436dfc1ae29265d0bfb0c1ce3cb7
>>> # Parent  03bc2afdfa26ad5ab057a28ac037524407ca5a92
>>> rebase: update _rebaseset inside transaction
>>
>> I've taken patch 1, I'm not sure about the approach of patch 2, see
>> below.
>>
>>> 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.
>>
>> It looks like the main issue here is that the filteredrevs is not
>> invalidated. The '_clearrebasesetvisibiliy' function should just clear
>> call that cache invalidation and that would probably be enough. Using
>> a new dedicated transaction where nothing happen for cache
>> invalidation purpose seems a bit odd.
>>
> I feel like business logic code, like rebase, should have as little
> awareness of cache lifetimes as possible.  Hard coding the clearing of
> the filteredrevs would mean that anything else that depends on
> _rebaseset would not be invalidated unless it was also touched in
> _clearrebasesetvisibility.  It just seems like tangling the consumer and
> provider of this data.

The purpose of the _rebaseset is to alter the visibility of the node. 
So, it seems okay it has to use multiple part of this visibility API.

> But it's not a big enough issue for me to push on.  I'll resend this
> patch with it just clearing the cache directly.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -496,7 +496,12 @@  class rebaseruntime(object):
                 collapsedas = newnode
             clearrebased(ui, repo, self.state, self.skipped, collapsedas)
 
-        clearstatus(repo)
+        # Put clearstatus in a transaction since it affects commit visibility
+        # and extensions depend on transaction close hooks for watching
+        # visibility updates.
+        with repo.transaction('rebasestatus') as tr:
+            clearstatus(repo)
+
         clearcollapsemsg(repo)
 
         ui.note(_("rebase completed\n"))