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

Re: [PATCH v2] Conflict option labels

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Thu, 13 Oct 2016 17:07:37 +0200

On 13 October 2016 at 15:46, Patrick Steinhardt
<patrick.steinhardt_at_elegosoft.com> wrote:
> Hi,
>
> here's the second version of the conflict option label patch.
> Changes:
>
> - reworded some labels
> - now using apr_array to pass around options
> - renamed and simplified svn_client_resolver_option_label
>
> The functionality has been lightly tested by creating conflict
> scenarios.
>

Quick review:
> + * Return a textual human-readable label of @a option, allocated in
> + * @a result_pool. The label is encoded in UTF-8 and usually
> + * contains up to three words.
> + *
> + * Additionally, the label may be localized to the language used
> + * by the current locale.
> + *
> + * @since New in 1.10.
> + */
> +const char *
> +svn_client_conflict_option_get_label(svn_client_conflict_option_t *option);
The docstring mentions RESULT_POOL, but there is no such argument. I
think it would be better to RESULT_POOL for this function. This would
help to avoid slightly incorrect pool usage like in
find_option_by_builtin():
[[[
          client_opt = apr_pcalloc(result_pool, sizeof(*client_opt));
          client_opt->choice = id;
          client_opt->code = opt->code;
          client_opt->label = svn_client_conflict_option_get_label(
              builtin_option);
        ^^^^^ the label is not copied to RESULT_POOL.

          SVN_ERR(svn_client_conflict_option_describe(&client_opt->long_desc,
                                                      builtin_option,
                                                      result_pool,
                                                      scratch_pool));
]]]

-- 
Ivan Zhakov
Received on 2016-10-13 17:08:13 CEST

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