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

login
register
mail settings
Submitter Pierre-Yves David
Date May 2, 2017, 11:43 p.m.
Message ID <6697da7c4eab3fbe3588.1493768618@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20366/
State Accepted
Headers show

Comments

Pierre-Yves David - May 2, 2017, 11:43 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@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.
Yuya Nishihara - May 3, 2017, 7:51 a.m.
On Wed, 03 May 2017 01:43:38 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@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.

Other than that, the change looks good to me.
Jun Wu - May 3, 2017, 8:20 a.m.
Other than the O(1) revs question, the series is a clear improvement to me.

Excerpts from Pierre-Yves David's message of 2017-05-03 01:43:38 +0200:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@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 = {}
> +
>          # a dict of arguments to be passed to hooks
>          self.hookargs = {}
>          self.file = opener.open(self.journal, "w")
Pierre-Yves David - May 3, 2017, 1:06 p.m.
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@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.

Cheers,
Yuya Nishihara - May 3, 2017, 1:46 p.m.
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@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.
Pierre-Yves David - May 3, 2017, 7:09 p.m.
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@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 - May 11, 2017, 12:49 p.m.
So, what is the status of this series?
I've important speedup work stuck behind it so I would like to see the 
topic move forward.

On 05/03/2017 09:09 PM, Pierre-Yves David wrote:
>
>
> 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@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,
>

Patch

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 = {}
+
         # a dict of arguments to be passed to hooks
         self.hookargs = {}
         self.file = opener.open(self.journal, "w")