Re: [Monotone-devel] update multiple projects

[Thomas Keller]

Am 31.03.10 00:12, schrieb Stephen Leake:
>>> 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.

The prompt is the problem, especially on stdio. Some platforms, f.e. Mac
OS X, even require an explicit key stroke because the standard merger
call doesn't block itself.

> 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. 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. If mtn au update is called however, it
shouldn't behave any different than mtn update.
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.

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

Yes thanks, this is more or less a hack to properly encode out-of-band
chatter in the stdio protocol, given the fact that we would have a very
very hard time to properly implement logging / error tracking in mtn
otherwise.

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

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.

>> 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) - for all
these use cases it would just be great if a good test case would tell us
"hey, the new implementation no longer spits out what the interface
implementor expects in this interface version, adapt the test and bump
interface_version!".

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

Yes, right, if you ever need such a thing, check if the FL() macro suits
your needs.

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

> Thanks for your review,

Thanks for your work!

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