[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Conflict option labels

From: Stefan Sperling <stsp_at_elego.de>
Date: Wed, 12 Oct 2016 16:32:24 +0200

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

This is an archived mail posted to the Subversion Dev mailing list.