Re: svn commit: r34967 - trunk/subversion/svn

2008-12-30 00:25:50 Daniel Shahaf napisał(a):
> (Can't sleep...)
>
> Arfrever Frehtes Taifersar Arahesis wrote on Mon, 29 Dec 2008 at 12:24 -0800:
> > Author: arfrever
> > Date: Mon Dec 29 12:24:29 2008
> > New Revision: 34967
> >
> > Log:
> > Implement the '--config-option=FILE:SECTION:OPTION=[VALUE]' option for svn.
> >
> ...
> >
> > Modified: trunk/subversion/svn/main.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/main.c?pathrev=34967&r1=34966&r2=34967
> > ==============================================================================
> > --- trunk/subversion/svn/main.c Mon Dec 29 11:12:18 2008 (r34966)
> > +++ trunk/subversion/svn/main.c Mon Dec 29 12:24:29 2008 (r34967)
> > @@ -1020,6 +1023,13 @@ svn_cl__check_cancel(void *baton)
> > return SVN_NO_ERROR;
> > }
> >
> > +typedef struct config_option_t
> > +{
> > + char *file;
> > + char *section;
> > + char *option;
> > + char *value;
>
> s/char/const char/ ?
>
> > +} config_option_t;
> >
> > 
> > /*** Main. ***/
> > @@ -1424,6 +1434,115 @@ main(int argc, const char *argv[])
> > err = svn_utf_cstring_to_utf8(&path_utf8, opt_arg, pool);
> > opt_state.config_dir = svn_path_canonicalize(path_utf8, pool);
> > break;
> > + case opt_config_options:
> > + {
> > + config_option_t *config_option;
> > + int i, e;
> > + svn_boolean_t v = FALSE;
>
> Please use longer variable names than 'e' and 'v'.
>
> The code is very hard to read as it is. (However, in this case I think
> you can drop them entirely; see below)
>
> > + apr_size_t len = strlen(opt_arg);
> > + /* 6 is the length of "x:x:x=". */
> > + if (len < 6)
> > + {
> > + err = svn_error_create
> > + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Invalid syntax of argument of --config-option"));
> > + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> > + }
> > + config_option = apr_pcalloc(pool, sizeof(config_option_t));
> > + config_option->option = apr_pcalloc(pool, len);
> > + config_option->value = apr_pcalloc(pool, len);
> > + e = 0;
> > + for (i = 0; i < len; i++)
> > + {
> > + if (opt_arg[i] == ':')
> > + {
> > + break;
> > + }
> > + }
>
> Use strchr()? (see below)
>
> > + if (i == e)
> > + {
> > + err = svn_error_create
> > + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Invalid syntax of argument of --config-option"));
> > + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> > + }
> > + config_option->file = apr_pcalloc(pool, i - e + 1);
> > + memcpy(config_option->file, opt_arg + e, i -e);
>
> Use apr_pstrndup() ? (see below)
>
> > + if (i < len -1)
> > + {
> > + i++;
> > + }
> > + else
> > + {
> > + err = svn_error_create
> > + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Invalid syntax of argument of --config-option"));
> > + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> > + }
> > + e = i;
> > + for (; i < len; i++)
> > + {
> > + if (opt_arg[i] == ':')
> > + {
> > + break;
> > + }
> > + }
> > + if (i == e)
> > + {
> > + err = svn_error_create
> > + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Invalid syntax of argument of --config-option"));
> > + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> > + }
> > + config_option->section = apr_pcalloc(pool, i - e + 1);
> > + memcpy(config_option->section, opt_arg + e, i - e);
> > + if (i < len -1)
> > + {
> > + i++;
> > + }
> > + else
> > + {
> > + err = svn_error_create
> > + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Invalid syntax of argument of --config-option"));
> > + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> > + }
> > + e = i;
> > + for (; i < len; i++)
> > + {
> > + if (opt_arg[i] == '=')
> > + {
> > + v = TRUE;
> > + break;
> > + }
> > + else if (opt_arg[i] == ':')
> > + {
> > + break;
> > + }
> > + }
> > + if ((i == e) || (v == FALSE))
> > + {
> > + err = svn_error_create
> > + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Invalid syntax of argument of --config-option"));
> > + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> > + }
>
> It looks very complicated. Could you simplify it a little? For example,
>
> function(const char *opt_arg)
> {
> const char *first_colon, *second_colon, *equal_sign;
>
> if (first_colon = strchr(opt_arg, ':'))
> if (second_colon = strchr(first_colon, ':'))
> if (equal_sign = strchr(second_colon, '='))
> {
> /* allocate and return a config_option_t (with apr_pstrndup) */
> }
>
> return error;
> }
>
> ?
>
> > /* Update the options in the config */
> > + for (i = 0; i < opt_state.config_options->nelts; i++)
> > + {
> > + config_option_t *config_option = APR_ARRAY_IDX(opt_state.config_options,
> > + i, config_option_t *);
> > + if (strcmp(config_option->file, "config") == 0)
> > + {
> > + svn_config_set(cfg_config, config_option->section,
> > + config_option->option, config_option->value);
> > + }
> > + else if(strcmp(config_option->file, "servers") == 0)
> > + {
> > + svn_config_set(cfg_servers, config_option->section,
> > + config_option->option, config_option->value);
> > + }
>
> else
> {
> /* print a warning */
> }
>
> ?
>
> > + }
> > +

I implemented the majority of these suggestions in r34975.

-- 
Arfrever Frehtes Taifersar Arahesis
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=996136