Re: [Monotone-devel] mtn_automate usage

[Thomas Keller]

Am 15.07.10 01:31, schrieb Timothy Brownawell:
> On 07/14/2010 04:46 PM, Thomas Keller wrote:
>>
>> While I worked on nvm.validate-changes I had the idea to use
>> mtn_automate() in my pre-commit test hook to only scan the diffed source
>> part for CRLFs (see the other mail), but this usage is currently
>> prevented as we only allow mtn_automate() being called from
>> register_command().
>>
>> Now this code was implemented about three years ago from William Uther
>> (I don't know if he is still around), so I wonder why he decided to
>> restrict this in that way. One sure thing is that we of course want to
>> prevent endless recursion, i.e. calling an automate command which calls
>> a hook which calls the automate command again, but I think this could be
>> prevented successfully, while still allowing mtn_automate for other use
>> cases.
> 
> How bad would this actually be? Say you have your netsync notification
> hooks set to call 'heads' or some of the graph commands to print a
> pretty report of what was synced, and you also want a custom command
> that will commit and then sync...
> 
> Maybe instead of a strict "no recursion" we want to prevent recursion
> past some (arbitrary, large) depth like 100 or so, or keep a stack of
> all calls with options and arguments to check for cycles.

What I remember from my early computer science courses is that proper
recursion checking is a science of its own. I haven't implemented
something like this before and I'm not aware of the caveats which could
come up with such an implementation. But maybe I'm wrong and all this is
easy.

>> Is there anything I haven't thought of? Is option handling a problem?
>> Probably, since f.e. workspace options could be written earlier than
>> expected, e.g. execute a workspace command with a new branch name, call
>> a automate command which also needs the workspace and executes
>> successfully, options are written back to _MTN/options, the normal
>> control flow of the calling workspace command fails, user expects
>> _MTN/options to be untouched...
>>
>> Anything else?
> 
> I think it was just about reentrancy in general. Our globals are
>   * database (database_impl is cached and global, and of course the
>     file is global)
>   * lua
>   * options (doesn't need to be global; make it or lua be a pointer
>     instead of being incorporated into app_state directly)
>   * keystore (but the object itself isn't global)
>   * workspace
>   * the initial working directory
> 
> So issues would be:
>   * if a hook gets called while inside a database transaction (in
>     particular, receiving data over netsync can lead to large
>     global transactions... actually 'remote'/'remote_stdio' have
>     this issue already, need to check that transaction_guard is
>     sane when given nested transactions and an inner one is
>     rolled back. If it isn't, getting an automate_command_cmd needs to
>     trigger any existing transaction to be committed before processing
>     starts.)

Ouch, right. But I'm not sure if automate commands should trigger
commits or checkpoints by this. I know that the automation interface is
stateless in database terms, but maybe it would be a good idea to
introduce transaction handling at least over stdio? If no transaction is
started then, we could work in "autocommit" mode from stdio terms, i.e.
any pending transactions are committed before a command executes.

>   * things that affect the keystore... if a command called from a
>     hooks adds or removes keys, that won't show up in the parent
>     command (if the keystore has been lazy-initialized before then).

Maybe the keystore should get some functionality to invalidate itself
(i.e. its state) when new things arrive or are removed? I mean, we can't
do anything if the lua hook spawns a new process for, say, mtn dropkey,
but if the hook happens to do it in-process, we could probably just set
the have_read flag in the key store state to false, no?

>   * things that affect the workspace, including writing out options.
>     everything other than writing out options tends to be user-explicit,
>     so we'd just need a check when writing options that we're the
>     outermost workspace-aware command. Possibly do this with something
>     similar to transaction_guard.

Yes, this is a very good idea. A nested, dedicated options writer.

>   * no-workspace commands would typically always see filename arguments
>     exactly as given. If called under a workspace-aware command, I think
>     they'd see them adjusted for initial working dir vs. workspace root.

This is another reason why we should get rid off the global workspace
path finding and fiddling code, this bites us again here.

Wow, much work ahead - thanks for this very nice compilation! I try to
keep that on my radar - feel free to add a couple of specific RoadMap
items for things on this list.

Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | http://thomaskeller.biz
Please note that according to the EU law on data retention, information
on every electronic information exchange might be retained for a period
of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en

signature.asc
Description: OpenPGP digital signature