[PATCH] help: make help_unknown_ref() NORETURN

[PATCH] help: make help_unknown_ref() NORETURN

[René Scharfe]

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);
 }
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);

 static inline void list_config_item(struct string_list *list,
 				    const char *prefix,
--
2.23.0

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

[Martin Ågren]

On Thu, 29 Aug 2019 at 21:15, René Scharfe <l.s.r@web.de> wrote:
>
> 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.

> 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

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

[René Scharfe]

Am 29.08.19 um 21:40 schrieb Martin Ågren:
> On Thu, 29 Aug 2019 at 21:15, René Scharfe <l.s.r@web.de> wrote:
>> 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)`.

Ah, I didn't even notice that.

> If we actually did call `die()`, I suppose the compiler
> should/could figure out by itself that this function, too, won't ever
> return.

The compiler can figure it out with exit(), too; system headers (at
least for glibc, but it's probably common) assign it the noreturn
attribute.  But there is no way to transmit that information to callers
across compilation units  if not for the header file, right?

> 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?

This inconsistency has been present since e56181060e ("help: add
help_unknown_ref()", 2013-05-04).  Using die() is going to be difficult
due to the multi-line suggestions printed by the function.

René

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

[Martin Ågren]

On Thu, 29 Aug 2019 at 22:08, René Scharfe <l.s.r@web.de> wrote:
>
> Am 29.08.19 um 21:40 schrieb Martin Ågren:
> > On Thu, 29 Aug 2019 at 21:15, René Scharfe <l.s.r@web.de> wrote:
> >> 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)`.
>
> Ah, I didn't even notice that.
>
> > If we actually did call `die()`, I suppose the compiler
> > should/could figure out by itself that this function, too, won't ever
> > return.
>
> The compiler can figure it out with exit(), too; system headers (at
> least for glibc, but it's probably common) assign it the noreturn
> attribute.  But there is no way to transmit that information to callers
> across compilation units  if not for the header file, right?

Of course, you're right.

> > I wonder whether the real bug here is that the implementation calls

(Re-reading this, "the real bug" might be a bit of a harsh statement. I
didn't mean to imply that this patch does not fix an actual problem.)

> > `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?
>
> This inconsistency has been present since e56181060e ("help: add
> help_unknown_ref()", 2013-05-04).  Using die() is going to be difficult
> due to the multi-line suggestions printed by the function.

Yeah, that's true. We could manually prefix each line with "fatal: " or
"error: ", then die with something like "see above", which is not very
cool, or die("%s", error), which is a bit repetitive. There are a few
decisions to be made for fixing this discrepancy.

Anyway, I don't think this is something that needs to hold up your
patch.

Martin