On 12 October 2016 at 15:38, Patrick Steinhardt
<patrick.steinha...@elegosoft.com> wrote:
> Hi,
>
> please find attached a patch pulling out the short descriptions
> of conflict resolution options from the client and puts them into
> libsvn_client.
>
> [[
> Move conflict resolution options' labels out of the client
>
> * include/svn_client.h:
> - Provide function `svn_client_conflict_option_label`
> * libsvn_client/conflicts.c:
> - Implement function `svn_client_conflict_option_label`
> - Introduce and set label field for svn_conflict_option_t
> * svn/conflict-callbacks.c:
> - Split client-specific and built-in resolver options
> - Implement conversion from built-in resolvers to
> client-specific options
> ]]
Hi Patrick. Thank you for the patch.
See my review inline:
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1764453)
> +++ subversion/include/svn_client.h (working copy)
> @@ -4718,6 +4718,22 @@
> svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
>
> /**
> + * Return a textual human-readable label of @a option, allocated in
> + * @a result_pool. The label is encoded in UTF-8 and may contain
> + * up to three words.
May replace 'may contain up to three words' -> 'usually contain up to
three words'?
> + *
> + * Additionally, the label may be localized to the language used
> + * by the current locale.
> + *
> + * @since New in 1.10.
> + */
> +svn_error_t *
> +svn_client_conflict_option_label(const char **label,
> + svn_client_conflict_option_t *option,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
The function name svn_client_conflict_option_label() sounds like an
operation instead of getter. May be
svn_client_conflict_option_get_label() is better name.
Another suggestion: may be return string instead of svn_error_t? I
hardly believe that this function may fail. I.e.:
const char *
svn_client_conflict_option_label(const char **label,
svn_client_conflict_option_t *option,
apr_pool_t *pool);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_postpone,
> + _("postpone"),
> _("skip this conflict and leave it unresolved"),
> resolve_postpone);
>
> @@ -7112,16 +7116,19 @@
> /* Resolver options for a binary file conflict. */
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_base_text,
> + _("accept base"),
> _("discard local and incoming changes for this binary file"),
> resolve_text_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_incoming_text,
> + _("accept theirs"),
As far I remember we're trying move from term mine/theirs to
incoming/local. What do you think about using 'Accept incoming text'
or 'Use incoming text' label for this option?
> _("accept incoming version of binary file"),
> resolve_text_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_merged_text,
> + _("accept working copy"),
I suggest use name 'mark as resolved'.
> _("accept binary file as it appears in the working copy"),
> resolve_text_conflict);
> }
> @@ -7130,31 +7137,37 @@
> /* Resolver options for a text file conflict. */
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_base_text,
> + _("accept base"),
I think we need better label for this option, but I don't have good ideas :(
> _("discard local and incoming changes for this file"),
> resolve_text_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_incoming_text,
> + _("accept theirs"),
> _("accept incoming version of entire file"),
> resolve_text_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text,
> + _("reject theirs"),
> _("reject all incoming changes for this file"),
> resolve_text_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_incoming_text_where_conflicted,
> + _("accept conflicts"),
I think we need better label for this option, but I don't have good ideas :(
> _("accept changes only where they conflict"),
> resolve_text_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text_where_conflicted,
> + _("reject conflicts"),
> _("reject changes which conflict and accept the rest"),
> resolve_text_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_merged_text,
> + _("accept working copy"),
> _("accept the file as it appears in the working copy"),
> resolve_text_conflict);
> }
> @@ -7176,36 +7189,43 @@
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_postpone,
> + _("postpone"),
> _("skip this conflict and leave it unresolved"),
> resolve_postpone);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_base_text,
> + _("accept base"),
> _("discard local and incoming changes for this property"),
> resolve_prop_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_incoming_text,
> + _("accept theirs"),
> _("accept incoming version of entire property value"),
> resolve_prop_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text,
> + _("accept working copy"),
'mark as resolved'
> _("accept working copy version of entire property value"),
> resolve_prop_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_incoming_text_where_conflicted,
> + _("accept conflicts"),
> N_("accept changes only where they conflict"),
> resolve_prop_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text_where_conflicted,
> + _("reject conflicts"),
> _("reject changes which conflict and accept the rest"),
> resolve_prop_conflict);
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_merged_text,
> + _("accept merged"),
> _("accept merged version of property value"),
> resolve_prop_conflict);
>
> @@ -7244,6 +7264,7 @@
>
> add_resolution_option(options, conflict,
> svn_client_conflict_option_accept_current_wc_state,
> + _("accept working copy"),
'mark as resolved'
> _("accept current working copy state"),
> do_resolve_func);
>
[...]
> return SVN_NO_ERROR;
> @@ -7651,8 +7676,8 @@
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_incoming_added_dir_replace_and_merge,
> - description,
> - resolve_merge_incoming_added_dir_replace_and_merge);
> + N_("replace my directory with incoming directory and merge"),
'replace and merge' ?
> + description, resolve_merge_incoming_added_dir_replace_and_merge);
> }
>
> return SVN_NO_ERROR;
> @@ -7724,7 +7749,7 @@
>
> add_resolution_option(options, conflict,
>
> svn_client_conflict_option_incoming_delete_ignore,
> - description,
> + N_("ignore incoming deletion"), description,
> resolve_incoming_delete_ignore);
> }
>
> @@ -7783,7 +7808,7 @@
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_incoming_delete_accept,
> - description,
> + N_("accept incoming deletion"), description,
> resolve_incoming_delete_accept);
> }
> }
> @@ -7960,7 +7985,7 @@
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_incoming_move_file_text_merge,
> - description,
> + _("move and merge"), description,
> resolve_incoming_move_file_text_merge);
> }
>
> @@ -8078,7 +8103,7 @@
> scratch_pool));
> add_resolution_option(options, conflict,
>
> svn_client_conflict_option_incoming_move_dir_merge,
> - description,
> + _("move and merge"), description,
> resolve_incoming_move_dir_merge);
> }
>
> @@ -8329,6 +8354,7 @@
> /* Add postpone option. */
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_postpone,
> + _("postpone"),
> _("skip this conflict and leave it unresolved"),
> resolve_postpone);
>
> @@ -8419,6 +8445,17 @@
> }
>
> svn_error_t *
> +svn_client_conflict_option_label(const char **label,
> + svn_client_conflict_option_t *option,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + *label = apr_pstrdup(result_pool, option->label);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> svn_client_conflict_option_describe(const char **description,
> svn_client_conflict_option_t *option,
> apr_pool_t *result_pool,
> Index: subversion/svn/conflict-callbacks.c
> ===================================================================
> --- subversion/svn/conflict-callbacks.c (revision 1764453)
> +++ subversion/svn/conflict-callbacks.c (working copy)
> @@ -374,80 +374,71 @@
> typedef struct resolver_option_t
> {
> const char *code; /* one or two characters */
> - const char *short_desc; /* label in prompt (localized) */
> - const char *long_desc; /* longer description (localized) */
> svn_client_conflict_option_id_t choice;
> /* or ..._undefined if not from libsvn_client */
> const char *accept_arg; /* --accept option argument (NOT localized) */
> } resolver_option_t;
>
> +typedef struct client_option_t
> +{
> + const char *code; /* one or two characters */
> + const char *label; /* label in prompt (localized) */
> + const char *long_desc; /* longer description (localized) */
> + svn_client_conflict_option_id_t choice;
> + /* or ..._undefined if not from libsvn_client */
> + const char *accept_arg; /* --accept option argument (NOT localized) */
> +} client_option_t;
> +
> /* Resolver options for conflict options offered by libsvn_client. */
> static const resolver_option_t builtin_resolver_options[] =
> {
> - { "r", NULL, NULL,
> - svn_client_conflict_option_merged_text,
> - SVN_CL__ACCEPT_WORKING },
> - { "mc", NULL, NULL,
> - svn_client_conflict_option_working_text_where_conflicted,
> - SVN_CL__ACCEPT_MINE_CONFLICT },
> - { "tc", NULL, NULL,
> - svn_client_conflict_option_incoming_text_where_conflicted,
> - SVN_CL__ACCEPT_THEIRS_CONFLICT },
> - { "mf", NULL, NULL,
> - svn_client_conflict_option_working_text,
> - SVN_CL__ACCEPT_MINE_FULL},
> - { "tf", NULL, NULL,
> - svn_client_conflict_option_incoming_text,
> - SVN_CL__ACCEPT_THEIRS_FULL },
> - { "p", N_("postpone"), NULL,
> - svn_client_conflict_option_postpone,
> - SVN_CL__ACCEPT_POSTPONE },
> + { "r", svn_client_conflict_option_merged_text,
> + SVN_CL__ACCEPT_WORKING },
> + { "mc", svn_client_conflict_option_working_text_where_conflicted,
> + SVN_CL__ACCEPT_MINE_CONFLICT },
> + { "tc", svn_client_conflict_option_incoming_text_where_conflicted,
> + SVN_CL__ACCEPT_THEIRS_CONFLICT },
> + { "mf", svn_client_conflict_option_working_text,
> + SVN_CL__ACCEPT_MINE_FULL},
> + { "tf", svn_client_conflict_option_incoming_text,
> + SVN_CL__ACCEPT_THEIRS_FULL },
> + { "p", svn_client_conflict_option_postpone,
> + SVN_CL__ACCEPT_POSTPONE },
>
> /* This option resolves a tree conflict to the current working copy state.
> */
> - { "r", NULL, NULL,
> - svn_client_conflict_option_accept_current_wc_state,
> - SVN_CL__ACCEPT_WORKING },
> + { "r", svn_client_conflict_option_accept_current_wc_state,
> + SVN_CL__ACCEPT_WORKING },
>
> /* These options use the same code since they only occur in
> * distinct conflict scenarios. */
> - { "u", N_("update move destination"), NULL,
> - svn_client_conflict_option_update_move_destination },
> - { "u", N_("update any moved-away children"), NULL,
> - svn_client_conflict_option_update_any_moved_away_children },
> + { "u", svn_client_conflict_option_update_move_destination },
> + { "u", svn_client_conflict_option_update_any_moved_away_children },
>
> /* Options for incoming add vs local add. */
> - { "i", N_("ignore incoming addition"), NULL,
> - svn_client_conflict_option_incoming_add_ignore },
> + { "i", svn_client_conflict_option_incoming_add_ignore },
>
> /* Options for incoming file add vs local file add upon merge. */
> - { "m", N_("merge the files"), NULL,
> - svn_client_conflict_option_incoming_added_file_text_merge },
> - { "M", N_("replace my file with incoming file and merge the files"), NULL,
> - svn_client_conflict_option_incoming_added_file_replace_and_merge },
> + { "m", svn_client_conflict_option_incoming_added_file_text_merge },
> + { "M", svn_client_conflict_option_incoming_added_file_replace_and_merge },
>
> /* Options for incoming dir add vs local dir add upon merge. */
> - { "m", N_("merge the directories"), NULL,
> - svn_client_conflict_option_incoming_added_dir_merge },
> - { "R", N_("delete my directory and replace it with incoming directory"),
> NULL,
> - svn_client_conflict_option_incoming_added_dir_replace },
> - { "M", N_("replace my directory with incoming directory and merge"), NULL,
> - svn_client_conflict_option_incoming_added_dir_replace_and_merge },
> + { "m", svn_client_conflict_option_incoming_added_dir_merge },
> + { "R", svn_client_conflict_option_incoming_added_dir_replace },
> + { "M", svn_client_conflict_option_incoming_added_dir_replace_and_merge },
>
> /* Options for incoming delete vs any. */
> - { "i", N_("ignore incoming deletion"), NULL,
> - svn_client_conflict_option_incoming_delete_ignore },
> - { "a", N_("accept incoming deletion"), NULL,
> - svn_client_conflict_option_incoming_delete_accept },
> + { "i", svn_client_conflict_option_incoming_delete_ignore },
> + { "a", svn_client_conflict_option_incoming_delete_accept },
>
> /* Options for incoming move vs local edit. */
> - { "m", NULL, NULL,
> svn_client_conflict_option_incoming_move_file_text_merge },
> - { "m", NULL, NULL, svn_client_conflict_option_incoming_move_dir_merge },
> + { "m", svn_client_conflict_option_incoming_move_file_text_merge },
> + { "m", svn_client_conflict_option_incoming_move_dir_merge },
>
> { NULL }
> };
>
> /* Extra resolver options offered by 'svn' for any conflict. */
> -static const resolver_option_t extra_resolver_options[] =
> +static const client_option_t extra_resolver_options[] =
> {
> /* Translators: keep long_desc below 70 characters (wrap with a left
> margin of 9 spaces if needed) */
> @@ -458,7 +449,7 @@
>
>
> /* Additional resolver options offered by 'svn' for a text conflict. */
> -static const resolver_option_t extra_resolver_options_text[] =
> +static const client_option_t extra_resolver_options_text[] =
> {
> /* Translators: keep long_desc below 70 characters (wrap with a left
> margin of 9 spaces if needed) */
> @@ -485,7 +476,7 @@
> };
>
> /* Additional resolver options offered by 'svn' for a property conflict. */
> -static const resolver_option_t extra_resolver_options_prop[] =
> +static const client_option_t extra_resolver_options_prop[] =
> {
> /* Translators: keep long_desc below 70 characters (wrap with a left
> margin of 9 spaces if needed) */
> @@ -501,7 +492,7 @@
> };
>
> /* Additional resolver options offered by 'svn' for a tree conflict. */
> -static const resolver_option_t extra_resolver_options_tree[] =
> +static const client_option_t extra_resolver_options_tree[] =
> {
> /* Translators: keep long_desc below 70 characters (wrap with a left
> margin of 9 spaces if needed) */
> @@ -522,11 +513,11 @@
>
> /* Return a pointer to the option description in OPTIONS matching the
> * one- or two-character OPTION_CODE. Return NULL if not found. */
> -static const resolver_option_t *
> -find_option(const resolver_option_t *options,
> +static const client_option_t *
> +find_option(const client_option_t *options,
> const char *option_code)
> {
> - const resolver_option_t *opt;
> + const client_option_t *opt;
>
> for (opt = options; opt->code; opt++)
> {
> @@ -539,24 +530,51 @@
>
> /* Return a pointer to the option description in OPTIONS matching the
> * conflict option ID CHOICE. Return NULL if not found. */
> -static const resolver_option_t *
> -find_option_by_id(const resolver_option_t *options,
> - svn_client_conflict_option_id_t choice)
> +static svn_error_t *
> +find_option_by_builtin(struct client_option_t **out,
> + const resolver_option_t *options,
> + svn_client_conflict_option_t *builtin_option,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
> const resolver_option_t *opt;
> + svn_client_conflict_option_id_t id;
>
> + *out = NULL;
I think it would be more clear to set '*out = NULL' at the end of the
function. But the all code will be simplified if we drop svn_error_t
from svn_client_option_get_label().
> +
> + id = svn_client_conflict_option_get_id(builtin_option);
> +
> for (opt = options; opt->code; opt++)
> {
> - if (opt->choice == choice)
> - return opt;
> + if (opt->choice == id)
> + {
> + client_option_t *client_opt;
> +
> + client_opt = apr_pcalloc(result_pool, sizeof(*client_opt));
> + client_opt->choice = id;
> + client_opt->code = opt->code;
> + SVN_ERR(svn_client_conflict_option_label(&client_opt->label,
> + builtin_option,
> + result_pool,
> + scratch_pool));
Indentation problem: function arguments should be aligned:
[[
SVN_ERR(svn_client_conflict_option_label(&client_opt->label,
builtin_option,
result_pool,
scratch_pool));
]]
> + SVN_ERR(svn_client_conflict_option_describe(&client_opt->long_desc,
> + builtin_option,
> + result_pool,
> + scratch_pool));
> + client_opt->accept_arg = opt->accept_arg;
> +
> + *out = client_opt;
> +
> + return SVN_NO_ERROR;
> + }
> }
> - return NULL;
> + return SVN_NO_ERROR;
> }
>
[...]
> @@ -693,7 +711,7 @@
>
> /* Set *OPTIONS to an array of resolution options for CONFLICT. */
> static svn_error_t *
> -build_text_conflict_options(resolver_option_t **options,
> +build_text_conflict_options(client_option_t **options,
> svn_client_conflict_t *conflict,
> svn_client_ctx_t *ctx,
> svn_boolean_t is_binary,
> @@ -700,8 +718,8 @@
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> - resolver_option_t *opt;
> - const resolver_option_t *o;
> + client_option_t *opt;
> + const client_option_t *o;
> apr_array_header_t *builtin_options;
> apr_size_t nopt;
> int i;
> @@ -714,7 +732,7 @@
> nopt = builtin_options->nelts + ARRAY_LEN(extra_resolver_options);
> if (!is_binary)
> nopt += ARRAY_LEN(extra_resolver_options_text);
> - *options = apr_pcalloc(result_pool, sizeof(*opt) * (nopt + 1));
> + *options = apr_pcalloc(result_pool, sizeof(opt) * (nopt + 1));
This change looks suspicions: Are you sure that you passing write size
to apr_pcalloc()?
[...]
svn_client_conflict_option_label
[1]
https://subversion.apache.org/docs/community-guide/conventions.html#log-messages
--
Ivan Zhakov
