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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 2, 2020, 07:30 UTC
Message-ID: <CAPig+cSZFOXuLBKBLrNkZ=dTmQUrp-QiUb7wYGoza-YmEFiM9A@mail.gmail.com>
In-Reply-To: <56535b0e36e94dc73aa570f4f3c0466305c6432f.1599026986.git.matheus.bernardino@usp.br>

On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares <matheus.bernardino@usp.br> wrote:

Show 21 quoted lines

> 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).