Re: [Monotone-devel] nvm.resolve_conflicts ready to land on mainline

[Tero Koskinen]

Hi,

On Sat, 01 Nov 2008 12:26:21 -0400 Stephen Leake wrote:

> I believe nvm.resolve_conflicts is ready to land on mainline.

I briefly looked at nvm.resolve_conflicts. A few comments:

* Builds and all tests pass on OpenBSD/i386.
* Documentation looks nice. I was able to figure out
  how to use the commands.
* Code (cmd_conflicts.cc) was pretty easy to read and understand.

However, it tooks quite a lot of commands to get
two revisions with conflicts merged. I would like
to have some kind of simple interactive menu system
instead of multiple commands. Perhaps something
like what Darcs or mergemaster have.

In addition, I found a bug from set_first_conflict function:
when doing command "mtn conflicts resolve_first interactive",
mtn crashes, because 
  N(args.size() == 2, F("wrong number of arguments"));
check is done after calling 
  idx(args,1)())

A patch below makes the interactive command to use the name
of the conflicting file by default if no name is given.
It also adds a comment to I(false) and changes two consecutive
if statements into if-else pair.

#
# old_revision [7ab23cfe863526079fd6407f129899b1dea8a4e1]
#
# patch "cmd_conflicts.cc"
#  from [e024806a8cf16675a1dfa7c1dca4464c3e6d0054]
#    to [6a7a4ab9bcb46b37bd17dd7ae81398e686e0dd86]
#
============================================================
--- cmd_conflicts.cc    e024806a8cf16675a1dfa7c1dca4464c3e6d0054
+++ cmd_conflicts.cc    6a7a4ab9bcb46b37bd17dd7ae81398e686e0dd86
@@ -272,12 +272,12 @@ set_first_conflict(database & db,
               break;
 
             case neither:
-              I(false);
+              I(false); // not reached
             }
         }
     }
 
-  if (side == neither)
+  else
     {
       for (std::vector<file_content_conflict>::iterator i = 
conflicts.result.file_content_conflicts.begin();
            i != conflicts.result.file_content_conflicts.end();
@@ -289,12 +289,25 @@ set_first_conflict(database & db,
             {
               if ("interactive" == idx(args,0)())
                 {
-                  
N(bookkeeping_path::external_string_is_bookkeeping_path(utf8(idx(args,1)())),
-                    F("result path must be under _MTN"));
-                  bookkeeping_path const result_path(idx(args,1)());
+                  bookkeeping_path result_path;
 
-                  N(args.size() == 2, F("wrong number of arguments"));
+                  if (args.size() == 1)
+                    {
+                      file_path conflict_file_name;
+                      conflicts.left_roster->get_name(conflict.nid, 
conflict_file_name);
+                      result_path = bookkeeping_root / "resolutions" / 
conflict_file_name.as_internal().c_str();
+                    }
+                  else if (args.size() == 2)
+                    {
+                      
N(bookkeeping_path::external_string_is_bookkeeping_path(utf8(idx(args,1)())),
+                        F("result path must be under _MTN"));
+                      result_path = idx(args,1)();
+                    }
+                  else
+                    F("wrong number of arguments");
 
+                  I(!result_path.empty());
+
                   if (do_interactive_merge(db, lua, conflicts, conflict.nid,
                                            conflict.ancestor, conflict.left, 
conflict.right, result_path))
                     {


Ps. Code seems to have multiple over 110 characters long lines,
does the Monotone coding standards say anything about maximum line
length?

-- 
Tero Koskinen <address@hidden>