You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Patrick Steinhardt <pa...@elegosoft.com> on 2016/10/13 13:46:32 UTC
[PATCH v2] Conflict option labels
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
]]
Regards
--
Patrick Steinhardt, Entwickler
elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
Re: [PATCH v2] Conflict option labels
Posted by Patrick Steinhardt <ps...@pks.im>.
On Thu, Oct 13, 2016 at 04:52:12PM +0200, Stefan Sperling wrote:
> On Thu, Oct 13, 2016 at 04:46:55PM +0200, Patrick Steinhardt wrote:
> > Is there by any chance a tool which helps generating this tedious
> > list?
>
> There are some but I cannot tell which is best and how well these are working.
>
> https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/vim/
> https://svn.apache.org/repos/asf/subversion/trunk/tools/dev/mklog.py
Oh, that's cool. Thanks for the pointers.
Regards
Patrick
Re: [PATCH v2] Conflict option labels
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 13, 2016 at 04:46:55PM +0200, Patrick Steinhardt wrote:
> Is there by any chance a tool which helps generating this tedious
> list?
There are some but I cannot tell which is best and how well these are working.
https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/vim/
https://svn.apache.org/repos/asf/subversion/trunk/tools/dev/mklog.py
Re: [PATCH v2] Conflict option labels
Posted by Patrick Steinhardt <pa...@elegosoft.com>.
On Thu, Oct 13, 2016 at 04:40:59PM +0200, Stefan Sperling wrote:
> On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote:
> > * 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
>
> This log message format is not entirely conforming to our guidelines.
>
> Can you review existing log messages for examples and adjust your
> patch submission accordingly?
>
> The above would usually be formatted like this:
>
> * subversion/libsvn_client/conflicts.c
> (svn_client_conflict_option_t): Add label.
> (add_resolution_option): Add label argument.
> (svn_client_conflict_option_get_label): New function.
> (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.
Is there by any chance a tool which helps generating this tedious
list?
Regards
--
Patrick Steinhardt, Entwickler
elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
Re: [PATCH v2] Conflict option labels
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote:
> * 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
This log message format is not entirely conforming to our guidelines.
Can you review existing log messages for examples and adjust your
patch submission accordingly?
The above would usually be formatted like this:
* subversion/libsvn_client/conflicts.c
(svn_client_conflict_option_t): Add label.
(add_resolution_option): Add label argument.
(svn_client_conflict_option_get_label): New function.
(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.
Re: [PATCH v2] Conflict option labels
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 13 October 2016 at 15:52, Patrick Steinhardt <ps...@pks.im> wrote:
> On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote:
>> Hi,
>>
[..]
> [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?
>
I'm for uppercased labels.
--
Ivan Zhakov
Re: [PATCH v2] Conflict option labels
Posted by Patrick Steinhardt <ps...@pks.im>.
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
Re: [PATCH v2] Conflict option labels
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 13 October 2016 at 15:46, Patrick Steinhardt
<pa...@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