Re: [PATCH v2] Conflict option labels

On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt 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.
>
> [[
> Move conflict resolution options' labels out of the client
>
> * subversion/include/svn_client.h:
> - new function `svn_client_conflict_option_get_label`
> * subversion/libsvn_client/conflicts.c:
> - svn_client_conflict_option_t: add label
> - add_resolution_option: add label argument
> - implement function `svn_client_conflict_option_get_label`
> - (svn_client_conflict_text_get_reslution_options,
> svn_client_conflict_prop_get_resolution_options,
> configure_option_accept_current_wc_state,
> configure_option_move_destination,
> configure_option_update_raise_moved_away_children,
> configure_option_incoming_add_ignore,
> configure_option_incoming_added_file_text_merge,
> configure_option_incoming_added_file_replace_and_merge,
> configure_option_incoming_added_dir_merge,
> configure_option_incoming_added_dir_replace,
> configure_option_incoming_added_dir_replace_and_merge,
> configure_option_incoming_delete_ignore,
> configure_option_incoming_delete_accept,
> configure_option_incoming_move_file_merge,
> configure_option_incoming_dir_merge,
> svn_client_conflict_tree_get_resolution_options): set
> resolution option labels
> * subversion/svn/conflict-callbacks.c:
> - resolver_option_t: remove short_desc and long_desc
> - client_option_t: new struct for client options
> - builtin_resolver_options: remove short_desc and long_desc
> - (extra_resolver_options,
> extra_resolver_options_text,
> extra_resolver_options_prop,
> extra_resolver_options_tree): convert to client_option_t
> - find_option: accept options as apr_array_header_t
> - find_option_by_builtin: function to create provided options
> from builtin library options
> - find_option_by_id: replaced by find_option_by_builtin
> - (prompt_string,
> help_string,
> prompt_user,
> build_text_conflict_options,
> build_prop_conflict_options,
> build_prop_text_conflict_options,
> handle_one_prop_conflict.
> build_tree_conflict_options,
> handle_tree_conflict): accept options as apr_array_header_t
> ]]

[snip]

By the way, one problem that still exists is consistency. Right
now, we have a mixture of labels where the first character is
uppercased and labels where the first character is lowercased.
With GUI clients in mind I personally lend towards using
uppercased labels, but I'd need to adjust remaining labels to
provide them (maybe in a separate patch, this one is big enough
already as-is).

Any opinions?

Regards
Patrick