On Wed, Oct 12, 2016 at 03:38:02PM +0200, Patrick Steinhardt 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
> ]]
Paths in the log message Should be relative to ^/subversion/trunk, and symbol
names should be added.
> /**
> + * 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.
> + *
> + * 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,
I'd prefer the name svn_client_conflict_option_get_label().
> + svn_client_conflict_option_t *option,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_incoming_text_where_conflicted,
> + _("accept conflicts"),
> N_("accept changes only where they conflict"),
You don't need N_() unless the string is part of a const struct instance
or array. Just use _() instead.
> 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);
Looks like scratch_pool is not needed?
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* 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,
You don't need the 'struct' keyword since client_option_t is a typedef.
> + SVN_ERR(svn_client_conflict_option_label(&client_opt->label,
> + builtin_option,
> + result_pool,
> + scratch_pool));
Please indent either like this:
SVN_ERR(svn_client_conflict_option_label(&client_opt->label,
builtin_option,
result_pool,
scratch_pool));
Or like this if the above style exceeds 80 columns:
SVN_ERR(svn_client_conflict_option_label(
&client_opt->label, builtin_option, result_pool,
scratch_pool));
> - return NULL;
> + return SVN_NO_ERROR;
> }
Unfortunately, the interactive menu code has no automatic tests :-(
Can you briefly describe how you tested these changes?
Thanks!
Received on 2016-10-12 16:33:04 CEST