Re: [PATCH v5 5/8] t/helper/test-config: unify exit labels

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
> test-config's main function has three different exit labels, all of
> which have to perform the same cleanup code before returning. Unify the
> labels in preparation for the next patch which will increase the cleanup
> section.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> @@ -69,16 +69,19 @@ static int early_config_cb(const char *var, const char *value, void *vdata)
> +#define TC_VALUE_NOT_FOUND 1
> +#define TC_CONFIG_FILE_ERROR 2
> +
>  int cmd__config(int argc, const char **argv)
>  {
> +       int i, val, ret = 0;
>
>         if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
>                 read_early_config(early_config_cb, (void *)argv[2]);
> -               return 0;
> +               return ret;
>         }

This one change feels both fragile and increases cognitive load, thus
does not seem desirable. It feels fragile because someone could come
along and insert code above this conditional which assigns some value
other than 0 to 'ret' (not realizing that this conditional wants to
return 0), thus breaking it. It increases cognitive load because it
forces the reader to scan all the code leading up to this point to
determine what value this conditional really wants to return.

Nevertheless, this is a minor objection, not necessarily worth a re-roll.

> @@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv)
>                         printf("Value not found for \"%s\"\n", argv[2]);
> -                       goto exit1;
> +                       ret = TC_VALUE_NOT_FOUND;

This sort of change, on the other hand, does not increase cognitive
load because it's quite obvious what return value this conditional
wants to return (because it's assigning it explicitly).