Re: [Monotone-devel] automate show_conflict

[Thomas Keller]

Stephen Leake schrieb:
> Stephen Leake <address@hidden> writes:
>
> > Thomas Keller <address@hidden> writes:
> >
> > > Stephen Leake schrieb:
> > > > This is now complete; 'mtn automate show_conflicts' outputs basic_io
> > > > for all conflict cases.
> > > > So I think the branch n.v.m.automate_show_conflict is ready to be
> > > > merged to main. Someone may want to review it first. And I have to
> > > > review the automate version number; there's been a release since I
> > > > checked it.
> > > The diff of nvm against nvm.automate_show_conflict is quite huge
> > > (>25.000 lines). It seems that nvm.automate_show_conflict doesn't
> > > contain all the recent changes in nvm. Could you therefor please
> > > propagate from nvm to your branch first and resubmit your request?
> > Good point. 
> >
> > 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!



============================================================
--- 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? :)




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

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.




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




+  if (file_type == get_type (*ancestor_roster, conflict.nid))
+    {
+      st.push_str_pair(syms::node_type, "file");
+      st.push_str_pair(syms::attr_name, conflict.key());
+      file_id ancestor_fid;
+      db_adaptor.db.get_file_content (db_adaptor.lca, conflict.nid, 
ancestor_fid);
+      st.push_str_pair(syms::ancestor_name, ancestor_name.as_external());
+      st.push_binary_pair(syms::ancestor_file_id, ancestor_fid.inner());
+      // FIXME: don't have this. 
st.push_str_pair(syms::ancestor_attr_value, ???);
+      file_id left_fid;
+      db_adaptor.db.get_file_content (db_adaptor.left_rid, 
conflict.nid, left_fid);
+      st.push_str_pair(syms::left_name, left_name.as_external());
+      st.push_binary_pair(syms::left_file_id, left_fid.inner());
+      put_attr_state_left (st, conflict);
+      file_id right_fid;
+      db_adaptor.db.get_file_content (db_adaptor.right_rid, 
conflict.nid, right_fid);
+      st.push_str_pair(syms::right_name, right_name.as_external());
+      st.push_binary_pair(syms::right_file_id, right_fid.inner());
+      put_attr_state_right (st, conflict);
+    }
+  else
+    {
+      st.push_str_pair(syms::node_type, "directory");
+      st.push_str_pair(syms::attr_name, conflict.key());
+      st.push_str_pair(syms::ancestor_name, ancestor_name.as_external());
+      // FIXME: don't have this. 
st.push_str_pair(syms::ancestor_attr_value, ???);
+      st.push_str_pair(syms::left_name, left_name.as_external());
+      put_attr_state_left (st, conflict);
+      st.push_str_pair(syms::right_name, right_name.as_external());
+      put_attr_state_right (st, conflict);
+    }

To avoid code duplication in these if ... else ... constructs in 
roster_merge.cc I'd have probably reordered the output stanzas to list 
all file / directory specific things on top and the common things below 
- but since this would mean that you'd have to redo all the test stdout 
files I don't think its a good idea particularily now...



Also in the same file I see a lot of these (but they're not always there):

+  boost::shared_ptr<roster_t const> 
left_roster(db_adaptor.rosters[db_adaptor.left_rid]);
+  I(0 != left_roster);
+  boost::shared_ptr<roster_t const> 
right_roster(db_adaptor.rosters[db_adaptor.right_rid]);
+  I(0 != right_roster);

I don't know much of boost::shared_ptr, but basically any of both 
invariants would fire if the db_adaptor.rosters map does not have the 
revision roster cached, correct? If we need invariants there, wouldn't 
it be better to check the root of the paranoia, i.e.

I(db_adaptor.rosters.find(db_adaptor.left_rid) != db_adaptor.rosters.end())

? But yeah, this looks pretty ugly ;)




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



============================================================
--- 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?



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.

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

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: show_conflicts has no idea if the line merger can solve 
those, and mtn merge has AFAIK no --dry-run option either to tell apart 
content merges which need user interaction (which then could be a use 
case for show_conflicts again) and those who don't.

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