On 4/5/06, Kamesh Jayachandran <kamesh@collab.net> wrote:
Thanks for the patch! A few comments:
>
> Index: subversion/tests/svn_test_main.c
> ===================================================================
> --- subversion/tests/svn_test_main.c (revision 19185)
> +++ subversion/tests/svn_test_main.c (working copy)
> @@ -26,6 +26,8 @@
> #include <apr_general.h>
> #include <apr_lib.h>
>
> +#include "svn_cmdline.h"
> +#include "svn_opt.h"
> #include "svn_pools.h"
> #include "svn_error.h"
> #include "svn_test.h"
> @@ -46,7 +48,25 @@
> /* Test option: Remove test directories after success */
> static int cleanup_mode = 0;
>
> +typedef enum {
> + cleanup_opt = SVN_OPT_FIRST_LONGOPT_ID,
> + fstype_opt,
> + list_opt,
> + verbose_opt
> +};
If you're going to typedef it, you need to typedef it to something.
So either drop the typedef, or give it a name after the }.
> +const apr_getopt_option_t cl_options[] =
> +{
> + {"cleanup", cleanup_opt, 0,
> + N_("remove test directories after success")},
> + {"fs-type", fstype_opt, 1,
> + N_("specify a fs-type ARG")},
> + {"list", list_opt, 0,
> + N_("lists all the tests with their short description")},
> + {"verbose", verbose_opt, 0,
> + N_("print extra information")},
> + {0, 0, 0, 0}
> +};
This should be static.
> /* ================================================================= */
> /* Stuff for cleanup processing */
>
> @@ -208,6 +228,11 @@
> apr_pool_t *pool, *test_pool;
> int ran_a_test = 0;
> char **arg;
> + int list_mode = 0;
> + int opt_id;
> + apr_status_t apr_err;
> + apr_getopt_t *os;
> + svn_error_t *err;
> /* How many tests are there? */
> int array_size = get_array_size();
>
> @@ -244,15 +269,37 @@
> test_argc = argc;
> test_argv = argv;
>
> - /* Scan the command line for the --verbose and --cleanup flags */
> - for (arg = &argv[1]; *arg; ++arg)
> + err = svn_cmdline__getopt_init(&os, pool, argc, argv);
> + if (err)
> + return svn_cmdline_handle_exit_error(err, pool, prog_name);
> + while (1)
> {
> - if (strcmp(*arg, "--cleanup") == 0)
> - cleanup_mode = 1;
> - else if (strcmp(*arg, "--verbose") == 0)
> - verbose_mode = 1;
> - else if (strncmp(*arg, "--fs-type=", 10) == 0)
> - opts.fs_type = apr_pstrdup(pool, (*arg) + 10);
> + const char *opt_arg;
> +
> + /* Parse the next option. */
> + apr_err = apr_getopt_long(os, cl_options, &opt_id, &opt_arg);
> + if (APR_STATUS_IS_EOF(apr_err))
> + break;
> + else if (apr_err)
> + {
> + printf("apr_getopt_long() failed.\n");
> + exit(1);
Why did it fail? Perhaps apr_strerror would be useful here.
> + }
> +
> + switch (opt_id) {
> + case cleanup_opt:
> + cleanup_mode = 1;
> + break;
> + case fstype_opt:
> + opts.fs_type= apr_pstrdup(pool, opt_arg);
Need a space before the =.
> + break;
> + case list_opt:
> + list_mode = 1;
> + break;
> + case verbose_opt:
> + verbose_mode = 1;
> + break;
> + }
> }
>
> /* Create an iteration pool for the tests */
> @@ -261,7 +308,7 @@
>
> if (argc >= 2) /* notice command-line arguments */
> {
> - if (! strcmp(argv[1], "list"))
> + if (! strcmp(argv[1], "list") || list_mode)
> {
> ran_a_test = 1;
>
> @@ -294,11 +341,6 @@
> svn_pool_clear(test_pool);
> svn_pool_clear(cleanup_pool);
> }
> - else if (argv[i][0] != '-')
> - {
> - /* (probably) a source directory pathname */
> - printf("notice: ignoring argument %d: '%s'\n", i, argv[i]);
> - }
> }
> }
> }
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Apr 5 22:24:51 2006