Re: [PATCH] poke: Allow parse_args_1 to exit

[Jose E. Marchesi]

Hi Arsen.
Thanks for the patch.

> This prevents double "unknown options" being printed.
> ---
> Evening,
>
> This patch makes all options explicitly handled in both getopt_long
> switch-cases, and allows parse_args_1 to exit immediately on unknown options.
>
> As proof:
>
>   [i] ~/poke/poke/build$ ./run poke -h
>   /home/arsen/poke/poke/build/poke/.libs/poke: invalid option -- 'h'
>   [i] ~/poke/poke/build 1 $ 
>
> OK for master?  Tested on x86_64-pc-linux-gnu, also by listing all possible
> values in an extra block, to see if GCC detects any missing cases.

OK for master.

> Also, maybe we should also add an unreachable assert in parse_args_2's
> case '?' (because, in theory, after parse_args_1 finishes, all args
> should be parsed, AFAICT)?

That would introduce unnecessary coupling IMO.

>
> Thanks, have a great night.
>
>  ChangeLog   |  9 +++++++++
>  poke/poke.c | 37 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 7b8ef89e..ec9da381 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,12 @@
> +2022-11-24  Arsen Arsenović  <arsen@aarsen.me>
> +
> +     * poke/poke.c (poke_getopt_string): Extract the getopt string, to
> +     avoid repetition.
> +     (parse_args_1): Use common poke_getopt_string, assert out on
> +     unhandled option, and exit on unknown option.
> +     (parse_args_2): Use common poke_getopt_string, assert out on
> +     unhandled option.
> +
>  2022-11-24  Agathe Porte  <floss@microjoe.org>
>  
>       * doc/poke.texi: fix sbm image bytes in example.
> diff --git a/poke/poke.c b/poke/poke.c
> index a13d2260..fdc825db 100644
> --- a/poke/poke.c
> +++ b/poke/poke.c
> @@ -158,6 +158,9 @@ static const struct option long_options[] =
>    {NULL, 0, NULL, 0},
>  };
>  
> +/* String passed to getopt_long.  */
> +static const char *const poke_getopt_string = "ql:c:s:L:";
> +
>  static void
>  print_help (void)
>  {
> @@ -351,7 +354,7 @@ parse_args_1 (int argc, char *argv[])
>  
>    while ((ret = getopt_long (argc,
>                               argv,
> -                             "ql:c:s:L:",
> +                             poke_getopt_string,
>                               long_options,
>                               NULL)) != -1)
>      {
> @@ -379,7 +382,25 @@ parse_args_1 (int argc, char *argv[])
>          case NO_INIT_FILE_ARG:
>            poke_load_init_file = 0;
>            break;
> +        case '?':
> +          exit (EXIT_FAILURE);
> +          break;
> +        case QUIET_ARG:
> +        case LOAD_ARG:
> +        case LOAD_AND_EXIT_ARG:
> +        case CMD_ARG:
> +        case SOURCE_ARG:
> +        case COLOR_ARG:
> +        case STYLE_ARG:
> +        case STYLE_BRIGHT_ARG:
> +        case STYLE_DARK_ARG:
> +        case 'c':
> +        case 'l':
> +        case 's':
> +          /* Handled in parse_args_2.  */
> +          break;
>          default:
> +          assert (!"Did you forget to add a CASE for a flag in 
> parse_args_1?");
>            break;
>          }
>      }
> @@ -394,7 +415,7 @@ parse_args_2 (int argc, char *argv[])
>    optind = 1;
>    while ((ret = getopt_long (argc,
>                               argv,
> -                             "ql:c:s:L:",
> +                             poke_getopt_string,
>                               long_options,
>                               NULL)) != -1)
>      {
> @@ -464,6 +485,13 @@ parse_args_2 (int argc, char *argv[])
>          case NO_INIT_FILE_ARG:
>            /* These are handled in parse_args_1.  */
>            break;
> +        case HELP_ARG:
> +        case VERSION_ARG:
> +          /* Handled in parse_args_1.  */
> +          break;
> +        case LOAD_AND_EXIT_ARG:
> +          /* Handled in set_script_args.  */
> +          break;
>            /* libtextstyle arguments are handled in pk-term.c, not
>               here.   */
>          case COLOR_ARG:
> @@ -471,8 +499,11 @@ parse_args_2 (int argc, char *argv[])
>          case STYLE_DARK_ARG:
>          case STYLE_BRIGHT_ARG:
>            break;
> -        default:
> +        case '?':
>            goto exit_failure;
> +        default:
> +          assert (!"Did you forget to add a case for a flag in 
> parse_args_2?");
> +          break;
>          }
>      }