Re: [PATCH] Make --accept match conflict handling prompt

On Fri, 28 Sep 2007, Eric Gillespie wrote:

> As part of this change, I renamed a couple identifiers to say
> "their" instead of "repos", because both "repos" and "base" are
> from the repository; the name "repos" is not clear at all:
...
> This also happens to match the UI for the interactive conflict
> resolution prompt, and for --accept. This is also a good thing,
> I think. I asked Sussman about it in IRC, and he said he was
> fine with the change. I'd like to go for more consistency, and
> rename the user identifiers as well:
...
> What do people think?

+1

I'm attaching some JavaHL changes which will need to go along with
this patch.

A few comments inline below...

...
> --- subversion/include/svn_wc.h (revision 26821)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1007,7 +1007,8 @@
> * they default to NULL.) */
>
> const char *base_file; /* common ancestor of the two files being merged */
> - const char *repos_file; /* repository's version of the file */
> + const char *their_file; /* their version of the file */
> + /* XXX I'd also prefer "my" here and "mine" below; why be inconsistent? */

+1

> const char *user_file; /* user's locally-edited version of the file */
> const char *merged_file; /* merged version of file; has conflict markers */
...
> --- subversion/svn/cl.h (revision 26821)
> +++ subversion/svn/cl.h (working copy)
...
> +typedef struct {
> + svn_cl__accept_t accept_which;
> + apr_hash_t *config;
> + const char *editor_cmd;
> + svn_boolean_t external_failed;
> + svn_boolean_t interactive;
> +} svn_cl__conflict_baton_t;
> +
> +/* Return address of newly allocated and initialized svn_cl__conflict_baton_t.
> + * @since New in 1.5.
> + */
> +svn_cl__conflict_baton_t *
> +svn_cl__conflict_baton_new(svn_cl__accept_t accept_which,
> + apr_hash_t *config,
> + const char *editor_cmd,
> + apr_pool_t *pool);

Subversion uses the verbs "create" or "make" more often than the word
"new" in constructor function names.

> +/* Return svn_cl__accept_t value corresponding to @ word.
> + * @since New in 1.5.
> + */
> +svn_cl__accept_t
> +svn_cl__accept_from_word(const char *word);

While I see value in having the "since what version" info in the doc
strings, these particular doc strings don't need to use Doxygen
markup.

> /* A mindless implementation of svn_wc_conflict_resolver_func_t that
> * does absolutely nothing to resolve conflicts. */
> svn_error_t *
> @@ -260,7 +311,7 @@
> one of the 3 fulltexts, edit the merged file on the spot, or just
> skip the conflict (to be resolved later). */

...and this (new) one is missing such markup.

> svn_error_t *
> -svn_cl__interactive_conflict_handler(svn_wc_conflict_result_t *result,
> +svn_cl__conflict_handler(svn_wc_conflict_result_t *result,
> const svn_wc_conflict_description_t *desc,
> void *baton,
> apr_pool_t *pool);

Level of indentation is off due to (good) function rename.

...
> --- subversion/svn/conflict-callbacks.c (revision 26821)
> +++ subversion/svn/conflict-callbacks.c (working copy)
...
> +svn_cl__accept_t
> +svn_cl__accept_from_word(const char *word)
> +{
> + if (strcmp(word, "postpone") == 0)
> + return svn_cl__accept_postpone;
> + if (strcmp(word, "base") == 0)
> + return svn_cl__accept_base;
> + if (strcmp(word, "mine") == 0)
> + return svn_cl__accept_mine;
> + if (strcmp(word, "theirs") == 0)
> + return svn_cl__accept_theirs;
> + if (strcmp(word, "edit") == 0)
> + return svn_cl__accept_edit;
> + if (strcmp(word, "launch") == 0)
> + return svn_cl__accept_launch;
> + /* word is an invalid action. */
> + return svn_cl__accept_invalid;
> +}

These magic strings are also used in the command-line help. Probably
want to share them via #define's.

> /* Utility to print a full description of the conflict. */
> static svn_error_t *
> print_conflict_description(const svn_wc_conflict_description_t *desc,
> @@ -104,9 +138,9 @@
> if (desc->base_file)
> SVN_ERR(svn_cmdline_printf(pool, _(" Ancestor file: %s\n"),
> desc->base_file));
> - if (desc->repos_file)
> - SVN_ERR(svn_cmdline_printf(pool, _(" Repository's file: %s\n"),
> - desc->repos_file));
> + if (desc->their_file)
> + SVN_ERR(svn_cmdline_printf(pool, _(" Their file: %s\n"),
> + desc->their_file));
> if (desc->user_file)
> SVN_ERR(svn_cmdline_printf(pool, _(" User's file: %s\n"),
> desc->user_file));

Here's another spot where you might want "my".

...
> +svn_cl__conflict_handler(svn_wc_conflict_result_t *result,
> + const svn_wc_conflict_description_t *desc,
> + void *baton,
> + apr_pool_t *pool)
> {

The changes to this routine are probably going to hork Augie F's patch.

...
> --- subversion/svn/main.c (revision 26821)
> +++ subversion/svn/main.c (working copy)
> @@ -211,9 +211,10 @@
> " "
> "history")},
> {"accept", svn_cl__accept_opt, 1,
> - N_("specify automatic conflict resolution source\n"
> + N_("specify automatic conflict resolution action\n"
> " "
> - "('left', 'right', or 'working')")},
> + /* TODO: 'launch' */
> + "('postpone', 'base', 'mine', 'theirs', 'edit')")},

Here's the help text I referred to above, where we'd want to use those
#defines.

...
> {svn_cl__targets_opt, 'R', svn_cl__depth_opt, 'q',
> - svn_cl__config_dir_opt, svn_cl__accept_opt} },
> + svn_cl__config_dir_opt, svn_cl__accept_opt},
> + {{svn_cl__accept_opt, N_("specify automatic conflict resolution source\n"
> + " "
> + /* TODO: 'launch' */
> + "('base', 'mine', 'theirs')")}} },

And again, here.

...
> + /* TODO: launch */
> + if (opt_state.accept_which == svn_cl__accept_launch)
> + return svn_cmdline_handle_exit_error
> + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("--accept=launch not yet implemented")),
> + pool, "svn: ");

And here.

...
> + if (opt_state.non_interactive)
> + {
> + if (opt_state.accept_which == svn_cl__accept_edit)
> + return svn_cmdline_handle_exit_error
> + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("--accept=edit incompatible with"
> + " --non-interactive")),
> + pool, "svn: ");
> + if (opt_state.accept_which == svn_cl__accept_launch)
> + return svn_cmdline_handle_exit_error
> + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("--accept=launch incompatible with"
> + " --non-interactive")),
> + pool, "svn: ");
> + }

...and here.