Re: [PATCH] Suppress conflict resolver in dry-run merge
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
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
On 01.11.2018 17:25, Stefan Sperling wrote: > On Thu, Nov 01, 2018 at 10:32:56AM +, 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
On Thu, Nov 01, 2018 at 10:32:56AM +, 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
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 wrote: > > On Wed, Oct 31, 2018 at 09:46:23AM +, 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(>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(, , >> >> >> 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 > Aalpha2 > --- 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': > Calpha2 > 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 > Aalpha2 > Summary of conflicts: > Tree conflicts: 1 > $ > >
Re: [PATCH] Suppress conflict resolver in dry-run merge
On Wed, Oct 31, 2018 at 09:46:23AM +, 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(>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(, , > > > 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 Aalpha2 --- 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': Calpha2 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 Aalpha2 Summary of conflicts: Tree conflicts: 1 $
Re: [PATCH] Suppress conflict resolver in dry-run merge
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(>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(, , 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 wrote: > > On Tue, Oct 30, 2018 at 06:39:49PM +, 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( >_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
On Tue, Oct 30, 2018 at 06:39:49PM +, 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( _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
[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. ]]] merge-cmd.c.patch Description: Binary data