On Wed, Oct 12, 2016 at 04:32:24PM +0200, Stefan Sperling wrote:
> On Wed, Oct 12, 2016 at 03:38:02PM +0200, Patrick Steinhardt wrote:
[snip]
> > + svn_client_conflict_option_t *option,
> > + apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool);
> > +
>
> > add_resolution_option(*options, conflict,
> > svn_client_conflict_option_incoming_text_where_conflicted,
> > + _("accept conflicts"),
> > N_("accept changes only where they conflict"),
>
> You don't need N_() unless the string is part of a const struct instance
> or array. Just use _() instead.
The other option was already pre-existing with N_(…). But I can
change that as I go.
>
> > 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)
> > +{
> > + *label = apr_pstrdup(result_pool, option->label);
>
> Looks like scratch_pool is not needed?
Yeah. I re-used local style regarding here, see
`svn_client_conflict_option_describe`, where the scratch buffer
isn't really needed, as well. Will drop.
>
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
>
> > /* Return a pointer to the option description in OPTIONS matching the
> > * conflict option ID CHOICE. Return NULL if not found. */
> > -static const resolver_option_t *
> > -find_option_by_id(const resolver_option_t *options,
> > - svn_client_conflict_option_id_t choice)
> > +static svn_error_t *
> > +find_option_by_builtin(struct client_option_t **out,
>
> You don't need the 'struct' keyword since client_option_t is a typedef.
Yeah, old habit, thanks.
> > + SVN_ERR(svn_client_conflict_option_label(&client_opt->label,
> > + builtin_option,
> > + result_pool,
> > + scratch_pool));
>
> Please indent either like this:
>
> SVN_ERR(svn_client_conflict_option_label(&client_opt->label,
> builtin_option,
> result_pool,
> scratch_pool));
>
>
> Or like this if the above style exceeds 80 columns:
>
> SVN_ERR(svn_client_conflict_option_label(
> &client_opt->label, builtin_option, result_pool,
> scratch_pool));
>
> > - return NULL;
> > + return SVN_NO_ERROR;
> > }
>
>
> Unfortunately, the interactive menu code has no automatic tests :-(
> Can you briefly describe how you tested these changes?
None done yet, it was more of an RFC. Will do tomorrow. Thanks
for your review.
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
Received on 2016-10-12 17:02:17 CEST