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

Pierre-Yves David pierre-yves.david at ens-lyon.org
Fri Dec 2 22:39:10 EST 2016



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

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.

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list