Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)

"Jeremy Whitlock" <jcscoobyrs@gmail.com> writes:

> [[[
> Add option to resolve conflicts by selecting a specific file by adding
> support to the svn executable to handle the "--accept" flag. The
> "resovled" subcommand is the only subcommand using this new flag as
   ^^^^^^^^

resolved

> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 25291)
> +++ subversion/include/svn_types.h (working copy)
> @@ -199,6 +199,34 @@
> svn_recursive
> };
>
> +/** The concept of automatic conflict resolution.
> + *
> + * @since New in 1.5
> + */
> +typedef enum
> +{

I think you want 'svn_accept_invalid = -1' here; see below.

> + /* Resolve the conflict as usual */
> + svn_accept_default,
> +
> + /* Resolve the conflict with the pre-update base file */
> + svn_accept_orig,
> +
> + /* Resolve the conflict with the pre-update working copy file */
> + svn_accept_mine,
> +
> + /* Resolve the conflict with the during-update repository file, new base */
> + svn_accept_base,
> +
> +} svn_accept_t;

Unfortunately, i don't think we've finished painting this
particular bikeshed yet ;->. I finally went and got myself a
conflict so i could see if we could take name inspiration from
there. So, how about svn_accept_{old,new,work} with strings
"left", "new", and "working"?

> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 25291)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -2474,6 +2474,23 @@
> void *cancel_baton,
> apr_pool_t *pool);
>
> +/**
> + * Just like "" but with the ability to pass an svn_accept_t argument
> + * for automatical conflict resolution handling.
> + *
> + * @since New in 1.5

You have this backwards. Move the svn_wc_resolved_conflict2
docstring here and document the new parameter. Then add a
"similar to"/deprecated comment for the old one; see
svn_wc_adm_open3 for example. svn_client_resolved2, too.

> Index: subversion/libsvn_wc/adm_ops.c

> @@ -2519,6 +2524,25 @@
> svn_boolean_t was_present, need_feedback = FALSE;
> apr_uint64_t modify_flags = 0;
> svn_wc_entry_t *entry = svn_wc_entry_dup(orig_entry, pool);
> +
> + /* Handle automatic conflict resolution before the temporary files are
> + * deleted. */
> + if (accept != svn_accept_default)
> + {
> + const char *base_name;
> +
> + if (accept == svn_accept_orig) {
> + base_name = entry->conflict_old;
> + } else if (accept == svn_accept_mine) {
> + base_name = entry->conflict_wrk;
> + } else if (accept == svn_accept_base) {
> + base_name = entry->conflict_new;
> + } /* Should never be anything else thanks to the svn_accept_from_word() */

Not all callers use svn_accept_from_word; maybe just say "Ignore
invalid values". If someone passes some random value instead of
one of the svn_accept_ values, that's his problem.

> @@ -2594,6 +2618,8 @@
> svn_boolean_t resolve_text;
> /* TRUE if property conflicts are to be resolved. */
> svn_boolean_t resolve_props;
> + /* Placeholder for the type of automatic conflict resolution to perform */
> + svn_accept_t accept;

Drop "Placeholder for the".

> Index: subversion/libsvn_subr/kitchensink.c
> ===================================================================
> --- subversion/libsvn_subr/kitchensink.c (revision 25291)
> +++ subversion/libsvn_subr/kitchensink.c (working copy)
> @@ -32,6 +32,18 @@
> return uuid_str;
> }
>
> +svn_accept_t
> +svn_accept_from_word(const char *word)
> +{
> + if (strcmp(word, "orig") == 0)
> + return svn_accept_orig;
> + if (strcmp(word, "mine") == 0)
> + return svn_accept_mine;
> + if (strcmp(word, "base") == 0)
> + return svn_accept_base;
> + /* Return default, which means no automatic conflict resolution */
> + return svn_accept_default;

This comment is wrong, as the only svn_accept_from_word caller
treats svn_accept_default as an error.

> Index: subversion/svn/main.c

> @@ -1381,6 +1385,21 @@
> case 'g':
> opt_state.use_merge_history = TRUE;
> break;
> + case svn_cl__accept_opt:
> + opt_state.accept = svn_accept_from_word(opt_arg);
> +
> + /* We need to make sure that the value passed to the accept flag
> + * was one of the three expected. Since default is what gets
> + * set when one of the three expected are not passed, checking for
> + * this as part of the command line parsing makes sense. */
> + if (opt_state.accept == svn_accept_default)
> + {
> + return svn_cmdline_handle_exit_error
> + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'%s' is not a valid accept value; try "
> + "'orig', 'mine', or 'base'"),
> + opt_arg), pool, "svn: ");
> + }

You've conflated default behavior with malformed input. I
preferred how you were using svn_accept_unknown for this earlier
(though i think svn_accept_invalid is a better name).

-- 
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org