[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Monotone-devel] automate show_conflict

From: Thomas Keller
Subject: Re: [Monotone-devel] automate show_conflict
Date: Fri, 18 Apr 2008 23:59:11 +0200
User-agent: Thunderbird (Macintosh/20080213)

Stephen Leake schrieb:
+             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
+             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::process, by this

    // at this point we process the data from _MTN/options if
    // the command needs it.
    if (cmd->use_workspace_options())

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

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

That head-finding algorithm is _not_ trivial; see the new function
'find_heads_to_merge' in (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.



GPG-Key 0x160D1092 | address@hidden |
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:

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]