Re: Merge conflict resolution

Thanks for the thoughtful and detailed response, Stefan.

On Wed, 22 Nov 2006, Stefan Küng wrote:

> Daniel Rall wrote:
>
> >While conflict markers might not currently seem like much of a problem
> >to most people (at least, to those not grumbling that Subversion is
> >"corrupting their files"), Merge Tracking really complicates things.
> >In today's Subversion, we apply only one revision range during any
> >merge operation. With Merge Tracking, we may apply multiple revision
> >ranges during a single merge operation, a rather fundamental
> >difference with many ramifications. Once a file has a merge conflict,
> >we have to stop applying revision ranges, meaning that we might apply
> >only a portion of the revisions requested for merge by the user.
>
> What happens if such a conflict would not exist anymore if all revision
> ranges got applied? I'm thinking of a situation where the same line got
> changed multiple times over multiple ranges, and the last change would
> render the file so that no conflict would occur.
> Does it create a conflict which has to be resolved or will that be
> handled 'gracefully'?

The current implementation on the merge-tracking branch does not
handle this situation in the manner you describe above. We basically
wrap the guts of 'merge' in a loop, apply one revision range at a time
to the WC (as if you'd run 'merge' multiple times), recording merge
history incrementally as we go.

Truthfully, I have a hard time imagining how to successfully implement
what you describe above. Any ideas?

I'm really open to anything here, including a completely different
approach than my suggested conflict resolution callback.

> >Requirements:
> >
> >- Allow merge conflicts to be resolved during application of a merge.
> >
> >- Allow as much automatic merging to be performed as possible without
> >requiring human interaction.
> >
> >- Allow declarative conflict resolution to avoid any human interaction
> >(e.g. --resolve-conflict-using=[older|theirs|mine]), supporting some
> >degree of conflict resolution from merges (e.g. build automation).
> >
> >- Maintain compatibility with the diff3 command.
> >
> >- Allow for resolution more types of conflicts (e.g properties,
> >obstructions, etc.).
>
> Will it be possible to have it work like before? Without user
> interaction *during* the merge but get conflicted files which must
> be resolved later?

As currently implemented on the merge-tracking branch, user
interaction is avoided for (internal) merges of revision ranges up
until there is a conflict. When a conflict occurs, merging of
subsequent revision ranges is stopped.

I'm proposing we inject as-needed conflict resolution so that it's
possible to apply the entire set of requested revision ranges. This
does not completely remove the problem -- an unresolved conflict may
leave us in same situation, with potentially only a portion of the
requested revisions applied by the merge.

Let's say I run 'svn merge -r 3:10 $src_url' into a WC which has
already had revisions 4-6 and 8-9 from $src_url merged into it. This
leaves revisions 3, 7, and 10 yet to be merged. Today on the
merge-tracking branch, if 7 yields a conflict, revision 10 is never
applied (this happens silently). This is a major departure from the
current behavior, effectively changing 'merge' to accept multiple
revision ranges and suffering the consequences. This is the problem
I'm trying to solve.

> I'm thinking of situations with flaky network connections, where a user
> might want to do the merging in one step without interruption, and then
> do the resolving later.
> In which state of the merge is the callback called? Just to avoid
> timeout errors when the user takes too long to resolve such a conflict
> during the merge operation.

Garrett Rooney and Madan S. had a similar concern. A network timeout
could potentially occur. However, applying only a portion of the
requested merge seems less than ideal. Do you have a better idea how
to handle the scenario I describe above? I'm all ears. :-)

> >Possible implementation:
> >
> >Subversion will first attempt a merge via its built-in diff library,
> >or the diff3 command specified in its config file (or via the
> >--diff3-cmd option, for the command-line client). If a merge conflict
> >occurs to either content (file or directory) or properties, it will
> >invoke a merge conflict resolution callback provided via its
> >svn_client_ctx_t structure (we may provide a default implementation of
> >this which calls out to an external binary a la the diff3 command used
> >in the original merge). This callback would prompt the user to
> >resolve the conflict (when in non-interactive mode), and upon
> ^^^
> :)

Yup -- *interactive*, thanks.

> >resolution allow subsequent merge of revision ranges to occur into the
> >same file (important for Subversion's Merge Tracking support). For
> >the command-line client, the callback would throw up a prompt
> >something like:
> >
> > Merge conflict detected for "foo.c" at line 37
> > mine:
> > struct iovec vec;
> > theirs:
> > int k;
> > older:
> > int i;
> > Retain (m)ine, (t)hiers, or (o)lder version? _
>
> I suggest to include the line numbers there. Otherwise it may be hard
> for the user to find out where exactly in the files that context is.

I agree (this is just an example, not something I've implemented).
Perhaps something like:

  Merge conflict detected for "foo.c":
  mine (line 37):
   struct iovec vec;
  theirs (line 36):
   int k;
  older (line 32):
   int i;
  Retain (m)ine, (t)hiers, or (o)lder version? _

> >Question for IDE developers:
> >
> >I'm really curious how IDEs handle this situation today. Taking
> >TortoiseSVN as an example, I've heard that its TortoiseMerge diff3
> >program is great for conflict resolution (though currently lacking an
> >edit mode), but not so great for performing merges of a large number
> >of changes (e.g. merging changes from trunk into a feature branch),
> >especially when the majority of those changes can be applied
> >automaticaly with no merge conflicts (and thus don't need typically
> >need a merge GUI). Is this an issue for TortoiseSVN/TortoiseMerge
> >today, or am I out of date? How about for other IDE developers? How
> >do you guys handle this today? ;-)
>
> If the callback provides the information so that the UI merge tools
> can use whole files, this will work ok. If however the callback only
> provides the context of the file where the conflict occurs, we'd
> have to write a custom merge tool just for this purpose - which is
> maybe not such a bad idea anyway.

I'd prefer the callback to be usable by invoking third-party merge
tools (e.g. the TortoiseMerge GUI), so supporint use of whole files
seems like the way to go. A custom merge implementation could
interrogate the files for more context (e.g. like the command-line
version shown in the example above).

> [copied from other mail]
> > I'm assuming that the callback will take care of doing the actual
> > merge, and that Subversion libraries will handle the WC meta data
> > manipulation. Here's the API I've integrated in the tree delta editor
> > (from libsvn_client/repos_diff.c); I may actually want to push this
> > deeper, down into the WC's editor:
>
> Is the Subversion internal diff3 called first and this callback only
> called if there is a conflict?

That's what I'd recommend. As today, you could always use a custom
diff3 command (e.g. GNU diff), which has an interface contract
allowing it to communicate conflict status back to Subversion by way
of exit codes:

http://svnbook.red-bean.com/en/1.2/svn.advanced.externaldifftools.html

Regardless of diff3, the conflict resolution callback would only be
invoked if the initial diff3 command indicated that a path was in
conflict after the merge attempt.

> > /** A callback used for resolving merge conflicts to content or
> > * properties encountered during the application of a tree delta to a
> > * working copy.
> > *
> > * This callback is only invoked when not in "dry run" mode. It
> > * provides for a smooth balance of automated merging via a
> > * non-interactive diff3 mechanism like Subversion's internal
> > * implementation (or an external tool like GNU diff specified via @c
> > * SVN_CONFIG_OPTION_DIFF3_CMD), and declarative or interactive merge
> > * conflict resolution (e.g via an external tool like TortoiseMerge or
> > * Araxis Merge).
> > *
> > * If @a content_state and/or @a prop_state are not @c NULL, and the
> > * conflict is resolved during the callback, set @a content_state
> > * and/or @a prop_state appropriately (e.g. likely to @c
> > * svn_wc_notify_state_merged); housekeeping for the resolution of @a
> > * path will occur automatically. If @a content_state and/or @a
> > * prop_state are not @c NULL but returned unchanged, it's assumed
> > * that the conflict has not been resolved.
> > *
> > * All allocations should be performed in @a pool.
> > *
> > * @a baton is a closure object; it should be provided by the
> > implementation,
> > * and passed by the caller.
> > *
> > * @since New in 1.5.
> > */
> > typedef svn_error_t *(*svn_client_conflict_resolver_func_t)
> > (const char *path,
> > svn_wc_notify_state_t *content_state,
> > svn_wc_notify_state_t *prop_state,
> > svn_boolean_t dry_run,
> > void *baton,
> > apr_pool_t *pool);
>
> I see that this callback only has one 'path' parameter. I don't see
> where the information would be to actually do a merge? Doesn't a merge
> require at least three files/paths to actually get a conflict which must
> be resolved?

In my prototyping, I made the simplifying (for me :) assumption that
the other paths could be extrapolated from the FS path (using some
Subversion svn_wc.h API, I'm guessing). Putting three paths into the
API signature would make it simpler to implement, though.

> Some other thoughts:
> * sometimes it's not possible to resolve a conflict without modifying
> also other files (not just the one which has the conflict). Will it be
> possible for the UI merge tool to just tell "sorry, can't resolve this"
> and Subversion will then behave as before and leave the file conflicted?

Yes, as with custom diff3's. On the merge-tracking branch, this has
the consequences described above (potential partial merge). On
Subversion's trunk, it would behave as today.

> * it would be nice if the svn:mime-type property value also is passed to
> the callback: UI tools could use this information to find out what the
> best way would be to resolve a conflict or just to simply adjust the UI
> according to the mime type

Good suggestion. What if the path in conflict is a directory? What
if it's the path's properties that are in conflict? Should we still
pass in the MIME type?

> * if the callback does not provide only the context where the conflict
> occurs but whole files, an information is required where Subversion can
> not resolve the conflict itself: UI merge tools may use a different
> diff3 algorithm which won't see a conflict where Subversion sees one.
> Maybe the line numbers of where the conflict is supposed to be.

So, in addition to the whole file, some additional context describing
where Subversion found a conflict. How do we handle this when a
custom diff3 implementation is used?