Re: [Monotone-devel] automate show_conflict

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Stephen Leake schrieb:
>
>> Now when I execute this in a workspace, au stdio does _not_ find the
>> workspace options, but au show_conflicts does:
>> $ cd $BUILD_DIR/tester_dir/tests/automate_show_conflicts_defaults
>> $ echo l14:show_conflictse | $BUILD_DIR/mtn au stdio
>> 0:2:l:53:misuse: please specify a branch, with --branch=BRANCH
>> $ $BUILD_DIR/mtn au show_conflicts
>>     left [19ab79c40805c9dc5e25d0b6fa5134291e0b42d9]
>>    right [d8a8bc9623c1ff9c0a5c082e40f0ff8ec6b43e72]
>> ancestor [8a211b54bf183c6aa9106fb7da30f22ffc5b821a]
>> ...
>> 'au stdio' resets the options for each command, and does _not_
>> read them from the workspace for each command. See cmd_automate.cc
>> near line 370. So when the current command is called at line 403,
>> 'app.opts' has only the options specified for the current command.
>> Which makes sense.
>
> Its also required for a long-living process like stdio that it
> re-reads stuff for every command, because the outside world could have
> changed completly in the meantime where it was just sitting around
> doing nothing.

Ah, that's a good point. One of the automate stdio commands might
change the branch of the workspace, for example.

> At first I thought it would be simple to require a workspace for a
> command run inside stdio by just calling
>
> workspace::require_workspace(F("no workspace found"));
>
> because my simple mind thought it already reads options in there, but
> then I found out that the only automate command which uses this
> currently (automate heads) actually fails miserably if run over stdio:
>
>
> $ echo l5:headse | mtn au stdio
> 0:0:l:0:              $ mtn au heads
> 8fb3fdc800830e6ba1bf3f99cc6a28c3fa98cc51
>
>
> And its clear why it fails - require_workspace just checks a variable
> which is set during startup in monotone.cc:224 if the workspace is
> found at all - the actual workspace option reading then happens only
> once in commands.cc:771:
>
>     if (cmd->use_workspace_options())
>       {
>         workspace::check_ws_format();
>         workspace::get_ws_options(app.opts);
>       }
>
> This code path is not executed again for any automate command run over
> stdio, just once for the call to automate stdio itself, as you noticed
> as well. The longer I look at this "mixing workspace options into
> application options stuff" the more I want to see that go away.

I agree it can be confusing. But I think we need to keep the automate
commands as close as possible to the non-automate commands, so people
can try things out with the latter and then expect it to work with
the former.

So since the non-automate commands read ws options, the automate
commands should as well.

>> We can have 'automate show_conflicts' read the ws options if it needs
>> them. Or do as you suggest, and simply require --branch instead.
>
> If we do the first, it would fix automate heads as well. I'd then
> second to extend the CMD_AUTOMATE macro then to be able to set
> use_workspace_options explicitely and do the same like what has been
> done in commands.cc:771 in cmd_automate.cc:402, just before the call
> to exec_from_automate. 

I think this is the right approach; it will be most consistent between
automate and non-automate commands. 

After sleeping on this, I was going to suggest that we have the code
in cmd_automate.cc copy the ws options into the new opts object. But
your point about needing to re-read them makes sense.

I also think we should do this on this branch. Since we're messing
with an important macro, we want to get more testing before pushing to
mainline.

And clearly we need to add tests for heads | au stdio and
show_conflicts | au stdio.

-- 
-- Stephe