Re: [Monotone-devel] update multiple projects

[Thomas Keller]

Am 31.03.10 13:10, schrieb 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.

Whatever channel it actually uses, stdio users do not expect that a
command prompts for input and with remote stdio its even more unclear
how this should be handled at all.

>> 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?

I don't understand the question. Currently, whether you do a normal or a
workspace merge, as soon as a content conflict arises, mtn opens a
merger process (might it be vim or kdiff3 or emacs or opendiff) and
waits until this process ends, i.e. returns, to pick up the saved
temporary file afterwards.
The problem is now that the shell call which opens the merger process
has to _block_ until the process actually ends, if it spawns in the
background and returns immediately, mtn cannot pick up the temp file and
therefor not the manual merge result. Thats why it waits for a key
stroke from the user to act upon further. And this is actually the
default behaviour for the default merger "opendiff" on Mac OS X...

>> [--non-interactive] 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.

This sounds more like a workaround for a problem similar to one which
you already solved quite smartly. Wouldn't be a better solution to
expand show_conflicts to work in workspace mode and give mtn automate
update an option --resolve-conflicts-file to resolve these on the fly?
If this would be in place, merge_into_workspace could profit from that
as well...

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

No, I'm not proposing anything. I just didn't like the idea to tell the
implementor to set MTN_MERGE="foo" just to make the internal merger call
fail... ;)

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

Heh, well, the code which resets the options before every command
execution is hairy and its not better that its used in similar (but not
quite equal) versions in three places: the mtn_automate lua hook,
CMD(stdio) and CMD(remote_stdio). When I hacked on the remote stuff the
last time I tried to unify this a bit, but this was not easily possible.
In the end I still felt not comfortable enough to make major changes in
this area without opening a big can of worms.

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

Btw, shouldn't o1:r40:...1:r40:...e l6:updatee work?

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

No, of course I don't want that. Out-of-band output (despite tickers) is
not standardized in any way and we haven't yet (and probably also won't
ever with this setup) make a contract saying that a stdio consumer could
rely on the syntax / format of progress messages, warnings or errors.
I'd say its more than enough if the test guarantees that the processing
of the individual update options is as documented. But if you're not up
for the task because you say "its already tested for the normal
commands" then I just have to remember that the next time I review some
code which touches this area.

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

If both share 98% of the code, why do you think CMD_AUTOMATE(update)'s
error handling is any better than the other?

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

Maybe we have something similar already which could be tweaked for that
- CMD's can take a list of aliases as third parameter. If we somehow
could expand the aliases to "links" inside the command tree structure,
we would be able to simply configure a command to be part of another
command tree / group.

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

No, I think this is a very good solution. One might tend to mix up the
lua context in which automate commands and other lua hooks are executed
and the one in which the test is executed, but actually they're
completely different (if only for the sake because the former is
executed while one and the same monotone instance is running). A
specific test implementation like this models a real-world usage of the
new function and is therefor a very valid test.

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