[PATCH V2] transaction: allow running file generators after finalizers

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sun Mar 27 01:15:50 EDT 2016



On 03/20/2016 10:49 AM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1458495989 25200
> #      Sun Mar 20 10:46:29 2016 -0700
> # Node ID 1431f0447a6b0be851f609b386fabf83b2f54666
> # Parent  ed75909c4c670a7d9db4a2bef9817a0d5f0b4d9c
> transaction: allow running file generators after finalizers

The general logic on this patch is good. But the actual changes to file 
generation function seems a bit too fragile to me. See my comment in 
context.


> […]
> @@ -276,12 +284,17 @@ class transaction(object):
>           # but for bookmarks that are handled outside this mechanism.
>           self._filegenerators[genid] = (order, filenames, genfunc, location)
>
> -    def _generatefiles(self, suffix=''):
> +    def _generatefiles(self, postfinalize=False, suffix=''):

The postfinalize=False as the default is an issue here,

Before this patch tr._generatefiles() would get all file generated. 
After that patch, only part of them are. Third parties and future 
callers are going to get this wrong and introduce various transaction 
bug. This is highlighted by the fact they had to be a special case when 
'suffix' is passed.

I think we need three values here: 'all', 'precl', 'postcl':

   def _generatefiles(self, suffix='', group='all')

This is a small change but I think this is a critical enough code that 
it deserved to be expurged from surprise.


Also, we should try to avoid changing the position of arguments, this 
gratuitously break other callers.

>           # write files registered for generation
>           any = False
> -        for entry in sorted(self._filegenerators.values()):
> +        for id, entry in sorted(self._filegenerators.iteritems()):
>               any = True
>               order, filenames, genfunc, location = entry
> +
> +            # for generation at closing, check if it's before or after finalize
> +            if not suffix and (id in postfinalizegenerators) != (postfinalize):
> +                continue
> +

[above, the suffix "hack" I was referring too]

> […]

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list