[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 00:22:21 +0200
User-agent: Thunderbird (Macintosh/20080213)

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!

--- c6e067dec1263e430f22e39fc63e30932d1c02d1
+++ f4dc9ea13418e8bc3e80e367503cf48d52f31f6c
@@ -1248,7 +1248,7 @@ CMD_AUTOMATE(get_current_revision, N_("[
                            excluded, join_words(execid));
   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? :)

+             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 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")
 -- FIXME: this should probably be set globally in lua-testsuite.lua for
 --        all tests.
+if ostype == "Windows" then
+set_env("APPDATA", test.root.."/empty")
 set_env("HOME", test.root.."/empty")
 srv = bg(pure_mtn("serve"), 1, false, true)

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


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]