On Wed, Oct 12, 2016 at 04:44:10PM +0200, Ivan Zhakov wrote: > On 12 October 2016 at 15:38, Patrick Steinhardt > <patrick.steinha...@elegosoft.com> wrote: > > Hi, > > > > please find attached a patch pulling out the short descriptions > > of conflict resolution options from the client and puts them into > > libsvn_client. > > > > [[ > > Move conflict resolution options' labels out of the client > > > > * include/svn_client.h: > > - Provide function `svn_client_conflict_option_label` > > * libsvn_client/conflicts.c: > > - Implement function `svn_client_conflict_option_label` > > - Introduce and set label field for svn_conflict_option_t > > * svn/conflict-callbacks.c: > > - Split client-specific and built-in resolver options > > - Implement conversion from built-in resolvers to > > client-specific options > > ]] > > Hi Patrick. Thank you for the patch. > > See my review inline: > > Index: subversion/include/svn_client.h > > =================================================================== > > --- subversion/include/svn_client.h (revision 1764453) > > +++ subversion/include/svn_client.h (working copy) > > @@ -4718,6 +4718,22 @@ > > svn_client_conflict_option_get_id(svn_client_conflict_option_t *option); > > > > /** > > + * Return a textual human-readable label of @a option, allocated in > > + * @a result_pool. The label is encoded in UTF-8 and may contain > > + * up to three words. > May replace 'may contain up to three words' -> 'usually contain up to > three words'?
Agreed. > > > + * > > + * Additionally, the label may be localized to the language used > > + * by the current locale. > > + * > > + * @since New in 1.10. > > + */ > > +svn_error_t * > > +svn_client_conflict_option_label(const char **label, > > + svn_client_conflict_option_t *option, > > + apr_pool_t *result_pool, > > + apr_pool_t *scratch_pool); > The function name svn_client_conflict_option_label() sounds like an > operation instead of getter. May be > svn_client_conflict_option_get_label() is better name. > > Another suggestion: may be return string instead of svn_error_t? I > hardly believe that this function may fail. I.e.: > const char * > svn_client_conflict_option_label(const char **label, > svn_client_conflict_option_t *option, > apr_pool_t *pool); Yeah. It's still a remnant of the olden days where I started implementing the feature. > > > > > add_resolution_option(*options, conflict, > > svn_client_conflict_option_postpone, > > + _("postpone"), > > _("skip this conflict and leave it unresolved"), > > resolve_postpone); > > > > @@ -7112,16 +7116,19 @@ > > /* Resolver options for a binary file conflict. */ > > add_resolution_option(*options, conflict, > > svn_client_conflict_option_base_text, > > + _("accept base"), > > _("discard local and incoming changes for this binary file"), > > resolve_text_conflict); > > > > add_resolution_option(*options, conflict, > > svn_client_conflict_option_incoming_text, > > + _("accept theirs"), > As far I remember we're trying move from term mine/theirs to > incoming/local. What do you think about using 'Accept incoming text' > or 'Use incoming text' label for this option? Okay. [snip] > > add_resolution_option(*options, conflict, > > svn_client_conflict_option_incoming_text_where_conflicted, > > + _("accept conflicts"), > I think we need better label for this option, but I don't have good ideas :( Maybe "Accept incoming on conflict" or suchsome? It's more than three words and still is somewhat ambiguous, but maybe better than `accept conflicts`. [snip] > > +static svn_error_t * > > +find_option_by_builtin(struct client_option_t **out, > > + const resolver_option_t *options, > > + svn_client_conflict_option_t *builtin_option, > > + apr_pool_t *result_pool, > > + apr_pool_t *scratch_pool) > > { > > const resolver_option_t *opt; > > + svn_client_conflict_option_id_t id; > > > > + *out = NULL; > I think it would be more clear to set '*out = NULL' at the end of the > function. But the all code will be simplified if we drop svn_error_t > from svn_client_option_get_label(). I'm a fan to clear `out` parameters at the beginning to avoid the case where we return early and leave it uninitialized. And in fact only simplifying `svn_conflict_option_get_label` does not suffice given that `svn_client_conflict_option_describe` may still fail. So I'd leave this function's return value as-is. > > > + > > + id = svn_client_conflict_option_get_id(builtin_option); > > + > > for (opt = options; opt->code; opt++) > > { > > - if (opt->choice == choice) > > - return opt; > > + if (opt->choice == id) > > + { > > + client_option_t *client_opt; > > + > > + client_opt = apr_pcalloc(result_pool, sizeof(*client_opt)); > > + client_opt->choice = id; > > + client_opt->code = opt->code; > > + SVN_ERR(svn_client_conflict_option_label(&client_opt->label, > > + builtin_option, > > + result_pool, > > + scratch_pool)); > Indentation problem: function arguments should be aligned: > [[ > SVN_ERR(svn_client_conflict_option_label(&client_opt->label, > builtin_option, > result_pool, > scratch_pool)); > ]] > > > + > > SVN_ERR(svn_client_conflict_option_describe(&client_opt->long_desc, > > + builtin_option, > > + result_pool, > > + scratch_pool)); > > + client_opt->accept_arg = opt->accept_arg; > > + > > + *out = client_opt; > > + > > + return SVN_NO_ERROR; > > + } > > } > > - return NULL; > > + return SVN_NO_ERROR; > > } > > > [...] > > > @@ -693,7 +711,7 @@ > > > > /* Set *OPTIONS to an array of resolution options for CONFLICT. */ > > static svn_error_t * > > -build_text_conflict_options(resolver_option_t **options, > > +build_text_conflict_options(client_option_t **options, > > svn_client_conflict_t *conflict, > > svn_client_ctx_t *ctx, > > svn_boolean_t is_binary, > > @@ -700,8 +718,8 @@ > > apr_pool_t *result_pool, > > apr_pool_t *scratch_pool) > > { > > - resolver_option_t *opt; > > - const resolver_option_t *o; > > + client_option_t *opt; > > + const client_option_t *o; > > apr_array_header_t *builtin_options; > > apr_size_t nopt; > > int i; > > @@ -714,7 +732,7 @@ > > nopt = builtin_options->nelts + ARRAY_LEN(extra_resolver_options); > > if (!is_binary) > > nopt += ARRAY_LEN(extra_resolver_options_text); > > - *options = apr_pcalloc(result_pool, sizeof(*opt) * (nopt + 1)); > > + *options = apr_pcalloc(result_pool, sizeof(opt) * (nopt + 1)); > This change looks suspicions: Are you sure that you passing write size > to apr_pcalloc()? No, this is actually intended. Previously, we initialized the complete array and set the struct's pointers in-place. Now we want to allocate an array of pointers and assign the pointers. Thanks for your review. Patrick -- 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
signature.asc
Description: PGP signature