> -----Original Message-----
> From: Julian Foad [mailto:julianfoad_at_btopenworld.com]
> Sent: vrijdag 15 maart 2013 22:01
> To: Subversion Development
> Cc: Philip Martin; Stefan Sperling; Bert Huijben
> Subject: Conflict resolver callback design
>
> A long email, for folks interested in conflict resolution.
>
> I have been evaluating our conflict resolution callback support, partly in
the
> course of studying how to provide better conflict resolution, and partly
in the
> course of issue #4316 -- fixing 'merge' so that it continues to the next
revision
> range if you interactively resolve all conflicts.
>
> In this email I analyze the current state of support for the conflict
resolution
> callback design and suggest ways in which it could be improved. I haven't
> mentioned everything here -- for example the several known problems with
> the current "conflict description" data type.
>
> At the end I talk about how this impacts a fix for issue #4316.
>
> Don't panic -- I'm not suggesting we should hold up 1.8 for any of this,
and I
> haven't written a concrete proposal for such changes at this point. With
that
> in mind, comments welcome.
>
>
> INTRODUCTION
>
> We have a conflict resolver callback function ('CB') defined as:
>
> /** A callback used in merge, update and switch for resolving conflicts
> * during the application of a tree delta to a working copy. [...] */
> typedef svn_error_t *(*svn_wc_conflict_resolver_func2_t)(
> svn_wc_conflict_result_t **result,
> const svn_wc_conflict_description2_t *description,
> baton, result_pool, scratch_pool);
>
> The rough idea is that the client's callback is called every time a
conflict is
> raised, and can decide whether to postpone it (record it in the WC and
move
> on), or resolve the conflict according to some pre-determined fixed choice
or
> set of rules, or let the user interactively resolve it.
>
>
> === Pre-conflict and post-conflict operating modes.
>
> The problem is basically that we have confused two different operating
> modes -- with different purposes and requirements -- for such a callback:
>
> (1) Sometimes we call the CB before raising a conflict, in order to
decide
> whether we need to record a conflict or can immediately resolve it.
>
> (2) Other times we record a conflict in the WC and then, later, we call
the CB
> as a way of presenting the stored conflict to the client. In this case we
want
> the user to be offered a range of interactive options including examining
the
> conflict and the WC and making changes to the WC.
>
> If we like the "hook" terminology, we could call mode 1 a "pre-conflict
hook"
> and mode 2 a "post-conflict hook". Both of these modes have their uses.
>
> Mode 1 is viable for a text or prop conflict, since the result can
reasonably be
> assumed to be a single file or property value that is to be installed at
the
> current path. With a tree conflict, however, the result is expected to be
at
> least a tree change at the current path, and possibly (especially when
moves
> are involved) a tree change at another path (or paths). A tree change
> generally requires using the WC APIs to build the result incrementally,
not
> just returning a single object and asking the WC to install it.
>
> In mode 1, the CB needs to return quickly because we are in the middle of
an
> update, switch or merge, with a network connection open, and we haven't
> yet recorded the conflict or completed the update (or switch or merge) so
> the new WC state is not complete (for example the incoming change might
> be half of a move and we might not have received the other half of the
move
> yet). For both of these reasons, it is not appropriate to offer
interactive
> resolution. It may not be feasible to offer all forms of tree conflict
resolution,
> especially where a move is involved.
>
> Advantages of mode 1 include: it avoids sending 'C' notifications; it may
result
> in less CPU and network load. The avoidance of 'C' notifications may be
> important for improving signal to noise ratio, in large scale usage. It
is not
> necessarily a significant advantage, since the notification receiver
should be
> able to process them and filter them out if required, but that may add
> unwanted complexity to the receiver.
>
> In mode 2, the update or switch can be complete before we start resolving,
> or in a merge of multiple revision ranges, at least one revision range can
be
> completely merged.
>
> Mode 2 doesn't need a callback function to implement it: we could instead
> provide a "pull" interface for the client to request information about the
> conflicts and
>
> Advantages of mode 2 include: plenty of time for interactive resolution;
all
> the WC state is available to be examined; client can implement whatever
> logic it likes to resolve a conflict rather than having to provide one of
a limited
> number of answers; can operate on more than one node and so is suitable
> for tree conflicts and conflicts involving moves.
>
>
> When the callback is called, these conditions should be well defined ...
>
> - The conflict description provided.
> - Whether the WC write lock is held.
> - Whether the conflict is already recorded in the WC.
> - The WC state of the victim path.
> - The WC state of other related paths (such as parent, children,
move-to).
> - What the callback is allowed to do to the WC (queries, changes) in
order to
> resolve the conflict.
>
> ... but currently are not.
>
>
> ANALYSIS
>
> === Where is it called?
>
> CB is called during up/sw/merge/resolve.
>
> Passed through 'ctx' to these libsvn_client (public/inter-library)
functions:
> svn_client_update
> svn_client_switch
> svn_client_merge
> svn_client_resolve
>
> Passed by parameters to these libsvn_wc(public/inter-library) functions:
> svn_wc_merge5
> svn_wc_merge_props3
> svn_wc__get_update_editor
> svn_wc__get_switch_editor
> svn_wc__get_file_external_editor
> svn_wc__resolve_conflicts
>
> up/sw/merge: Library functions call CB only for text and prop conflicts.
> resolve: Library functions call CB for all conflicts.
>
> up/sw/merge: Library functions call CB *before* installing a conflict
> description in the WC DB (mode 1) -- at least for text & prop conflicts;
not
> sure about local-move tree conflicts.
This was the case in <= 1.7
> resolve: Library functions call CB *after* installing a conflict
description in the
> WC DB (mode 2).
And in 1.8 all the conflicts are installed before calling the resolver
functions. (Current state and has been so for at least a half year). It is
still invoked in separate places for tree and text conflicts though.
> 'svn update' (and 'svn switch') currently uses mode 2. When it calls the
> libsvn_client update function, it passes in a (mode 1) CB that simply
records
> the paths and postpones the conflict resolution. After the update is
> complete, it then calls 'resolve' on each recorded conflicted path (mode
2).
>
> 'svn merge' currently uses a mixture of mode 1 and mode 2. When
> interactive resolution is requested, it works the same way 'update' does;
> otherwise it uses mode 1 for text and prop conflicts and (AFAICS) mode 2
for
> tree conflicts.
Svn_wc_merge() and friends always install the conflicts now, so the complete
conflict is in the db for text, prop and tree conflicts before the conflict
resolver is called. (The conflict resolver is now invoked using
svn_wc__conflict_invoke_resolver())
So at least some of the information you summarized before is based on
looking at the 1.7 state.
>
>
> === Is the conflict description consistent?
>
> up/sw/merge: CB is passed a conflict description.
> resolve: CB is passed a *different* description for the same conflict, in
some
> cases (notably, property conflicts).
>
> I modified 'svn' so that it ran a merge using a mode 1 callback (for text
and
> prop conflicts) that records each conflict description, and then ran
'resolve'
> with a callback that compares each conflict description with the
> corresponding one that was recorded earlier. There are big differences in
> the property conflict descriptions. This is a symptom of using different
code
> paths to generate the descriptions.
>
>
> === The WC write lock
>
> up/sw/merge: CB is called while there is a WC write lock on the 'victim'
path.
> resolve: CB is called while there is a WC write lock covering 'all
possible paths
> affected by resolving' the conflicts in the given tree.
>
>
> === What may the CB do with the WC?
>
> It's not clear what the CB is allowed to do to the WC.
>
>
> === The 'adds_as_modification' flag
>
> Some existing APIs have an 'adds_as_modification' flag, which says don't
> raise a tree conflict for add-onto-add (if the node kind is the same), but
> instead resolve the potential conflict by collapsing the two adds and
treating
> the local add as the desired new value, so the result looks like a
modification
> -- or non-modification -- of the incoming added node.
>
> This flag is an example of a "mode 1" pre-determination for certain kinds
of
> conflict. This kind of functionality should be provided by a mode-1
conflict
> resolution callback.
>
> It is present in: svn_client_update4(), svn_client__update_internal(),
> svn_wc__get_update_editor().
This was introduced right before introducing tree conflicts.
Personally I would say we should move this behavior to a compatibility
conflict resolver and remove it from the update/switch handling.
>
>
> MERGE WITH INTERACTIVE RESOLUTION
>
> === Issue #4316 'Merge errors out after resolving conflicts'
>
> The interactive resolution for 'svn merge' is broken: it doesn't go on to
merge
> the remaining revision ranges even if you resolve all the conflicts --
issue
> #4316.
'svn' explicitly doesn't hook the conflict callbacks that it did hook in
1.7...
Most of this issue would be resolved by hooking the conflict callback again.
>
> In order to fix that, I have a patch which makes svn_client_merge*() call
the
> CB for all conflicts raised, after merging each revision range, and then
go on
> to the next revision range. Merge will no longer ever make "mode 1" calls
to
> the CB, always mode 2.
Nice, but if 'svn' doesn't hook the callback, this fix won't do a thing.
It might improve the behavior if we do both, but if 'svn' doesn't install
the conflict handler, it will just postpone.
> For consistency I would consider doing the same kind of thing to 'update'
and
> 'switch' APIs -- that is, make them call the CB at the end of the
operation, for
> each conflict that was raised, instead the 'svn' client running the update
with
> a 'postpone' CB installed and then running 'resolve' afterwards. I
haven't yet
> planned to do this but it would seem to be the sensible thing.
I think stsp did some work here, but most of this is in 'svn', not
libsvn_client.
>
> === Notifications
>
> This changes the notifications that are produced by a merge when using a
> fixed conflict resolution option such as '--accept=mine-full'. Now, even
these
> conflicts will result in a 'C' notification being sent to the notification
receiver,
> and later a 'Resolved conflicts on xxx' notification if the CB resolves
the
> conflict. I will attempt to patch the notification receiver in 'svn' to
notice if
> some or all of the reported conflicts were resolved, and at least make the
> summary of conflicts say something intelligent about that.
+1
I don't see why the notifications should be exactly the same as before, but
the summarizations should be ok.
>
>
> NEXT STEPS
>
> At some point after fixing #4316, and subject to feedback, I intend to
follow
> up with a more concrete proposal for how we can revise our API support for
> the conflict resolution callback, to recognize the difference between mode
1
> and mode 2. We should provide two very different callbacks or mechanisms,
> something very minimal or nothing at all for mode 1, and something rather
> more flexible than we have for mode 2.
>
> I'm not suggesting any of this should happen before 1.8, but now is the
time
> to say if you think it should.
>
> Thoughts?
>
> - Julian
>
>
>
> --
> Certified & Supported Apache Subversion Downloads:
> http://www.wandisco.com/subversion/download
Received on 2013-03-15 22:55:17 CET