Hi Stefan.
I'm just about starting to get the idea that I think you're talking about, but
I'm still having a hard time seeing how a state machine in the library would
need more than one state transition for any chosen resolution, or why it would
need to control information-display requests.
I think it would help if we could see how these ideas work, step by step, for
some non-trivial use case.
Stefan Sperling wrote:
> 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_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.
You agree that those options shouldn't be found outside a 3-way merge tool
that's displaying the conflicts in question? So what are they doing in this
list?
>>> [...]
>>> 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.
OK, I'm glad to learn this was partly me misunderstanding your intentions.
> 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.
Perhaps you could give an example of one part of this state machine, that's
sufficiently complex that a state machine is a useful way of achieving it.
What's an example of when an information-display request such as "show me a
diff between base and theirs" needs to be tied into the state machine?
> 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?
Yes.
> Or should we restrict clients to handle one conflict at
> a time?
No.
> This restriction might make it easier to implement the resolver.
> We could also make this restriction initially and lift it later.
You can only lift such a restriction if the initial architecture was designed
for parallel resolving.
>> To enable this, the application needs access to the metadata
>> [...] 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 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.[...]
>
> 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?
Well, I'm not convinced yet.
- Julian
