Hi Evgeny, it took me a while to get back to this, since I find it still quite challenging to get my head into this part of the SVN design and needed some uninterrupted time to fully focus on this topic.
On 10/14/2016 18:24, Evgeny Kotkov wrote:
> Stefan Hett <luke1...@apache.org> writes:
>
>> [...]
On the indentation issues (actually related to the code below), I sent a
separate patch-mail to the dev list already earlier: "[PATCH] Correct
indentation in svn_client_conflict_text_resolve_by_id".
>> +
>> + if (option == NULL)
>> + return
>> svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
>> + NULL,
>> + _("Inapplicable conflict resolution option "
>> + "given for conflicted path '%s'"),
> As for the change itself, I don't think that silently transforming one
> conflict option id to another in svn_client_conflict_text_resolve_by_id()
> API is a proper thing to do, because we don't expose this option id as a
> viable resolution option in svn_client_conflict_text_get_resolution_options().
>
> I think that a better way would be to handle this case in the command line
> by using svn_client_conflict_option_working_text for binary files, if
> we run with --accept=working.
With the current design it doesn't seem to be too simple to achieve what
you describe. The approach you describe was the first one I tried during
the hackathon. However, I soon realized that it won't work without
significantly changing the API. That led me to a second approach to
simply extend the API of
svn_client_conflict_text_get_resolution_options() to query the
appropriate option directly. Ivan pointed out that this is not good (and
I agree there with him). Therefore, stsp suggested to go with the third
approach (which is the approach presented in r1764902).
To explain the situation in more detail, as far as I understand it
(since I'm still learning the SVN source I might be wrong, so please
correct me, if you see any mistake here):
The entry point for the resolve-command is in resolve-cmd.c:
svn_cl__resolve().
This then passes on the call to svn_cl__walk_conflicts() to walk all
conflicts. The specified accept-option is kept in the opt_state.
svn_cl__walk_conflicts() translates the accept-option to a corresponding
option_id (svn_client_conflict_option_merged_text in this case).
Since up to that point there is no "binary" or "text" conflict state,
you cannot determine which resolution option is the "correct" one to go
with... The current design relies on one option being given for any type
of conflict which can be applied to either type.
The option_id is then being passed to walk_conflicts() where
svn_wc_walk_stats() transitions the call to the WC layer with the
callback function being set to conflict_status_walker(). In that
function svn_cl__resolve_conflict() is called to resolve an actual
conflict based on the already specified option_id.
Only at the point of the data flow you then can determine whether the
given (accept-)option is applied onto a "binary" or a "text" conflict
and that's where the current chosen solution will first determine an
appropriate conflict option based on the retrieved text-resolution
options or (if none exists) will attempt to retrieve a conflict option
for the semantically equal
svn_client_conflict_option_working_text-option (in case of a binary
conflict).
So the options one could think of would be to extend the API and allow
two conflict options to be passed to svn_cl__resolve_conflict() rather
than only a single one (aka: one for plain-text and one for binary-text
conflicts). But this is where I concluded that the complexity of such an
approach is not preferable over one of the other alternatives.
Or did I miss your point and you had something else in mind?
Regards,
Stefan
smime.p7s
Description: S/MIME Cryptographic Signature
