Re: [Monotone-devel] update multiple projects

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Am 28.03.10 18:17, schrieb Stephen Leake:
>> Stephen Leake <address@hidden> writes:
>> 
>>>     function gds_update_all()
>>>        local workspaces = gds_find_workspaces (gds_missions)
>>>
>>>        for _, ws in pairs(workspaces) do
>>>            -- cd("../" .. ws)
>>>            -- mtn ("update")
>>>            mtn_automate ("update", "--workspace=" .. ws)
>>>            io.stderr:write(ws .. "\n")
>>>        end
>>>     end
>> 
>> I've started working on this. 'mtn automate update' is implemented;
>> see cmd_merging.cc in 2cc8eafc9f3350379e72c47c24e567ffc52d7fbc in
>> branch nvm.stephe. There's a test for it. It was easy, since there is
>> no output to format (it's progress, not useful for parsing).
>> 
>> I added a new Lua function 'change_workspace' (cmd_automate.cc in
>> f0b5f85a97853d52f94fd3aab2606ba093a40bff), which basically calls
>> paths.cc go_to_workspace.
>> [...] 
>> Any comments on 'automate update' or 'change_workspace'?
>
> Looks good in general, a few open points though:
>
> 1) How does automate update behaves (especially over stdio) when a
> workspace merge has to take place? I don't see any special code handling
> that in a sane way.

I just tried 'mtn automate update' and 'mtn automate stdio'
'l6:updatee'; it's the same as the current update behavior (pops up a
separate Emacs session running ediff3). That doesn't prompt the user
via the stdio channels; it just spawns a separate process, which can
prompt or otherwise interact with the user. So I don't see a problem.

If the user customizes the merge hooks, it might prompt via stdio. But
that's true in general; users can mess things up if they customize
hooks.

Spawning a merger over remote_stdio would be a problem; the merger
would be running on the remote machine. But 'remote update' would have
to be enabled on the server in the first place; that's not likely.

The user can set MTN_MERGE to "fail" if they really don't want a
merger to run (we could mention that in the manual for 'automate
update').

Perhaps automate update should interpret --non-interactive to mean
"don't spawn a merger (fail if a merger is needed)". That would be
simpler than setting MTN_MERGE. Actually, that makes sense for
non-automate update, anyway (I just checked; update does not currently
respect --non-interactive in this sense).

By the way, it is your recent enhancements to automate stdio (the 'p'
channel) that made implementing 'automate update' so easy; thanks
again for that!

> 2) From what I see change_workspace() might potentially open a can of
> worms / bugs, especially on long-running stdio instances where an
> existing database connection is kept open. One could f.e. think a call
> like `automate lua change_workspace /path/to/new/workspace` is harmless,
> while everything might explode (tm) afterwards because the workspace
> points to an entirely different database...

change_workspace calls the same function that opens a workspace in the
first place. After that, any code that wants "the database associated
with the current workspace" should parse the workspace options, which
sets the database cache properly. I guess that is probably not true
now for most Lua extension functions now; there's a huge assumption
that the workspace doesn't change. But every top-level function (like
'update' or 'inventory') does that.

I've added this in the manual;

"After calling change_workspace, the next operation should read the
workspace options. That means it should be a top level command like
'update'".

Of course, the only "top level command" available from Lua at the
moment is 'update' via mtn_automate.

Perhaps change_workspace should itself parse the workspace options,
but that seems overkill.

My current Emacs front-end uses one automate stdio session per
workspace. I don't intend to use change_workspace there. But that
doesn't mean someone won't.

I agree this could be a problem, but I think the functionality
change_workspace gives now is worth it; we can add checks and more
robust switching as problems arise.

Let me emphasize my use case; 

I maintain several branches in parallel; it's a hierarchy of
libraries. 

Therefore I need a scripting language to run mtn commands in each
workspace.

Since mtn provides the Lua scripting language, it seems the logical
choice.

I have some bash scripts, but I much prefer Lua! And in some
situations, bash doesn't work. In particular, I have some workspace
accessed via an NFS mount from a Windows machine; Cygwin bash does not
see those mounts. Any other scripting language provided by Cygwin will
have the same problem. Using non-Cygwin scripting languages is just
too painful; no installer, more likely to be non-portable with a Unix
system, etc.

This particular problem would go away if there was a Cygwin NFS
client. But I still prefer Lua over bash, and it will be more portable
to other mtn users.

> 3) It would be cool if you could test the options for automate update a
> bit more 

Since the normal option processing is the same between 'automate
update' and 'update', I only need to test the error cases (see
CMD_AUTOMATE(update...) in cmd_merging.cc). I tried to test
revision_selectors.size() > 1, but couldn't build a stdio command
string that worked. But I don't have to go thru stdio to test that;
I've added a test for that.

> and harden the test when it looks for the right progress message
> (progress[9] might break at any time in the future when somebody
> decides to add another test file, while it possibly should not do
> so).

Right; it now fetches the last line, rather than the 9th.

> 4) What do you mean with
>
> +      // need a variant of P that doesn't require F?
> +      P(F(f.what()));

I was thinking this was not an appropriate idiom; F calls gettext, but
in this case the argument is not a fixed string, so the translators
can't do anything with it.

I've changed it to use lua_pushlstring to return the error message,
rather than dumping it as a progress message, and now it doesn't use
F.

I guess we can assume that the message in the exception is either
formated by the operating system using the current locale, or by some
monotone C++ code that uses F. So it should be properly localized.

I could add tests for change_workspace in tests/. However, that
doesn't seem to be the convention for Lua extension functions; I don't
see explicit tests of mtn_automate, for example.

> 5) Minor nitpicks:
>
> +  E(args.empty(), origin::user,
> +    F("update takes no command arguments"));
>
> We usually use one single message - "wrong argument count" - for this.

Ok; fixed.

> +  update (app, args);
>
> There is an additional unneeded space after update.

Yes; fixed. That's a conflict between my work Ada style and monotone
C++ style. Unfortunately, Emacs doesn't help me here. The Ada compiler
does check this stuff; I wish the C++ compiler would.


Thanks for your review,

-- 
-- Stephe