Re: [Monotone-devel] resolving name conflicts; implementation issues

[Stephen Leake]

Stephen Leake <address@hidden> writes:

> The next step is to add the "suture" resolution. I'll call it
> "resolve_suture" instead of "resolve_content_ws". I suspect most
> people have a vague idea of what "suture" means in English, but will
> be motivated to check the manual to see what mtn means by it.
>
> "resolve_suture" will require a file name for the new content;  I
> don't want to require a workspace for this.
>
> For the first implementation, I'll simply drop both old node ids, and
> add a new one. This is clearly not enough, but does seem to be the
> consensus as a minimum requirement for "suture". Then we'll have a
> concrete example for further discussions on how to store the extra
> information required for history, future merging, etc.

This is implemented; see tests/resolve_duplicate_name_conflict in
726aaf7152d47e625f3217a587744bd5622e8590. That implements both the
"checkout.sh" and "thermostat.c" use cases. 'merge
--resolve-conflicts-file' succeeds with the following messages:

mtn: 2 heads on branch 'testbranch'
mtn: [left]  1337cb1059c4bc3e376b14381b43e9383c654da1
mtn: [right] d5f1dd136c86b5bbd5e71b0c3365667e328af492
mtn: suturing checkout.sh, checkout.sh into checkout.sh
mtn: renaming thermostat.c to thermostat-westinghouse.c
mtn: renaming thermostat.c to thermostat-honeywell.c
mtn: [merged] 76536a3d05eb605277e96523c51697be4803abff
mtn: note: your workspaces have not been updated

However, a subsequent 'update' drops checkout.sh:

mtn: updating along branch 'testbranch'
mtn: selected update target 76536a3d05eb605277e96523c51697be4803abff
mtn: [left]  084affdd52bf9d8dbb2ff94a848bc0c577a9dcc6
mtn: [right] 76536a3d05eb605277e96523c51697be4803abff
mtn: warning: Content changes to the file 'checkout.sh'
mtn: warning: will be ignored during this merge as the file has been
mtn: warning: removed on one side of the merge.  Affected revisions include:
mtn: warning: Revision: 084affdd52bf9d8dbb2ff94a848bc0c577a9dcc6
mtn: adding checkout.sh
mtn: renaming thermostat.c to thermostat-honeywell.c
mtn: adding thermostat-westinghouse.c
mtn: dropping checkout.sh
mtn: updated to base revision 76536a3d05eb605277e96523c51697be4803abff

The warning and 'dropping checkout.sh' are not what we want. This is
due to diediedie. 

Note that if I had sutured to a different name, the sutured file would
have survived, and could be renamed back later. But that's messy.

I have not looked at fixing it in detail yet. It may be that we should
merge this branch with nvm.mark-merge-existence; that looks promising.

I don't have any other tests for consistency, and I haven't run the
rest of the test suite.


One problem I had: an invariant failure in roster.cc
roster_t::drop_detached_node. The rationale for it is in a comment
starting at line 257 in roster.hh. In short, the current design says
you cannot drop a node that has never been attached. But roster_merge
puts conflict nodes in that state.

I think the simplest fix is to simply relax that requirement; allow
dropping nodes that have never been attached. That means the following
change in drop_detached_node:

-  safe_erase(old_locations, nid);
+  old_locations.erase(nid);

This breaks no existing functionality; it just means one invariant
check is removed, so some new bugs may be harder to find.

Another solution is to have roster_merge _not_ put conflict nodes in
the merged roster. But that seems like overkill.

I have a question; when are temporary node ids converted to true node
ids? I can't find that in the code.


> I'll add the resolution parsing functions as well.

This is done. I changed the format of the 'automate show_conflicts'
output slightly, to make it easier to read back in. I'll make the same
changes in main before the next release.

--
-- Stephe