On Thu, May 17, 2012 at 11:09:48AM +0100, Julian Foad wrote:
> Let's take a closer look at what the user (through the client
> application) needs to be able to do.
Thanks for your comments. I think we're about to each consensus
on this. We just have a slight misunderstanding to clear up, and
need to agree on whether the main resolution logic should be implemented
by clients themselves, or in the library.
> > /* The set of all conflict choices for all kinds of conflicts. */
> > enum svn_client_resolution_choice_t {
> > svn_client_resolution_choice_mark_resolved,
> > svn_client_resolution_choice_theirs_full,
> > svn_client_resolution_choice_mine_full,
>
> "theirs-full" and "mine-full" are nice and simple: each choice leads to a
> single, definite result. If the user chooses one of these, the
> Subversion library can simply execute the user's decision with no
> further info or interaction needed.
Yes.
> > svn_client_resolution_choice_theirs_conflict,
> > svn_client_resolution_choice_mine_conflict,
>
> "theirs-conflict" and "mine-conflict" are good options for an
> interactive 3-way merge tool to offer the user while showing the user
> which sections of the file do have conflicts and which don't. It's not
> an option I'd like to choose outside a 3-way merge tool. In terms of
> tree conflicts, the same applies: I'd only be comfortable with this
> option when it's presented in a 3-way directory-merge window that's
> showing me which children are conflicting and which are not. After
> pressing this button in the 3-way (file or dir) merge tool, I wouldn't
> expect the tool to immediately close and Subversion library to
> immediately finalize my choice, instead I'd expect to be able to review
> and edit the result and then finally confirm.
Agreed.
> > [...]
> > svn_client_resolution_choice_diff_full,
>
> "diff-full" isn't a resolution choice, it's the user asking to see
> more information before choosing.
There seems to be a slight misunderstanding here.
I don't intend the values of this enum to represent "final" resolution
choices. Rather, each choice allows the conflict resolver to make
progress in the resolution process. Some options may result in a "final"
resolution state where the conflict is resolved. Other options may simply
allow the user to request information or carry out other actions that
are in the set of permitted options of the current resolver state.
In technical terms, I think of the new resolver as a state machine,
where each conflict choice triggers a state transition. The set of
final states may either resolve the conflict or postpone resolution.
Maybe we need a different name than svn_client_resolution_choice_t to
avoid this misunderstanding? Note that it isn't called
svn_client_final_resolution_choice_t :)
Wherever you say "not a resolution" below, my above response applies.
> We need to get away from the mode
> of operation of the existing interactive resolver loop where the user
> interacts my means of question-and-answer. That question-and-answer
> loop should be in the presentation layer (svn), not in the library.
Here I disagree. The loop we have today is a small state machine
that runs within the 'svn' client. We need this loop in one form
or another, either in the client or in the library. Today it is in
the client, so each client has to reimplement this loop with a set of
possible states and transitions. You seem to favour this model but I
don't see any advantage in keeping it.
I want to put as many aspects of the resolution process as possible into
the library so that each client uses the same state machine to resolve
conflicts. This makes it easier to work with several clients simultaneously
because terminology and logic will be the same for all clients during
the conflict resolution process. And it allows clients to implement
conflict resolution dialogs with much less effort.
> > svn_client_resolution_choice_edit_file,
>
> "edit-file" isn't a resolution choice that the library can then
> "execute" in a black-box manner. Instead, the user wants the GUI to
> help him/her prepare the final result with the aid of an editor, and
> then the user may choose to confirm that resulting file as the final
> resolved result. The resolution needs to be communicated to the Svn lib
> in the form of a file, not "he chose 'edit' as the resolution".
Yes, the library will receive a file.
However, it will also have an internal resolver state that makes the
acceptance of such a file a valid resolution choice. For example, it
probably makes no sense to provide a file when resolving a directory
vs. directory tree conflict. If the resolver knows the set of valid
options for the conflict in question, it can reject such invalid input
from clients. Your opinion seems to be that the decision about whether
a choice is valid or not belongs into the client. I say that leads to
unnecessary complexity and variance between client implementations.
> > [...]
> > svn_client_resolution_choice_scan_log_for_moves,
>
> Not a resolution. Instead, there should be a way for the GUI to get the
> suggestion(s) of what moves might be applicable, and to offer those as
> suggestions to the user, and then for the user to select maybe one of
> the choices or maybe another resolution.
Again, this is a good description of what I intend to do. It seems
we're on the same page but you misunderstood my intentions.
> > svn_client_resolution_choice_specify_local_target_for_incoming_move,
> > svn_client_resolution_choice_merge_edits_into_local_file,
> > [...]
> > };
>
> A GUI wants the user to be in control. The first obvious thing is the
> user should be able to select which file/dir to resolve next. If the
> GUI wants to have a button that enters a loop that iterates over all
> conflicts sequentially, that's it's business and we should make it
> possible to do that, but we should not implement this loop in the Svn
> lib.
I realised after comments from Bert and Mark that we need to run the
main loop of the resolution process within the client, not within the
library. So I've ditched the callback-table idea in favour of a set of
functions that receive a resolver state object which is opaque to the
client (I'll elaborate on that in a different post). This way, the library
can keep state and reject invalid or nonsense choices it receives
from the client, and clients are in control of the overall flow.
> The overall process the user goes through to resolve a particular
> conflict needs to include a reviewing phase, an editing phase, and
> finally a confirmation that the result is as the user wants it. How
> does this fit with the idea of there being a point where the user
> chooses a resolution such as "choose edit-file"?
It fits because "choose edit-file" is just a state transition,
rather than a final choice.
> While in the reviewing phase of resolving any particular conflict, the
> user should be able to bring up, on demand, a diff between 'mine' and
> 'base' or 'mine:theirs' or 'base:theirs' or whatever he/she chooses,
> jump to the location in their editor where the next text conflict
> occurs, review the log messages on the source and/or target branch,
> and so on. And all of this in parallel, in separate, non-modal
> windows.
Indeed, clients should be allowed to do all that. Do you think we even
need to design the API such that clients can resolve multiple conflicts
in parallel? Or should we restrict clients to handle one conflict at
a time? This restriction might make it easier to implement the resolver.
We could also make this restriction initially and lift it later.
> To enable this, the application needs access to the metadata about the
> conflict. This metadata will include at least the WC location or repository
> location (URL, rev, etc.) of each of the three files/dirs involved. It needs
> to be sufficient to be able to display any required diffs, displaying
> friendly file
> names (not the names of temp files) and labels (e.g. "Mine", "Theirs").
>
> In short, it needs information like this:
>
> svn_node_kind_t node_kind;
> svn_wc_conflict_kind_t kind;
>
> svn_wc_conflict_action_t action;
> svn_wc_conflict_reason_t reason;
> svn_wc_operation_t operation;
>
> const char *base_abspath;
> const char *their_abspath;
> const char *my_abspath;
> const char *merged_file;
>
> const svn_wc_conflict_version_t *src_left_version;
> const svn_wc_conflict_version_t *src_right_version;
>
> ... which is an extract from svn_wc_conflict_description2_t. Certainly we
> don't want the data to be presented in an "svn_wc_" struct, but I can't get
> away from the fact that that's the kind of metadata we need.
This is the kind of data the resolver can use to determine a set of
valid states and transitions for conflict resolution.
The client will drive the state machine so just giving it an
svn_wc_conflict_description2_t struct an insufficient client<->library
interface. At the very least, we must also force clients to pass back
in any state the resolver needs to keep.
Does all that make sense?
