Re: [Monotone-devel] automate show_conflict

[Thomas Keller]

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. 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 nor why we explicitely want to 
read / use workspace options for every automate command. 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. 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.

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 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'm now a bit undecided if automate show_conflicts really need such a
> > rather complex behaviour to get the heads it should work on, because
> > there is already everything in place to get started if run from a
> > workspace:
> >
> > a) mtn automate get_option branch to get the branch name
> > b) mtn automate heads BRANCHNAME to get the heads of the branch
> >
> > One could just fire the two commands above to feed proper arguments
> > into automate show_conflicts, or even allow a convenience --branch
> > option so a) would not be needed.
>
> The intent is to precisely follow the algorithm that 'merge' does in
> picking the first two heads to compare, since this is intended as a
> precursor to 'merge'. Your step a) is the same, but step b) is not.
> The simplest way to accomplish the intent is to call the actual
> head-finding code that merge does, which is what the current
> CMD_AUTOMATE(show_conflicts) code does when explicit revs are not
> given.
>
> That head-finding algorithm is _not_ trivial; see the new function
> 'find_heads_to_merge' in cmd_merging.cc (split out from the CMD(merge)
> code). It finds the best pair of heads to merge first, based on the
> ancestor tree structure.

Hrm, ok, you're right, I wasn't aware of this.

> I guess we should allow for a --branch option, so automate
> show_conflicts could be used outside a workspace even when not
> specifying the revs explicitly; that's easy to do.

+1

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