You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2013/09/17 19:58:58 UTC
svn commit: r1524145 - /subversion/trunk/subversion/svn/conflict-callbacks.c
Author: stsp
Date: Tue Sep 17 17:58:58 2013
New Revision: 1524145
URL: http://svn.apache.org/r1524145
Log:
In the interactive conflcit 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.
Modified:
subversion/trunk/subversion/svn/conflict-callbacks.c
Modified: subversion/trunk/subversion/svn/conflict-callbacks.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/conflict-callbacks.c?rev=1524145&r1=1524144&r2=1524145&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/conflict-callbacks.c (original)
+++ subversion/trunk/subversion/svn/conflict-callbacks.c Tue Sep 17 17:58:58 2013
@@ -399,31 +399,11 @@ launch_resolver(svn_boolean_t *performed
svn_cl__interactive_conflict_baton_t *b,
apr_pool_t *pool)
{
- svn_error_t *err;
-
- err = svn_cl__merge_file_externally(desc->base_abspath, desc->their_abspath,
- desc->my_abspath, desc->merged_file,
- desc->local_abspath, b->config, NULL,
- pool);
- if (err && err->apr_err == SVN_ERR_CL_NO_EXTERNAL_MERGE_TOOL)
- {
- SVN_ERR(svn_cmdline_fprintf(stderr, pool, "%s\n",
- err->message ? err->message :
- _("No merge tool found, "
- "try '(m) merge' instead.\n")));
- svn_error_clear(err);
- }
- else if (err && err->apr_err == SVN_ERR_EXTERNAL_PROGRAM)
- {
- SVN_ERR(svn_cmdline_fprintf(stderr, pool, "%s\n",
- err->message ? err->message :
- _("Error running merge tool, "
- "try '(m) merge' instead.")));
- svn_error_clear(err);
- }
- else if (err)
- return svn_error_trace(err);
- else if (performed_edit)
+ SVN_ERR(svn_cl__merge_file_externally(desc->base_abspath, desc->their_abspath,
+ desc->my_abspath, desc->merged_file,
+ desc->local_abspath, b->config, NULL,
+ pool));
+ if (performed_edit)
*performed_edit = TRUE;
return SVN_NO_ERROR;
@@ -474,8 +454,8 @@ static const resolver_option_t text_conf
"(same) [theirs-full]"),
svn_wc_conflict_choose_theirs_full },
{ "", "", "", svn_wc_conflict_choose_unspecified },
- { "m", N_("merge"), N_("use internal merge tool to resolve "
- "conflict"), -1 },
+ { "m", N_("merge"), N_("use merge tool to resolve conflict"),
+ -1 },
{ "l", N_("launch tool"), N_("launch external tool to resolve "
"conflict [launch]"), -1 },
{ "p", N_("postpone"), N_("mark the conflict to be resolved later"
@@ -835,20 +815,35 @@ handle_text_conflict(svn_wc_conflict_res
else if (strcmp(opt->code, "m") == 0 || strcmp(opt->code, ":-g") == 0 ||
strcmp(opt->code, "=>-") == 0 || strcmp(opt->code, ":>.") == 0)
{
- if (desc->kind != svn_wc_conflict_kind_text)
+ svn_boolean_t remains_in_conflict;
+ svn_error_t *err;
+
+ err = launch_resolver(&performed_edit, desc, b, iterpool);
+ if (err)
{
- SVN_ERR(svn_cmdline_fprintf(stderr, iterpool,
- _("Invalid option; can only "
- "resolve text conflicts with "
- "the internal merge tool."
- "\n\n")));
- continue;
+ if (err->apr_err == SVN_ERR_CL_NO_EXTERNAL_MERGE_TOOL ||
+ err->apr_err == SVN_ERR_EXTERNAL_PROGRAM)
+ {
+ /* Try the internal merge tool. */
+ svn_error_clear(err);
+ }
+ else
+ return svn_error_trace(err);
}
- if (desc->base_abspath && desc->their_abspath &&
+ if (!performed_edit &&
+ desc->base_abspath && desc->their_abspath &&
desc->my_abspath && desc->merged_file)
{
- svn_boolean_t remains_in_conflict;
+ if (desc->kind != svn_wc_conflict_kind_text)
+ {
+ SVN_ERR(svn_cmdline_fprintf(stderr, iterpool,
+ _("Invalid option; can only "
+ "resolve text conflicts with "
+ "the internal merge tool."
+ "\n\n")));
+ continue;
+ }
SVN_ERR(svn_cl__merge_file(desc->base_abspath,
desc->their_abspath,
@@ -860,11 +855,13 @@ handle_text_conflict(svn_wc_conflict_res
b->config,
&remains_in_conflict,
iterpool));
- knows_something = !remains_in_conflict;
}
else
SVN_ERR(svn_cmdline_fprintf(stderr, iterpool,
_("Invalid option.\n\n")));
+
+ if (performed_edit || !remains_in_conflict)
+ knows_something = TRUE;
}
else if (strcmp(opt->code, "l") == 0 || strcmp(opt->code, ":-l") == 0)
{
@@ -875,7 +872,28 @@ handle_text_conflict(svn_wc_conflict_res
if (desc->base_abspath && desc->their_abspath &&
desc->my_abspath && desc->merged_file)
{
- SVN_ERR(launch_resolver(&performed_edit, desc, b, iterpool));
+ svn_error_t *err;
+
+ err = launch_resolver(&performed_edit, desc, b, iterpool);
+ if (err && err->apr_err == SVN_ERR_CL_NO_EXTERNAL_MERGE_TOOL)
+ {
+ SVN_ERR(svn_cmdline_fprintf(stderr, iterpool, "%s\n",
+ err->message ? err->message :
+ _("No merge tool found, "
+ "try '(m) merge' instead.\n")));
+ svn_error_clear(err);
+ }
+ else if (err && err->apr_err == SVN_ERR_EXTERNAL_PROGRAM)
+ {
+ SVN_ERR(svn_cmdline_fprintf(stderr, iterpool, "%s\n",
+ err->message ? err->message :
+ _("Error running merge tool, "
+ "try '(m) merge' instead.")));
+ svn_error_clear(err);
+ }
+ else if (err)
+ return svn_error_trace(err);
+
if (performed_edit)
knows_something = TRUE;
}
Re: external merge tool with 'm' at conflict prompt
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 05, 2014 at 03:04:29PM -0800, Ben Reser wrote:
> 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
I've implemented this today (see r1589707 and follow-ups).
Does it look good to you now?
Thanks!
Re: external merge tool with 'm' at conflict prompt
Posted by Ben Reser <be...@reser.org>.
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.
external merge tool with 'm' at conflict prompt (was: Re: svn
commit: r1524145 - /subversion/trunk/subversion/svn/conflict-callbacks.c)
Posted by Stefan Sperling <st...@elego.de>.
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@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.
Re: svn commit: r1524145 - /subversion/trunk/subversion/svn/conflict-callbacks.c
Posted by Ben Reser <be...@reser.org>.
On 11/13/13 12:06 AM, Stefan Sperling wrote:
> Fair enough. Can you please add your rationale to the issue as well
> so the original reporter will see it?
>
> While at it, please reopen the issue and set the milestone to 1.9.x.
> I'll try to get it this done then and also address the issues you've
> raised. I agree the error handling seems flaky.
>
> I don't think the comment about exit codes you cited was added by me.
> I'll have to investigate what was intended there.
I was less than clear on where my veto applies. I'm -1 on the backport with
the specific issues I raised. I'm like -0 on the backport if those issues are
resolved. If other people think changing the behavior of those commands is ok
in 1.8.x I'm not going to stand in the way.
More than anything the error handling just doesn't make any sense to me.
Re: svn commit: r1524145 -
/subversion/trunk/subversion/svn/conflict-callbacks.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Nov 12, 2013 at 08:20:37PM -0800, Ben Reser wrote:
> 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.
Fair enough. Can you please add your rationale to the issue as well
so the original reporter will see it?
While at it, please reopen the issue and set the milestone to 1.9.x.
I'll try to get it this done then and also address the issues you've
raised. I agree the error handling seems flaky.
I don't think the comment about exit codes you cited was added by me.
I'll have to investigate what was intended there.
Thanks!
Re: svn commit: r1524145 - /subversion/trunk/subversion/svn/conflict-callbacks.c
Posted by Ben Reser <be...@reser.org>.
On 9/17/13 10:58 AM, stsp@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.