Re: [Monotone-devel] update multiple projects

[Stephen Leake]

Thomas Keller <address@hidden> writes:

>>> 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.
>
> The prompt is the problem, especially on stdio.

I guess the spawned process might use the same stdio channel to
prompt.

> Some platforms, f.e. Mac OS X, even require an explicit key stroke
> because the standard merger call doesn't block itself.

Do you mean this is required even with the standard hooks?

The test I did above was on Windows.

Hm. There was something about "merger might prompt" in the original
Emacs DVC code. I never did understand the use case for that, so it's
gone now.

>> 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).
>
> --non-interactive is a new option of 0.46 which was introduced exactly
> for this and other use cases. The original rationale was that key
> decryption should not lead to a passphrase prompt in case all other
> methods (phraseless try, ssh-agent and get_passphrase lua hook) did not
> succeeed.

Right. And I just ran into exactly that problem; someone had their
get_passphrase hook written incorrectly, and the Emacs interface just
froze, with no error message. --non-interactive on commit is the
solution.

> This option is set to true in CMD(stdio) and CMD(remote_stdio), so
> it would just be great if the new update command would honor this
> option as well.

Specifically, do you mean "fail if manual merge is required and
--non-interactive is set"?

I'd rather not, actually. I can now use 'automate update' from my
Emacs front end, via the automate stdio session I maintain for each
workspace. In that case, I want the current behavior of popping up a
separate Emacs for a workspace conflict.

I think we need to make the value of --non-interactive available in
Lua hooks, so the user overriding a hook can check it before doing
something that might prompt.

> If mtn au update is called however, it shouldn't behave any
> different than mtn update. 

Right.

> And finally, I don't think messing with environment variables is a
> good idea here at all... normally stdio instances are ran within
> "clean" environments and it would be cool if we could just keep that
> as is.

Messing with environments is only necessary if you want non-standard
behavior; I think that would be acceptable.

I'm not clear exactly what you are proposing.

>>> 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.
>
> My main fear is that this introduces some kind of "stealth" workspace /
> database switch within a running stdio instance - i.e. the database
> which is found initially is opened very early before stdio accepts the
> first command (see db.ensure_open() in cmd_automate.cc) - and I have the
> fuzzy feeling that the other assumptions and hackery we do f.e. to reset
> global options for every single automate command execution, might play
> not nice with that. I haven't tested this thoroughly, I'm just not
> comfortable with that.

It's actually better than what I said above. 

The workspace options are read (by calling workspace::get_options) in
three places: cmd.cc commands::process, cmd_automate.cc
LUAEXT(mtn_automate), and CMD_AUTOMATE_NO_STDIO(stdio). So any command
run via mtn_automate will be ok after change_workspace. 

The other Lua extensions don't refresh the workspace options. So
get_confdir is the only Lua extension that could be wrong after
change_workspace. 

But I guess you are worried about someone executing change_workspace
in some Lua hook, and then returning to the stdio process without
calling mtn_automate. But that is safe, because each command in
automate_stdio refreshes the workspace options. Someone was prescient
when they wrote automate stdio (I think that was you :).

I've updated the manual to say this.

> I understand your use case though and I tend to say you're right - we
> should maybe start thinking about fixing things when these actually
> come up.

Ok. 

>>> 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.
>
> I know this may sound a bit backwards for now, but imagine the
> implementation of update and automate update diverge in the future 
> or new special features are added to the automate version which do
> not exist in the non-automate version or the other way around (which
> might even be more problematic, because an apparently "harmless"
> change to mtn update could mean incompatible changes to the stdio
> version) - 

The code is literally shared at the moment; 'mtn update' and 'mtn
automate update' do the same error checks with different responses,
then call cmd_merger.cc update to do _everything_ else.

So the scenarios you are positing require someone to break that
sharing; clearly it is up to them to add the appropriate tests.

By this argument, I should rerun _all_ of the update tests using
'automate update'. I don't think that is reasonable. I suppose it
might be possible to parameterize the current tests, so they run
either 'mtn update' or 'mtn automate update', but even that's a lot of
work (and I suspect it's not possible).

As you say above, 'update' and 'automate update' should have the
_same_ behavior. This should be a general requirement for all automate
commands that are also non-automate commands. That will ease user
understanding as well as code and test maintenance.

Hmm. That doesn't apply to commands that produce useful output, like
inventory (and all of the other current automate commands :); the
automate output is in basic_io format.

Actually, I would argue that the error handling in
CMD_AUTOMATE(update) is better than in CMD(update), and we should
change CMD(update) to match. Then we could introduce a new macro
CMD_CMD_AUTOMATE that declares both at once, to ensure exactly the
same behavior, for commands that don't produce usable output. 

There are several other mtn commands that would be nice to run via au
stdio; commit and merge are at the top of my list. We'll have the same
testing/maintenance issue with them, so we should get this right now.

>> 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.
>
> Which is a pity, actually and shouldn't hold you off creating one for
> change_workspace :)

Ok, I added a test. Surprisingly difficult; the mtn lua extensions are
not directly available in the __driver__.lua script. It took me a
while to figure out how to call change_workspace. Maybe you can
suggest a better alternative.

Or maybe we need a new tester variant that makes the Lua extensions
directly available.

--
-- Stephe