Re: [Monotone-devel] mtn_automate usage

[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.

> 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.)
  * effects on Lua don't matter, since they're presumably intentional
  * options being changed... these are restored after each call,
    so it should be fine. --key is stored in the key_store object,
    but that object is command-local so that's also fine.
  * 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).
  * 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.
  * 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.

--
Timothy

Free public monotone hosting: http://mtn-host.prjek.net