[PATCH 1 of 8] transaction: introduce "changes" dictionary to precisely track updates

[Pierre-Yves David]

On 05/03/2017 03:46 PM, Yuya Nishihara wrote:
> On Wed, 3 May 2017 15:06:05 +0200, Pierre-Yves David wrote:
>> On 05/03/2017 09:51 AM, Yuya Nishihara wrote:
>>> On Wed, 03 May 2017 01:43:38 +0200, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>>>> # Date 1493742678 -7200
>>>> #      Tue May 02 18:31:18 2017 +0200
>>>> # Branch stable
>>>> # Node ID 6697da7c4eab3fbe3588a2f91fa3f99b16f808ac
>>>> # Parent  fbb5f4bf94928b98fa87871e84bb2ef972ec2d51
>>>> # EXP-Topic obscache
>>>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 6697da7c4eab
>>>> transaction: introduce "changes" dictionary to precisely track updates
>>>>
>>>> The transaction is already tracking some data intended for hooks (in
>>>> 'hookargs'). However, that information is minimal as we optimise for
>>>> passing data to other processes through environment variables. There are
>>>> multiple places were we could use more complete and lower level
>>>> information locally (eg: cache update, better report of changes to
>>>> hooks, etc...).
>>>>
>>>> For this purpose we introduces a 'changes' dictionary on the
>>>> transaction.  It is intended to track every changes happening to the
>>>> repository (eg: new revs, bookmarks move, phases move, obs-markers,
>>>> etc).
>>>>
>>>> For now we just adds the 'changes' dictionary. We'll adds more tracking
>>>> and usages over time.
>>>>
>>>> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
>>>> --- a/mercurial/transaction.py
>>>> +++ b/mercurial/transaction.py
>>>> @@ -137,6 +137,10 @@ class transaction(object):
>>>>              releasefn = lambda tr, success: None
>>>>          self.releasefn = releasefn
>>>>
>>>> +        # A dict dedicated to precisely tracking the changes introduced in the
>>>> +        # transaction.
>>>> +        self.changes = {}
>>>
>>> I'm not sure if it's good idea to add more free-form dict to the transaction
>>> class, since that would make code less manageable in general.
>>
>> Can you elaborate on your worries here?
>>
>> The content of the dictionnary should be strictly defined by the
>> localrepo object, and the data should be filled at low level by code
>> that already requires a transaction to be present. So I think the result
>> will be quite manageable.
>>
>> We -need- something else than "hookargs" to carry transaction related
>> information. For example we needs to tracks bookmark movement, phases
>> changes and tags movement. The "hookargs" dictionnary is not appropriate
>> to track such data.
>
> My concern is unclear, sorry. The transaction code seems hard to review because
> it has arbitrary hooks, weakrefs in them, hookargs dict, etc. And now 'changes'
> dict. They all seem to make things too abstract.
>
> I believe the result looks better overall than before, but I can't say the
> transaction itself gets better.

The net result at the end of my work should be clearer:

- code tracking changes should be clearer and safer,
- code reacting to changes would have more well defined spot to react to 
these changes.

(as a bonus, we might be able to compute the hookargs, dict for the 
'changes' one at some point)

Cheers,

-- 
Pierre-Yves David