Re: [Monotone-devel] automate show_conflict

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Stephen Leake schrieb:
>>> +CMD_AUTOMATE(show_conflicts, N_("[LEFT_REVID RIGHT_REVID]"),
>>> +             N_("Shows the conflicts between two revisions."),
>>> +             N_("If no arguments are given, left_revid and
>>> right_revid default to the"
>>> +                "first two heads that would be chosen by the merge
>>> command."),
>>> +             options::opts::none)
>>> +{
>>> +  database    db(app);
>>> +  project_t   project(db);
>>> +  revision_id l_id, r_id;
>>> +
>>> +  if (args.size() == 0)
>>> +    {
>>> +      // get ids from heads
>>> +      set<revision_id> heads;
>>> +      project.get_branch_heads(app.opts.branchname, heads,
>>> +                               app.opts.ignore_suspend_certs);
>>>
>>> This won't work as expected. app.opts.branchname is not set because
>>> options::opts::none is defined and it is also not set through the
>>> workspace options because you did not require a workspace anywhere
>>> which would read those and set app.opts.branchname.
>> Ah. I did not test with an explicit branch option, nor outside a
>> workspace. I'll add tests for this.
>> On the other hand, this _does__ "work as expected" in a
>> workspace (see the tests automate_show_conflicts_defaults,
>> resolve_duplicate_name_conflict), so I did something right :).
>> Running in the debugger, app.opts.branchname is "testbranch" at this
>> point, with a command line of "mtn automate show_conflicts", in the
>> working directory tester_dir/tests/resolve_duplicate_name_conflict. So
>> something is parsing the options file.
>> This is apparently done in commands.cc commands::process, by this
>> code:
>>     // at this point we process the data from _MTN/options if
>>     // the command needs it.
>>     if (cmd->use_workspace_options())
>>       {
>>         workspace::check_ws_format();
>>         workspace::get_ws_options(app.opts);
>>       }
>> I don't clearly see how cmd-use_workspace_options() is set, but
>> there
>> is another macro CMD_NO_WORKSPACE in cmd.hh that is explicitly for
>> commands that should not use _MTN/options. So I think the current
>> CMD_AUTOMATE(show_conflicts) code is correct.
>
> m_use_workspace_options is set by the ctor of automate in
> cmd_automate.cc, line 42f. 

Ah, that makes sense.

> However, I cannot find a place where the automate class calls
> command::process where the code you pasted above is executed (and I
> grep'd over the sources, this is the only place where
> use_workspace_options() is called at all).
>
> So I still have no idea why it works then 

Here's the stack from the debugger, at the point of calling
'project.get_branch_heads':

#0  commands::automate_show_conflicts::exec_from_automate (this=0xb708c0, 
    address@hidden, address@hidden, address@hidden, address@hidden)
    at ../monotone.experiment.automate_show_conflict/cmd_merging.cc:962
#1  0x004d5aff in commands::automate::exec (this=0xb708c0, address@hidden, 
    address@hidden, address@hidden)
    at ../monotone.experiment.automate_show_conflict/cmd_automate.cc:62
#2  0x0045976b in commands::process (address@hidden, address@hidden, 
    address@hidden)
    at ../monotone.experiment.automate_show_conflict/commands.cc:777
#3  0x00756422 in cpp_main (argc=11, argv=0x32bf0)
    at ../monotone.experiment.automate_show_conflict/monotone.cc:265
#4  0x00757f9b in main (argc=11, argv=0x32bf0)
    at ../monotone.experiment.automate_show_conflict/win32/main.cc:200

command::process is called from cpp_main in monotone.cc

Note that I comment out the -O2 from CFLAGS and CXXFLAGS in the top
level Makefile when I want to run under the debugger.

> nor why we explicitely want to read / use workspace options for every
> automate command. 

I agree we should not _require_ a workspace for automate, but we
should use it, and in particular _MTN/options, when it is present. I
believe that's what the current code does.

If I run 'mtn automate show_conflicts' not in a workspace, it first
complains about "no db", then (when I specify --db) about "no heads on
branch ''". So then I allowed a --branch option, and it works.

> I set the boolean value to false in the automate ctor and ran the
> testsuite over the binary and all 43 automate-related tests
> succeeded. 

The test suite normally specifies all options, thus not relying on the
workspace options. So this result is not particularly relevant. Emacs
DVC relies on the workspace options for automate commands. 

In general, I'm working on writing more tests that explicitly use the
workspace options.

> When I ran this on your two new tests, the second one
> (automate_show_conflicts_default) fails in line 46 where it is
> unable to determine the branch, so it _seems_ to be executed
> somewhere.

Ah, good; that means it is testing the use of the workspace options
:).

I'll add a case where --branch is specified.

> After digging a little deeper I tried to run the original code (with
> use_workspace_options set to true) via stdio and this gave the result
> I expected for normal execution:
>
> $ echo l14:show_conflictse | ../../../mtn automate stdio
> 0:2:l:68:misuse: branch '' has 0 heads; must be at least 2 for
> show_conflicts
>
> (this was executed in the test directory for
> automate_show_conflicts_default after I added a check(false) in line
> 46 in the test driver)

In this case, the stack from the debugger is:

#0  commands::automate_show_conflicts::exec_from_automate (this=0xb708c0, 
    address@hidden, address@hidden, address@hidden, address@hidden)
    at ../monotone.experiment.automate_show_conflict/cmd_merging.cc:962
#1  0x004d94c9 in commands::automate_stdio::exec_from_automate (
    this=0xb724e0, address@hidden, address@hidden, address@hidden, 
    address@hidden)
    at ../monotone.experiment.automate_show_conflict/cmd_automate.cc:403
#2  0x004d5aff in commands::automate::exec (this=0xb724e0, address@hidden, 
    address@hidden, address@hidden)
    at ../monotone.experiment.automate_show_conflict/cmd_automate.cc:62
#3  0x0045976b in commands::process (address@hidden, address@hidden, 
    address@hidden)
    at ../monotone.experiment.automate_show_conflict/commands.cc:777
#4  0x00756422 in cpp_main (argc=3, argv=0x32f20)
    at ../monotone.experiment.automate_show_conflict/monotone.cc:265
#5  0x00757f9b in main (argc=3, argv=0x32f20)
    at ../monotone.experiment.automate_show_conflict/win32/main.cc:200

This is the same as the stack above, with one additional frame for
automate_stdio::exec_from_automate.

So in effect the workspace options are processed by 'automate stdio',
not 'show_conflicts'.

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

-- 
-- Stephe