On 9/17/13 10:58 AM, stsp_at_apache.org wrote:
> Author: stsp
> Date: Tue Sep 17 17:58:58 2013
> New Revision: 1524145
>
> URL: http://svn.apache.org/r1524145
> Log:
> In the interactive confictt prompt, make the 'merge' option try the external
> merge tool first, and fall back to the internal merge tool if the external
> tool fails.
>
> Avoids confusion if users configure an external merge tool and then
> attempt to use the 'm' option to launch that tool (instead of using
> the 'l' option).
>
>
> * subversion/svn/conflict-callbacks.c
> (launch_resolver): Return any error from svn_cl__merge_file_externally(),
> instead of suggesting the 'm' option on error.
> (text_conflict_options): Tweak help string for 'm' option accordingly.
> (handle_text_conflict): Try external merge tool first if users picks 'm',
> fall back to internal merge tool if the external tool fails to run.
> Suggest the internal merge tool ('m' option) if the user picks 'l' but
> the external tool fails to run.
I understand the user confusion but I think you're just creating more problems.
As the resolver exists right now on trunk is confusing in my opinion. If you
choose the 'm' option it tries the external merge tool and if there is no
external tool configured or the configured tool fails then it proceeds to the
internal tool.
I take two issues with this:
1) It removes the ability to choose the internal tool. There may be very good
reasons to not want to use your configured merge tool. Perhaps I know it won't
handle this merge well. This could be resolved by adding another option to be
able to choose the internal merge tool.
2) If the merge tool exits with an error it proceeds to try the internal merge
tool. Specifically it looks for SVN_ERR_EXTERNAL_PROGRAM. Which is returned
whenever the exit code for the external merge tool is anything other than 0 or
1. I'm not sure how it's sane to proceed in that case with the internal merge
tool. The code for svn_cl__merge_file_externally has this comment above the
test to return SVN_ERR_EXTERNAL_PROGRAM:
[[[
/* Exit code 0 means the merge was successful.
* Exit code 1 means the file was left in conflict but it
* is OK to continue with the merge.
* Any other exit code means there was a real problem. */
]]]
I'm guessing you're trying to catch if the command is configured but fails to
run. But that's not what the code does at all since the error is only sent if
the command does run and exits with an error. Maybe there should be some error
code a external merge tool should return if it wants to decline to be used for
the specific merge, but I don't think we have anything like that now.
The SVN Book makes things even more confused because it says:
[[[
The merge tool is expected to return an error code of 0 to indicate success,
or 1 to indicate failure.
]]]
(from
http://svnbook.red-bean.com/en/1.8/svn.advanced.externaldifftools.html#svn.advanced.externaldifftools.merge
)
To further confuse things, using the 'l' option retains the existing behavior
of failing out of the merge with an error in these cases.
I'm not sure what we should be doing here but this much I can say this change
shouldn't be backported. At least not yet. Though I'm really not sure how I
feel about even backporting this to 1.8 since it pretty dramatically changes
the interface's behavior in ways that may be surprising to users.
As such I'm voting -1 on this backport on the 1.8.x change.
Received on 2013-11-13 05:21:19 CET