Re: [Monotone-devel] automate show_conflict

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Stephen Leake schrieb:
>>> Doing that required some code fixes to deal with the hexenc changes,
>>> and now some tests are failing. So I have more work to do.
>> Ok, everything is merged, fixed, and pushed; ready for review.
>
> Many thanks for your work! Here we go:
>
> [extensive tests]
>
> Wow, I haven't read through them, so I just trust you here that they
> all do test what they should - well done!

Thanks.

To be fair, I borrowed all of the tests from
conflict_messages/__driver__.lua; all the work of constructing the
test cases was done there.

And not all branches of the if .. else constructs in cmd_merging.cc
are actually tested; often conflicts are asymmetric in the left/right
revisions, but only one case is tested. If you go thru the change
history in detail, you'll see that; there is a point where it says
"all tests pass", and a later point where it says "all missing cases
filled in". I decided the code was simple enough to get right by
symmetry to not require more tests. 

> ============================================================
> --- automate.cc c6e067dec1263e430f22e39fc63e30932d1c02d1
> +++ automate.cc f4dc9ea13418e8bc3e80e367503cf48d52f31f6c
> @@ -1248,7 +1248,7 @@ CMD_AUTOMATE(get_current_revision, N_("[
>                             excluded, join_words(execid));
>    rev.check_sane();
>    N(rev.is_nontrivial(), F("no changes to commit"));
> -
> +
>    calculate_ident(rev, ident);
>    write_revision(rev, dat);
>
> This is spurious and shouldn't be there - with what have you played
> around here? :)

This is just a whitespace change. monotone/HACKING says:

    Please review your patch prior to submission, to not include
    accidental white-space-only changes 

In general, I agree with this and try to follow it. But exceptions do
get thru, especially when merging; I think this happened on the last
propagate from n.v.m. See my other post on a new merge process for
help in this area.

On the other hand, it is much easier to follow this policy if everyone
follows a standard "no trailing whitespace" policy. I have Emacs set
to trim trailing whitespace from all files when saved. That seems the
simplest solution in practice.

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

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

The branch is not needed if explicit revs are given; it is only used
to find the revs to compare.

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.

> +    }
> +  else
> +    throw usage(execid);
>
> A minor thing - please use
>
>   N(false, F("wrong argument count"));
>
> instead which is used for all automate commands. We decided a while
> back that it doesn't help to throw a full-blown help message to the
> developer just because he implemented something wrong...

Ah. I copied the code from CMD(show_conflicts); I see that there is a
different convention for CMD_AUTOMATE. That makes sense.

I'll add a test for this as well.

> +  if (file_type == get_type (*ancestor_roster, conflict.nid))
> +    {
> <snip>
> +    }
> +  else
> +    {
> + <snip>
> +    }
>
> To avoid code duplication in these if ... else ... constructs in
> roster_merge.cc 

I was not happy with the amount of code duplication here either.
However, those if .. else constructs where already there, for the
show_conflicts command. Since the tests in tests/conflict_messages do
_not_ actually check the messages in detail, I was not clear I would
not break something by rearranging this structure. So I kept it. 

Now that we have a more thorough (although still not complete) test,
it would be possible to restructure this to save some code
duplication. But possibly not worth it.

> ============================================================
> --- tests/conflict_messages/__driver__.lua
> 4294e488db07d33260abd6ab276fc85d9c2dd508
> +++ tests/conflict_messages/__driver__.lua
> a681cb85649f5e5ce53bad9b8ef8f2300c22d9d8
> @@ -36,7 +36,7 @@ message = "conflict: missing root direct
>
>  message = "conflict: missing root directory"
>
> -check(mtn("update", "--debug"), 1, false, true)
> +check(mtn("update"), 1, false, true)
>  check(qgrep(message, "stderr"))
>
> This shouldn't be part of the patch, but I guess its ok.

I guess I could have done this on mainline, as part of a "cleanup"
patch. My habit is to check in such minor things as I find them, and
not worry too much about it.

> ============================================================
> --- tests/non_workspace_keydir/__driver__.lua
> d32720e0e63d4429dc590c0849a1eb8794848b06
> +++ tests/non_workspace_keydir/__driver__.lua
> 7c2ff80fd51ef2ff5a4402a71dd576ac39a1f0d4
> @@ -50,7 +50,11 @@ mkdir(test.root.."/empty")
>  mkdir(test.root.."/empty")
>  -- FIXME: this should probably be set globally in lua-testsuite.lua for
>  --        all tests.
> +if ostype == "Windows" then
> +set_env("APPDATA", test.root.."/empty")
> +else
>  set_env("HOME", test.root.."/empty")
> +end
>  srv = bg(pure_mtn("serve"), 1, false, true)
>  sleep(2)
>  srv:finish()
>
> But this should definitely not be part of the patch, should it?

Well, it depends on whether you want all the tests to pass, or not. I
did make this same change on mainline, but it's not checked in yet.

I guess these things do contribute to the work of reviewing the
branch, so it would be better if they were not there. I'll keep that
in mind for the future.

> Overall I have a bit of a mixed feeling about the implementation of
> the stanza generation in roster_merge.cc, especially because of all
> the scattered if (basic_io) ... else calls. What I would have
> envisioned here is that these functions would return basic_io in
> general as internal informational data structure, which would later be
> "postprocessed" for a nice user-understandable output of mtn
> show_conflicts or just be printed to the output stream for mtn
> automate show_conflicts.

I don't think that postprocessor would be easy to write. And it's
certainly not how the code was written in the first place, so it's not
a minimal change.

It would be cleaner to have two parallel sets of functions, one that
does basic_io for automate and one that does stdio for non-automate. I
didn't do that because I wanted to take advantage of the existing if
.. else structure. In hindsight, I think it would have been better to
start from scratch.

> Beside that automate show_conflicts can only be the first step towards
> automated conflict resolution, especially since content conflicts are
> probably that one type which would get reported on quite a lot merges,
> but for which no automate (line-) merge commands exist yet. We should
> discuss further how this could work out (i.e. if we like to depend on
> the internal line merger of monotone and tack an automate interface on
> it or if content merging should be entirely in the scope of the user's
> client or both).

Yes. See my other post on a proposed merge process. I did not
explicitly propose an "automate merge", but that would make sense.

> Also, any further call to mtn merge would (even though the existance
> of show_conflicts and the possibility to easily solve non-content
> conflicts before the merge with it) behave unpredictable when it comes
> to content conflicts: 

This is the point of the proposed merge process; to allow merge to
resovle _all_ conflicts nicely/predictably, with appropriate input
prepared ahead of time by the user.

-- 
-- Stephe