[PATCH 2 of 2 V2] rebase: update _rebaseset inside transaction

[Durham Goode]

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