Re: [PATCH] help: make help_unknown_ref() NORETURN

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 29, 2019, 19:40 UTC
Message-ID: <CAN0heSp4bdJt4NKELOkvdpoygbkNZVn-96FrZnu9ymE5UNOSag@mail.gmail.com>
In-Reply-To: <d0606d19-0900-3908-2962-ceb24015f753@web.de>

On Thu, 29 Aug 2019 at 21:15, René Scharfe <l.s.r@web.de> wrote:

Show 40 quoted lines

>
> Announce that calling help_unknown_ref() exits the program.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Patch generated with --function-context for easier review.
>
>  help.c | 3 ++-
>  help.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/help.c b/help.c
> index 5261d83ecf..9ff2be6b18 100644
> --- a/help.c
> +++ b/help.c
> @@ -774,22 +774,23 @@ static struct string_list guess_refs(const char *ref)
>         return similar_refs;
>  }
>
> -void help_unknown_ref(const char *ref, const char *cmd, const char *error)
> +NORETURN void help_unknown_ref(const char *ref, const char *cmd,
> +                              const char *error)
>  {
>         int i;
>         struct string_list suggested_refs = guess_refs(ref);
>
>         fprintf_ln(stderr, _("%s: %s - %s"), cmd, ref, error);
>
>         if (suggested_refs.nr > 0) {
>                 fprintf_ln(stderr,
>                            Q_("\nDid you mean this?",
>                               "\nDid you mean one of these?",
>                               suggested_refs.nr));
>                 for (i = 0; i < suggested_refs.nr; i++)
>                         fprintf(stderr, "\t%s\n", suggested_refs.items[i].string);
>         }
>
>         string_list_clear(&suggested_refs, 0);
>         exit(1);
>  }

Indeed, this function always ends up exit()-ing.

Show 11 quoted lines

> diff --git a/help.h b/help.h
> index b8780fbd0f..7a455beeb7 100644
> --- a/help.h
> +++ b/help.h
> @@ -42,8 +42,8 @@ void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdn
>  /*
>   * call this to die(), when it is suspected that the user mistyped a
>   * ref to the command, to give suggested "correct" refs.
>   */
> -void help_unknown_ref(const char *ref, const char *cmd, const char *error);
> +NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error);

Funny how this claims we'll call `die()`, when we'll actually call `exit(1)`. If we actually did call `die()`, I suppose the compiler should/could figure out by itself that this function, too, won't ever return.

I wonder whether the real bug here is that the implementation calls `exit(1)`, not `die()`. That is, the exit code is wrong (1 != 128) and we're missing out on the flexibility offered by `set_die_routine()`. If not that, then I'd say the documentation is buggy. Hm?

In any case, your patch seems correct. Just wondering what should be done on top of it...

Martin