You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jonathan Guy <jo...@ikoder.com> on 2018/10/30 18:39:49 UTC
[PATCH] Suppress conflict resolver in dry-run merge
[[[
*subversion/svn/merge-cmd.c
(svn_cl__merge): Suppress the interactive conflict resolver
if a merge has been performed with the dry-run option.
]]]
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 01, 2018 at 05:38:48PM +0100, Branko Čibej wrote:
> On 01.11.2018 17:25, Stefan Sperling wrote:
> > While many SVN operations support a dry-run mode, at present the conflict
> > resolver does not. It might actually be nice to see what the resolver would
> > do while in dry-run mode. A dry-run merge could show the result of successful
> > resolution in cases where a recommended resolution option exists, rather
> > than showing a conflict. For instance, a merge to the new location of a
> > moved item could be shown, instead of showing a conflict at the old location.
>
> Wouldn't that only work if the nature of the conflict was recorded
> somewhere in the working copy? And wouldn't recording it be exactly
> opposite of what 'svn merge --dry-run' really means?
Indeed, the resolver only reads such information from on-disk working
copy state. So this idea would probably not be worth the effort.
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Branko Čibej <br...@apache.org>.
On 01.11.2018 17:25, Stefan Sperling wrote:
> On Thu, Nov 01, 2018 at 10:32:56AM +0000, Jonathan Guy wrote:
>> Ok I looked into this a bit more and I see what's going on now.
>> The post merge conflict resolver runs because the merge operation reported conflicts (via the conflict stats).
>> This calls
>> svn_client_conflict_walk
>> which calls
>> svn_wc_walk_status
>> which reports no conflict at that path because the wc was never changed because it was a dry run. So the whole operation gets dropped here.
>>
>> I’m still convinced the whole thing is a pointless exercise.
>> A dry-run merge will not produce any actual conflicts on-disk so the resolver will never do anything.
>> I guess there could be other reasons I don’t know of to run the resolver so we’ll just leave it as is.
>>
>> Thanks again for your time.
> Thanks for digging into this further.
>
> I think you are raising a good point. The fact that it works as expected
> right now is due to a side-effect. So your proposed change is still worth
> some consideration.
>
> While many SVN operations support a dry-run mode, at present the conflict
> resolver does not. It might actually be nice to see what the resolver would
> do while in dry-run mode. A dry-run merge could show the result of successful
> resolution in cases where a recommended resolution option exists, rather
> than showing a conflict. For instance, a merge to the new location of a
> moved item could be shown, instead of showing a conflict at the old location.
Wouldn't that only work if the nature of the conflict was recorded
somewhere in the working copy? And wouldn't recording it be exactly
opposite of what 'svn merge --dry-run' really means?
On the other hand, we've had requests for 'svn merge --dry-run' to show
potential text conflicts and even launch an external diff or merge tool
to display them. So there's clearly room for these kinds of features.
Maybe a '--moist-run' option is what we need. :)
-- Brane
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 01, 2018 at 05:25:10PM +0100, Stefan Sperling wrote:
> Until the resolver grows such a dry-run mode I think your patch makes sense.
> Do we agree here?
Committed in https://svn.apache.org/r1845557 with small whitespace
adjustments to keep lines below 80 columns in length.
I have also expanded the log message a bit.
Thank you for your contribution, Jonathan!
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 01, 2018 at 10:32:56AM +0000, Jonathan Guy wrote:
> Ok I looked into this a bit more and I see what's going on now.
> The post merge conflict resolver runs because the merge operation reported conflicts (via the conflict stats).
> This calls
> svn_client_conflict_walk
> which calls
> svn_wc_walk_status
> which reports no conflict at that path because the wc was never changed because it was a dry run. So the whole operation gets dropped here.
>
> I’m still convinced the whole thing is a pointless exercise.
> A dry-run merge will not produce any actual conflicts on-disk so the resolver will never do anything.
> I guess there could be other reasons I don’t know of to run the resolver so we’ll just leave it as is.
>
> Thanks again for your time.
Thanks for digging into this further.
I think you are raising a good point. The fact that it works as expected
right now is due to a side-effect. So your proposed change is still worth
some consideration.
While many SVN operations support a dry-run mode, at present the conflict
resolver does not. It might actually be nice to see what the resolver would
do while in dry-run mode. A dry-run merge could show the result of successful
resolution in cases where a recommended resolution option exists, rather
than showing a conflict. For instance, a merge to the new location of a
moved item could be shown, instead of showing a conflict at the old location.
This would require some work in the conflict resolver which would mostly
be trivial; it's just that there's a lot of code in the resolver so
completing even trivial changes in a consistent manner takes time.
Until the resolver grows such a dry-run mode I think your patch makes sense.
Do we agree here?
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Jonathan Guy <jo...@ikoder.com>.
Ok I looked into this a bit more and I see what's going on now.
The post merge conflict resolver runs because the merge operation reported conflicts (via the conflict stats).
This calls
svn_client_conflict_walk
which calls
svn_wc_walk_status
which reports no conflict at that path because the wc was never changed because it was a dry run. So the whole operation gets dropped here.
I’m still convinced the whole thing is a pointless exercise.
A dry-run merge will not produce any actual conflicts on-disk so the resolver will never do anything.
I guess there could be other reasons I don’t know of to run the resolver so we’ll just leave it as is.
Thanks again for your time.
Regards
Jonathan
> On 31 Oct 2018, at 12:35, Stefan Sperling <st...@elego.de> wrote:
>
> On Wed, Oct 31, 2018 at 09:46:23AM +0000, Jonathan Guy wrote:
>> Hi Stephan
>> Thanks for the reply.
>> I did see code section you refer to however the svn_client__resolve_conflicts will only be called if you have ctx->conflict_func2.
>> That function (AFAIK) is only installed if an “accept” option is supplied as the code below
>>
>>
>> /* Install a legacy conflict handler if the --accept option was given.
>> * Else, svn_client_merge5() may abort the merge in an undesirable way.
>> * See the docstring at conflict_func_merge_cmd() for details */
>> if (opt_state->accept_which != svn_cl__accept_unspecified)
>> {
>> struct conflict_func_merge_cmd_baton *b = apr_pcalloc(pool, sizeof(*b));
>>
>> b->accept_which = opt_state->accept_which;
>> SVN_ERR(svn_dirent_get_absolute(&b->path_prefix, "", pool));
>> b->conflict_stats = conflict_stats;
>>
>> ctx->conflict_func2 = conflict_func_merge_cmd;
>> ctx->conflict_baton2 = b;
>> }
>>
>>
>> The resolver that runs post run_merge is however run regardless if there is an accept argument or not.
>> If no accept option is given the user is ultimately prompted to resolve the conflict (dry-run or no dry-run).
>>
>>
>> /* If we are in interactive mode and either the user gave no --accept
>> * option or the option did not apply, then prompt. */
>> if (option_id == svn_client_conflict_option_unspecified)
>> {
>> svn_boolean_t resolved = FALSE;
>> svn_boolean_t postponed = FALSE;
>> svn_boolean_t printed_description = FALSE;
>> svn_error_t *err;
>>
>> *quit = FALSE;
>>
>> while (!resolved && !postponed && !*quit)
>> {
>> err = resolve_conflict_interactively(&resolved, &postponed,
>>
>>
>> Sorry I’m no trying to be argumentative I just need to clear on this one aspect.
>> Thanks for any help.
>
> Can you provide an example? I cannot reproduce the problem like this:
>
> $ svn merge ^/trunk
> --- Merging r2 through r5 into '.':
> C alpha
> A alpha2
> --- Recording mergeinfo for merge of r2 through r5 into '.':
> U .
> Summary of conflicts:
> Tree conflicts: 1
> Searching tree conflict details for 'alpha' in repository:
> Checking r4... done
> Tree conflict on 'alpha':
> File merged from
> '^/trunk/alpha@1'
> to
> '^/trunk/alpha@5'
> was moved to '^/trunk/alpha2' by stsp in r4.
> A file which differs from the corresponding file on the merge source branch was found in the working copy.
> Applying recommended resolution 'Move and merge':
> C alpha2
> Tree conflict at 'alpha' marked as resolved.
> Merge conflict discovered in file 'alpha2'.
> Select: (p) Postpone, (df) Show diff, (e) Edit file, (m) Merge,
> (s) Show all options: q
> Summary of conflicts:
> Text conflicts: 1 remaining (and 0 already resolved)
> Tree conflicts: 0 remaining (and 1 already resolved)
> $ sv revert
> $ pwd
> /tmp/svn-sandbox/branch
> $ svn revert -R .
> Reverted '.'
> Reverted 'alpha'
> Reverted 'alpha2'
> $ svn merge ^/trunk --dry-run
> --- Merging r2 through r5 into '.':
> C alpha
> A alpha2
> Summary of conflicts:
> Tree conflicts: 1
> $
>
>
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Oct 31, 2018 at 09:46:23AM +0000, Jonathan Guy wrote:
> Hi Stephan
> Thanks for the reply.
> I did see code section you refer to however the svn_client__resolve_conflicts will only be called if you have ctx->conflict_func2.
> That function (AFAIK) is only installed if an “accept” option is supplied as the code below
>
>
> /* Install a legacy conflict handler if the --accept option was given.
> * Else, svn_client_merge5() may abort the merge in an undesirable way.
> * See the docstring at conflict_func_merge_cmd() for details */
> if (opt_state->accept_which != svn_cl__accept_unspecified)
> {
> struct conflict_func_merge_cmd_baton *b = apr_pcalloc(pool, sizeof(*b));
>
> b->accept_which = opt_state->accept_which;
> SVN_ERR(svn_dirent_get_absolute(&b->path_prefix, "", pool));
> b->conflict_stats = conflict_stats;
>
> ctx->conflict_func2 = conflict_func_merge_cmd;
> ctx->conflict_baton2 = b;
> }
>
>
> The resolver that runs post run_merge is however run regardless if there is an accept argument or not.
> If no accept option is given the user is ultimately prompted to resolve the conflict (dry-run or no dry-run).
>
>
> /* If we are in interactive mode and either the user gave no --accept
> * option or the option did not apply, then prompt. */
> if (option_id == svn_client_conflict_option_unspecified)
> {
> svn_boolean_t resolved = FALSE;
> svn_boolean_t postponed = FALSE;
> svn_boolean_t printed_description = FALSE;
> svn_error_t *err;
>
> *quit = FALSE;
>
> while (!resolved && !postponed && !*quit)
> {
> err = resolve_conflict_interactively(&resolved, &postponed,
>
>
> Sorry I’m no trying to be argumentative I just need to clear on this one aspect.
> Thanks for any help.
Can you provide an example? I cannot reproduce the problem like this:
$ svn merge ^/trunk
--- Merging r2 through r5 into '.':
C alpha
A alpha2
--- Recording mergeinfo for merge of r2 through r5 into '.':
U .
Summary of conflicts:
Tree conflicts: 1
Searching tree conflict details for 'alpha' in repository:
Checking r4... done
Tree conflict on 'alpha':
File merged from
'^/trunk/alpha@1'
to
'^/trunk/alpha@5'
was moved to '^/trunk/alpha2' by stsp in r4.
A file which differs from the corresponding file on the merge source branch was found in the working copy.
Applying recommended resolution 'Move and merge':
C alpha2
Tree conflict at 'alpha' marked as resolved.
Merge conflict discovered in file 'alpha2'.
Select: (p) Postpone, (df) Show diff, (e) Edit file, (m) Merge,
(s) Show all options: q
Summary of conflicts:
Text conflicts: 1 remaining (and 0 already resolved)
Tree conflicts: 0 remaining (and 1 already resolved)
$ sv revert
$ pwd
/tmp/svn-sandbox/branch
$ svn revert -R .
Reverted '.'
Reverted 'alpha'
Reverted 'alpha2'
$ svn merge ^/trunk --dry-run
--- Merging r2 through r5 into '.':
C alpha
A alpha2
Summary of conflicts:
Tree conflicts: 1
$
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Jonathan Guy <jo...@ikoder.com>.
Hi Stephan
Thanks for the reply.
I did see code section you refer to however the svn_client__resolve_conflicts will only be called if you have ctx->conflict_func2.
That function (AFAIK) is only installed if an “accept” option is supplied as the code below
/* Install a legacy conflict handler if the --accept option was given.
* Else, svn_client_merge5() may abort the merge in an undesirable way.
* See the docstring at conflict_func_merge_cmd() for details */
if (opt_state->accept_which != svn_cl__accept_unspecified)
{
struct conflict_func_merge_cmd_baton *b = apr_pcalloc(pool, sizeof(*b));
b->accept_which = opt_state->accept_which;
SVN_ERR(svn_dirent_get_absolute(&b->path_prefix, "", pool));
b->conflict_stats = conflict_stats;
ctx->conflict_func2 = conflict_func_merge_cmd;
ctx->conflict_baton2 = b;
}
The resolver that runs post run_merge is however run regardless if there is an accept argument or not.
If no accept option is given the user is ultimately prompted to resolve the conflict (dry-run or no dry-run).
/* If we are in interactive mode and either the user gave no --accept
* option or the option did not apply, then prompt. */
if (option_id == svn_client_conflict_option_unspecified)
{
svn_boolean_t resolved = FALSE;
svn_boolean_t postponed = FALSE;
svn_boolean_t printed_description = FALSE;
svn_error_t *err;
*quit = FALSE;
while (!resolved && !postponed && !*quit)
{
err = resolve_conflict_interactively(&resolved, &postponed,
Sorry I’m no trying to be argumentative I just need to clear on this one aspect.
Thanks for any help.
Regards
Jonathan
> On 31 Oct 2018, at 08:32, Stefan Sperling <st...@elego.de> wrote:
>
> On Tue, Oct 30, 2018 at 06:39:49PM +0000, Jonathan Guy wrote:
>> [[[
>> *subversion/svn/merge-cmd.c
>> (svn_cl__merge): Suppress the interactive conflict resolver
>> if a merge has been performed with the dry-run option.
>> ]]]
>
> Hi Jonathan,
>
> Thank your for the patch. I am afraid I misled you earlier by suggesting
> that you send a patch for the issue. This is in fact not a problem in
> the current implementation because the dry_run flag gets passed down
> into libsvn_client, where it takes part in the decision about running
> the conflict resolver.
>
> Specifically, this part of the do_merge() function in the file
> subversion/libsvn_client/merge.c checks the flag:
>
> /* Give the conflict resolver callback the opportunity to
> * resolve any conflicts that were raised. If it resolves all
> * of them, go around again to merge the next sub-range (if any). */
> if (conflicted_range_report && ctx->conflict_func2 && ! dry_run)
> {
> svn_boolean_t conflicts_remain;
>
> SVN_ERR(svn_client__resolve_conflicts(
> &conflicts_remain, merge_cmd_baton.conflicted_paths,
> ctx, iterpool));
> if (conflicts_remain)
> break;
>
> So your patch is redundant and we don't need to apply it.
> I should have tested the current behaviour before recommending that
> you send a patch.
>
> Regardless, thank you for your contribution!
>
> Stefan
Re: [PATCH] Suppress conflict resolver in dry-run merge
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 30, 2018 at 06:39:49PM +0000, Jonathan Guy wrote:
> [[[
> *subversion/svn/merge-cmd.c
> (svn_cl__merge): Suppress the interactive conflict resolver
> if a merge has been performed with the dry-run option.
> ]]]
Hi Jonathan,
Thank your for the patch. I am afraid I misled you earlier by suggesting
that you send a patch for the issue. This is in fact not a problem in
the current implementation because the dry_run flag gets passed down
into libsvn_client, where it takes part in the decision about running
the conflict resolver.
Specifically, this part of the do_merge() function in the file
subversion/libsvn_client/merge.c checks the flag:
/* Give the conflict resolver callback the opportunity to
* resolve any conflicts that were raised. If it resolves all
* of them, go around again to merge the next sub-range (if any). */
if (conflicted_range_report && ctx->conflict_func2 && ! dry_run)
{
svn_boolean_t conflicts_remain;
SVN_ERR(svn_client__resolve_conflicts(
&conflicts_remain, merge_cmd_baton.conflicted_paths,
ctx, iterpool));
if (conflicts_remain)
break;
So your patch is redundant and we don't need to apply it.
I should have tested the current behaviour before recommending that
you send a patch.
Regardless, thank you for your contribution!
Stefan