threads / patch / 51775

patchhelp: make help_unknown_ref() NORETURN

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

## tl;dr

4 messages between Aug 29, 2019 and Aug 30, 2019. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

René Scharfe· Aug 29, 2019, 19:13 UTC · lore

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

Show changes to 2 files +3 −3

help.c, help.h

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

Martin Ågren· Aug 29, 2019, 19:40 UTC · re: René Scharfe · lore

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

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

René Scharfe· Aug 29, 2019, 20:08 UTC · re: Martin Ågren · lore

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

Am 29.08.19 um 21:40 schrieb Martin Ågren:

Show 15 quoted lines

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

Martin Ågren· Aug 30, 2019, 03:50 UTC · re: René Scharfe · lore

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

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

Show 28 quoted lines

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

Show 7 quoted lines

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