Re: [PATCH v3] Conflict option labels

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