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 <ps...@pks.im> on 2016/10/13 15:26:01 UTC
[PATCH v3] Conflict option labels
Hi,
the third version re-adds the result pool to
`svn_client_conflict_option_get_lazel`.
[[
Move conflict resolution options' labels out of the client
* subversion/include/svn_client.h:
(svn_client_conflict_option_get_label): New function.
* 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.
* 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): New 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
Patick
Re: [PATCH v3] Conflict option labels
Posted by Stefan <lu...@posteo.de>.
On 10/14/2016 10:27 AM, Stefan Sperling wrote:
> On Thu, Oct 13, 2016 at 05:59:01PM +0200, Stefan wrote:
>> On 10/13/2016 5:26 PM, Patrick Steinhardt wrote:
>>> sion re-adds the result pool to
>>> `svn_client_conflict_option_get_lazel`.
>>> diff --git a/subversion/include/svn_client.h
>>> b/subversion/include/svn_client.h
>>> index 9bbe62b..f456c92 100644
>>> --- a/subversion/include/svn_client.h
>>> +++ b/subversion/include/svn_client.h
>>> @@ -4718,6 +4718,20 @@ svn_client_conflict_option_id_t
>>> svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
>>>
>>> [...]
>>>
>>> add_resolution_option(*options, conflict,
>>> svn_client_conflict_option_merged_text,
>>> + _("Mark as resolved"),
>>> _("accept binary file as it appears in the working copy"),
>>> resolve_text_conflict);
>> Not sure whether "Mark as resolved" means much to the user. Maybe
>> "Accept/Use current version" would be easier to get? Or maybe
>> "Accept/Use current" to keep the label consistent with the style of the
>> others.
> To avoid yet another round-trip for Patrick, I'd prefer us to focus on
> the mechanical nature of this change, which is about moving these labels
> as-is into the library and exposing them via API.
>
> We can still tweak these strings after this change is committed (and I
> agree they should be changed).
+1
Re: [PATCH v3] Conflict option labels
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 14 October 2016 at 10:27, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Oct 13, 2016 at 05:59:01PM +0200, Stefan wrote:
>> On 10/13/2016 5:26 PM, Patrick Steinhardt wrote:
>> > sion re-adds the result pool to
>> > `svn_client_conflict_option_get_lazel`.
>>
>> > diff --git a/subversion/include/svn_client.h
>> > b/subversion/include/svn_client.h
>> > index 9bbe62b..f456c92 100644
>> > --- a/subversion/include/svn_client.h
>> > +++ b/subversion/include/svn_client.h
>> > @@ -4718,6 +4718,20 @@ svn_client_conflict_option_id_t
>> > svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
>> >
>> > [...]
>> >
>> > add_resolution_option(*options, conflict,
>> > svn_client_conflict_option_merged_text,
>> > + _("Mark as resolved"),
>> > _("accept binary file as it appears in the working copy"),
>> > resolve_text_conflict);
>> Not sure whether "Mark as resolved" means much to the user. Maybe
>> "Accept/Use current version" would be easier to get? Or maybe
>> "Accept/Use current" to keep the label consistent with the style of the
>> others.
>
> To avoid yet another round-trip for Patrick, I'd prefer us to focus on
> the mechanical nature of this change, which is about moving these labels
> as-is into the library and exposing them via API.
>
> We can still tweak these strings after this change is committed (and I
> agree they should be changed).
+1.
--
Ivan Zhakov
Re: [PATCH v3] Conflict option labels
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 13, 2016 at 05:59:01PM +0200, Stefan wrote:
> On 10/13/2016 5:26 PM, Patrick Steinhardt wrote:
> > sion re-adds the result pool to
> > `svn_client_conflict_option_get_lazel`.
>
> > diff --git a/subversion/include/svn_client.h
> > b/subversion/include/svn_client.h
> > index 9bbe62b..f456c92 100644
> > --- a/subversion/include/svn_client.h
> > +++ b/subversion/include/svn_client.h
> > @@ -4718,6 +4718,20 @@ svn_client_conflict_option_id_t
> > svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
> >
> > [...]
> >
> > add_resolution_option(*options, conflict,
> > svn_client_conflict_option_merged_text,
> > + _("Mark as resolved"),
> > _("accept binary file as it appears in the working copy"),
> > resolve_text_conflict);
> Not sure whether "Mark as resolved" means much to the user. Maybe
> "Accept/Use current version" would be easier to get? Or maybe
> "Accept/Use current" to keep the label consistent with the style of the
> others.
To avoid yet another round-trip for Patrick, I'd prefer us to focus on
the mechanical nature of this change, which is about moving these labels
as-is into the library and exposing them via API.
We can still tweak these strings after this change is committed (and I
agree they should be changed).
Re: [PATCH v3] Conflict option labels
Posted by Stefan <lu...@posteo.de>.
On 10/13/2016 5:26 PM, Patrick Steinhardt wrote:
> sion re-adds the result pool to
> `svn_client_conflict_option_get_lazel`.
> diff --git a/subversion/include/svn_client.h
> b/subversion/include/svn_client.h
> index 9bbe62b..f456c92 100644
> --- a/subversion/include/svn_client.h
> +++ b/subversion/include/svn_client.h
> @@ -4718,6 +4718,20 @@ svn_client_conflict_option_id_t
> svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
>
> [...]
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_merged_text,
> + _("Mark as resolved"),
> _("accept binary file as it appears in the working copy"),
> resolve_text_conflict);
Not sure whether "Mark as resolved" means much to the user. Maybe
"Accept/Use current version" would be easier to get? Or maybe
"Accept/Use current" to keep the label consistent with the style of the
others.
> [...]
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text,
> + _("Reject incoming"),
> _("reject all incoming changes for this file"),
> resolve_text_conflict);
IMO "Reject incoming" is the inversed way to describe what's being done.
Better would be: "Accept current"?
>
> [...]
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text_where_conflicted,
> + _("Reject conflicts"),
> _("reject changes which conflict and accept the rest"),
> resolve_text_conflict);
=> "Accept incoming non-conflicting changes only."?
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_merged_text,
> + _("Mark as resolved"),
> _("accept the file as it appears in the working copy"),
> resolve_text_conflict);
Same as above: "Accept/Use current"
> [...]
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text,
> + _("Mark as resolved"),
> _("accept working copy version of entire property value"),
> resolve_prop_conflict);
Same as above.
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_incoming_text_where_conflicted,
> - N_("accept changes only where they conflict"),
> + _("Accept incoming for conflicts"),
> + _("accept incoming changes only where they conflict"),
> resolve_prop_conflict);
The wording is a bit misleading IMO. It might be interpreted as
non-conflicting incoming changes being rejected rather than these being
merged implicitly (or am I wrong?).
Maybe better wording would be:
Label: Resolve conflicts with incoming changes
Desc: Accept incoming and resolve conflicts using the incoming changes.
>
> add_resolution_option(*options, conflict,
> svn_client_conflict_option_working_text_where_conflicted,
> + _("Reject conflicts"),
> _("reject changes which conflict and accept the rest"),
> resolve_prop_conflict);
Reject conflicts doesn't quite reflect what's being done. Maybe:
"Resolve conflicts with local changes."
"Accept incoming and resolve conflicts using the local changes."
> [...]
> add_resolution_option(options, conflict,
>
> svn_client_conflict_option_accept_current_wc_state,
> + _("Mark as resolved"),
> _("accept current working copy state"),
> do_resolve_func);
See above.
>
> @@ -7264,6 +7285,7 @@
> configure_option_update_move_destination(svn_client_conflict_t *conflict,
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_update_move_destination,
> + _("Update move destination"),
> _("apply incoming changes to move destination"),
> resolve_update_moved_away_node);
> }
The label doesn't quite reflect what's being done (aka: how it's
updated). Maybe: "Apply changes to move destination"
> @@ -7298,6 +7320,7 @@ configure_option_update_raise_moved_away_children(
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_update_any_moved_away_children,
> + _("Update any moved-away children"),
> _("prepare for updating moved-away children, if any"),
> resolve_update_raise_moved_away);
> }
Same as above.
> [...]
>
> return SVN_NO_ERROR;
> @@ -7425,7 +7448,8 @@
> configure_option_incoming_added_file_text_merge(svn_client_conflict_t
> *conflict,
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_incoming_added_file_text_merge,
> - description, resolve_merge_incoming_added_file_text_merge);
> + _("Merge the files"), description,
> + resolve_merge_incoming_added_file_text_merge);
> }
Maybe better: Merge incoming file with local.
>
> return SVN_NO_ERROR;
> @@ -7481,6 +7505,7 @@
> configure_option_incoming_added_file_replace_and_merge(
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_incoming_added_file_replace_and_merge,
> + _("Replace and merge"),
> description,
> resolve_merge_incoming_added_file_replace_and_merge);
> }
TBH: I'm not sure what the behavior of this option is. It sounds like
it's the same as the previous one to me... Where's the difference?
>
> @@ -7533,7 +7558,7 @@
> configure_option_incoming_added_dir_merge(svn_client_conflict_t *conflict,
> scratch_pool));
> add_resolution_option(options, conflict,
>
> svn_client_conflict_option_incoming_added_dir_merge,
> - description,
> + _("Merge the directories"), description,
> resolve_merge_incoming_added_dir_merge);
> }
>
> [..]
>
> return SVN_NO_ERROR;
> @@ -7644,8 +7669,8 @@
> configure_option_incoming_added_dir_replace_and_merge(
> add_resolution_option(
> options, conflict,
> svn_client_conflict_option_incoming_added_dir_replace_and_merge,
> - description,
> - resolve_merge_incoming_added_dir_replace_and_merge);
> + _("Replace and merge"),
> + description, resolve_merge_incoming_added_dir_replace_and_merge);
> }
Same as before. Where is this different to
svn_client_conflict_option_incoming_added_dir_merge?
Great work btw. :-)
Regards,
Stefan
Re: [PATCH v3] Conflict option labels
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 14 October 2016 at 12:06, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 14 October 2016 at 11:43, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On 13 October 2016 at 17:26, Patrick Steinhardt <ps...@pks.im> wrote:
>>> Hi,
>>>
>>> the third version re-adds the result pool to
>>> `svn_client_conflict_option_get_lazel`.
>>>
>> [...]
>>> @@ -582,15 +604,16 @@ prompt_string(const resolver_option_t *options,
>>> }
>>> else
>>> {
>>> - opt = options++;
>>> - if (! opt->code)
>>> + if (i >= options->nelts)
>>> break;
>>> + opt = APR_ARRAY_IDX(options, i, client_option_t *);
>>> + i++;
>>> }
>>>
>>> if (! first)
>>> result = apr_pstrcat(pool, result, ",", SVN_VA_NULL);
>>> s = apr_psprintf(pool, " (%s) %s", opt->code,
>>> - opt->short_desc ? _(opt->short_desc) : opt->long_desc);
>>> + opt->label ? _(opt->label) : opt->long_desc);
>> The opt->label is already localized, so _() is not needed.
>>
>> Beside of that patch looks fine and I'm ready to commit it in current
>> state. Stefan, do you have any comments on the patch?
>>
> Here is updated patch and log message that adapted to r1764848 changes.
Committed in r1764883. Thanks!
--
Ivan Zhakov
Re: [PATCH v3] Conflict option labels
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 14 October 2016 at 11:43, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 13 October 2016 at 17:26, Patrick Steinhardt <ps...@pks.im> wrote:
>> Hi,
>>
>> the third version re-adds the result pool to
>> `svn_client_conflict_option_get_lazel`.
>>
> [...]
>> @@ -582,15 +604,16 @@ prompt_string(const resolver_option_t *options,
>> }
>> else
>> {
>> - opt = options++;
>> - if (! opt->code)
>> + if (i >= options->nelts)
>> break;
>> + opt = APR_ARRAY_IDX(options, i, client_option_t *);
>> + i++;
>> }
>>
>> if (! first)
>> result = apr_pstrcat(pool, result, ",", SVN_VA_NULL);
>> s = apr_psprintf(pool, " (%s) %s", opt->code,
>> - opt->short_desc ? _(opt->short_desc) : opt->long_desc);
>> + opt->label ? _(opt->label) : opt->long_desc);
> The opt->label is already localized, so _() is not needed.
>
> Beside of that patch looks fine and I'm ready to commit it in current
> state. Stefan, do you have any comments on the patch?
>
Here is updated patch and log message that adapted to r1764848 changes.
[[
Move conflict resolution options' labels out of the client
* subversion/include/svn_client.h:
(svn_client_conflict_option_get_label): New function.
* 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,
configure_option_local_move_file_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): New 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.
]]
--
Ivan Zhakov
Re: [PATCH v3] Conflict option labels
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 13 October 2016 at 17:26, Patrick Steinhardt <ps...@pks.im> wrote:
> Hi,
>
> the third version re-adds the result pool to
> `svn_client_conflict_option_get_lazel`.
>
[...]
> @@ -582,15 +604,16 @@ prompt_string(const resolver_option_t *options,
> }
> else
> {
> - opt = options++;
> - if (! opt->code)
> + if (i >= options->nelts)
> break;
> + opt = APR_ARRAY_IDX(options, i, client_option_t *);
> + i++;
> }
>
> if (! first)
> result = apr_pstrcat(pool, result, ",", SVN_VA_NULL);
> s = apr_psprintf(pool, " (%s) %s", opt->code,
> - opt->short_desc ? _(opt->short_desc) : opt->long_desc);
> + opt->label ? _(opt->label) : opt->long_desc);
The opt->label is already localized, so _() is not needed.
Beside of that patch looks fine and I'm ready to commit it in current
state. Stefan, do you have any comments on the patch?
--
Ivan Zhakov