[PATCH] rebase: update _rebaseset inside transaction

[Pierre-Yves David]

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

-- 
Pierre-Yves David