Re: [Monotone-devel] automate show_conflict

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Stephen Leake schrieb:
>>> In the end I think we should do two things: Disable
>>> use_workspace_options for automate commands (since they're "used" only
>>> when these commands are executed outside of stdio) and explicitely use
>>> / read the workspace options if show_conflicts didn't get enough
>>> arguments.
>> I don't think this gains us anything. With use_workspace_options
>> true
>> for all automate commands, executing 'show_conflicts' via 'automate
>> stdio' processes the workspace options only once, in cpp_main.
>> On the other hand, setting use_workspace_options true loses the
>> workspace options when not running via automate stdio.
>> Emacs DVC runs automate both with via stdio and directly, although I
>> agree running via stdio is preferred in general.
>
> Thats why I want to make it explicit - if a command doesn't need a
> workspace, mtn should not even try to read any possible existing
> _MTN/options with some (yet unknown) side effects when it comes to
> options.

Well, that sounds reasonable, but it's a bigger change than I think we
should do on this branch. 

Setting use_workspace_options false affects the default behavior of
all automate commands, so I think it needs more discussion and thought.

If we are executing 'mtn automate stdio', it can't tell what commands
might be run in the future, so it can't tell whether to read the
workspace options. So it makes sense that it reads them by default,
just in case. But at the same time not complain if a workspace is not
found.

Then all other automate commands might as well assume the workspace
options have been read (if present), since they must all behave the
same when run via stdio or not.

Hmm. I suppose it would also be consistent for automate stdio to _not_
read the workspace options, but for each automate command that wants
them to read them. That duplicates work, but not a lot.

As I understand it, the only downside of leaving use_workspace_options
true is that mtn might accidently read some random file with
unpredictable results if run outside a workspace. But the name of the
file is quite specific, and there are format checks on the structure
of the file, so I think that risk is acceptably small.

> And thats why I also like to have automate show_conflicts behave like
> any other workspace automate command (f.e. inventory), which bails out
> with "workspace required, but not found" before anything else. 

That also seems like a good convention, but since
use_workspace_options is currently true, I don't think we can be sure
that all automate commands actually follow this convention.

In addition, automate show_conflicts is _not_ "like inventory"; it
only needs a workspace if it must search for the heads to compare, and
--branch is not given. So if the revids are given as arguments, or
--branch is given, it doesn't care whether a workspace is present.

After it figures out what revs to compare, it does not need a
workspace.

"need a workspace" can mean at least two different things. For
show_conflicts (and probably other commands), it just means "can get
arguments from _MTN/options". For other commands like inventory, the
workspace is needed for the basic operation of the command.

For the commands that needs a workspace for their basic operation,
bailing out early if the ws is not present makes sense. 

For commands that can get arguments from the ws, it makes more sense to
check each argument in turn, and give a nicer error message about how to
specify the argument.

> The "empty branch" error message is not explicitely telling anybody
> "hey, there is a workspace missing" - it could also be the case that
> for some weird reason the branch option in _MTN/options is just
> empty (who knows).

I just pushed a change to give the error message 

    misuse: please specify a branch, with --branch=BRANCH

when app.opts.branchname is the empty string. That's copied from the
merge command. I'm not clear if that's the right convention for
automate commands; it seems reasonable.

I guess it could also suggest "or execute in a workspace"; if so,
'merge' (and other commands) should also give that error message.

> And, lastly, I surely want that commands behave equally on stdio and
> non-stdio usage :)

That is true now (with use_workspace_options true), so I don't see
what the problem is.

-- 
-- Stephe