On 3/5/14, 4:11 AM, Stefan Sperling wrote:
> Digging up an old thread:
Thanks for revisiting this.
> 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.
1) We're giving the merge tool paths to files that contain potentially
unversioned and locally modified content. Content that can't be replaced if it
does something wrong. Technically it should only touch the output file and if
it fails we can just run the merge again from our internal tool, but that seems
like a huge assumption to make with unversioned data. Maybe I'm being overly
paranoid here.
2) If I had a external merge tool that was failing, I'd probably want to stop
and figure out why. I'm probably using that tool because I prefer it to the
internal merge. I don't think it's a valid assumption that just because my
external tool is currently failing that I want Subversion to just go ahead and
use the internal tool. If we print an error and wait for the user to decide
what to do they could potentially leave the interactive merge running while
they fix their merge tool (at least in some cases). Of course if the wrong
tool is configured they might not be able to fix it without stopping the merge.
But running the internal merge tool seems like the wrong option because then
they'll have to revert the merge (which is difficult for some users) and try again.
> 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'.
Printing a message doesn't do anything to resolve my concerns.
I'd rather we do the following:
m: run the external merge tool if configured or the internal merge tool if not.
If the external merge tool fails, print an error and let the user decide what
to do.
l: run the external merge tool (maybe make this option prompt if you want to
use the configured one or provide one, but that's probbaly getting out of scope)
i: run the internal merge tool
> 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.
I'd be satisfied if we defined an exit code and documented it clearly (even if
that's different than the interface now). E.G. saying exit 42 means the merge
tool is saying it defers and to use the internal merge tool and then only
having the automatic behavior in this case.
People that aren't sure what exit codes their merge tool uses can simply write
a wrapper around it that never exits with 42 (or whatever number we choose).
People that want to always automatically fall back on the internal merge tool
can do so as well with a wrapper script.
> 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.
Is that part of your intent here? To allow people to make their merge tools
fail on file types they don't want to do the merge with and automatically use
the internal tool? If so I'd much rather we just provide a specified exit code
interface for this than making it global for everyone.
> 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.")));
See above, you've left out the potential opportunity for the user to fix their
external merge tool.
> 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.
There's two different implementations of running the local merge tool and
they're in conflict (though maybe that's ok).
conflict_func_interactive has a switch statement that handles the
--accept=launch option, which fails out of the merge on any error from the
merge tool.
Further on down in conflict_func_interactive it calls handle_text_conflict
which prompts the user. If the user says to run the local merge tool it then
runs the launch_resolver function which spints out the error messages you
mentioned above and clears the error. Thus the user is prompted again and can
choose a different option.
I suspect when I wrote the original message I confused the two implementations
and that that the --accept=launch behavior was what you get when you choose l
from the interactive menu.
The difference here makes sense but if choosing "l" had failed out of the
interactive merge in case of an error form the merge tool, while choosing "m"
in the same conditions simply prompted you again, that would be problematic to me.
Received on 2014-03-06 00:05:06 CET