[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 30 Dec 2008 01:25:50 +0200 (Jerusalem Standard Time)

(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?pathrev4967&r14966&r24967
> =============================================================================> --- 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 */
           }

?

> + }
> +

Still can't sleep,

Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=995351
Received on 2008-12-30 00:46:29 CET

This is an archived mail posted to the Subversion Dev mailing list.