Digging up an old thread:
On Tue, Nov 12, 2013 at 08:20:37PM -0800, Ben Reser wrote:
> 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.
I'm not just not trying to catch one particular problem case.
I think it doesn't matter what error the external merge tool is raising.
Fact is, it most likely didn't produce a useful result, and another fact
is that we don't know why. I would guess that in practice it will be
due to misconfigured merge-tool settings or a broken wrapper script,
most of the time. But the reason is unknown.
If we're going to change the meaning of 'm' from "run the internal
merge tool" to "run the external merge tool, and if it fails, run
the internal merge tool" I see no better way of dealing with this.
Of course, we could additionally print a line such as:
"External merge tool failed, falling back to internal merge tool"
to inform the user.
If this is not acceptable, then I'd rather go back to the original
meaning of 'm'.
> 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
> )
The fact that there is no clearly defined exit-code interface for
external merge tools is a related problem. It's not the first time
this is biting us. The idea of having a way for the external merge
tool telling svn to fall back to the internal tool is not new.
Generally, if users configure an external merge tool, we currently
have to assume that they want to do all their merges with it because
Subversion doesn't yet support automated per-file-type merge tools.
So if the external tool fails, we can either offer no merge at all,
or fall back to the internal merge.
I'll note that 1.8 prints errors suggesting just that:
_("No merge tool found, "
"try '(m) merge' instead.\n")));
_("Error running merge tool, "
"try '(m) merge' instead.")));
> 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 don't think that's confusing at all. The existing behaviour of 'l'
is retained. What is confusing about that? Are you saying the existing
behaviour of 'l' was already confusing? Or did it just become confusing
when the meaning of 'm' was changed? The way I see it, the new behaviour
of 'm' should obsolete 'l', if anything.
> 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 2014-03-05 13:12:14 CET