Re: [PATCH] Conflict option labels

On Wed, Oct 12, 2016 at 04:44:10PM +0200, Ivan Zhakov wrote:
> On 12 October 2016 at 15:38, Patrick Steinhardt
> <patrick.steinhardt_at_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