{"thread":{"id":"51775","subject":"[PATCH] help: make help_unknown_ref() NORETURN","startedAt":"2019-08-29T19:13:22Z","lastAt":"2019-08-30T03:50:23Z","messageCount":4,"participants":["René Scharfe","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381537","messageId":"d0606d19-0900-3908-2962-ceb24015f753@web.de","threadId":"51775","inReplyTo":null,"subject":"[PATCH] help: make help_unknown_ref() NORETURN","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-29T19:13:16Z","receivedAt":"2019-08-29T19:13:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Announce that calling help_unknown_ref() exits the program.\n\nSigned-off-by: René Scharfe \n---\nPatch generated with --function-context for easier review.\n\n help.c | 3 ++-\n help.h | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 5261d83ecf..9ff2be6b18 100644\n--- a/help.c\n+++ b/help.c\n@@ -774,22 +774,23 @@ static struct string_list guess_refs(const char *ref)\n \treturn similar_refs;\n }\n\n-void help_unknown_ref(const char *ref, const char *cmd, const char *error)\n+NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n+\t\t\t const char *error)\n {\n \tint i;\n \tstruct string_list suggested_refs = guess_refs(ref);\n\n \tfprintf_ln(stderr, _(\"%s: %s - %s\"), cmd, ref, error);\n\n \tif (suggested_refs.nr > 0) {\n \t\tfprintf_ln(stderr,\n \t\t\t Q_(\"\\nDid you mean this?\",\n \t\t\t \"\\nDid you mean one of these?\",\n \t\t\t suggested_refs.nr));\n \t\tfor (i = 0; i < suggested_refs.nr; i++)\n \t\t\tfprintf(stderr, \"\\t%s\\n\", suggested_refs.items[i].string);\n \t}\n\n \tstring_list_clear(&suggested_refs, 0);\n \texit(1);\n }\ndiff --git a/help.h b/help.h\nindex b8780fbd0f..7a455beeb7 100644\n--- a/help.h\n+++ b/help.h\n@@ -42,8 +42,8 @@ void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdn\n /*\n * call this to die(), when it is suspected that the user mistyped a\n * ref to the command, to give suggested \"correct\" refs.\n */\n-void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n+NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n\n static inline void list_config_item(struct string_list *list,\n \t\t\t\t const char *prefix,\n--\n2.23.0\n"},{"id":"381538","messageId":"CAN0heSp4bdJt4NKELOkvdpoygbkNZVn-96FrZnu9ymE5UNOSag@mail.gmail.com","threadId":"51775","inReplyTo":"d0606d19-0900-3908-2962-ceb24015f753@web.de","subject":"Re: [PATCH] help: make help_unknown_ref() NORETURN","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-29T19:40:39Z","receivedAt":"2019-08-29T19:40:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 29 Aug 2019 at 21:15, René Scharfe wrote:\n>\n> Announce that calling help_unknown_ref() exits the program.\n>\n> Signed-off-by: René Scharfe \n> ---\n> Patch generated with --function-context for easier review.\n>\n> help.c | 3 ++-\n> help.h | 2 +-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/help.c b/help.c\n> index 5261d83ecf..9ff2be6b18 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -774,22 +774,23 @@ static struct string_list guess_refs(const char *ref)\n> return similar_refs;\n> }\n>\n> -void help_unknown_ref(const char *ref, const char *cmd, const char *error)\n> +NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n> + const char *error)\n> {\n> int i;\n> struct string_list suggested_refs = guess_refs(ref);\n>\n> fprintf_ln(stderr, _(\"%s: %s - %s\"), cmd, ref, error);\n>\n> if (suggested_refs.nr > 0) {\n> fprintf_ln(stderr,\n> Q_(\"\\nDid you mean this?\",\n> \"\\nDid you mean one of these?\",\n> suggested_refs.nr));\n> for (i = 0; i < suggested_refs.nr; i++)\n> fprintf(stderr, \"\\t%s\\n\", suggested_refs.items[i].string);\n> }\n>\n> string_list_clear(&suggested_refs, 0);\n> exit(1);\n> }\n\nIndeed, this function always ends up exit()-ing.\n\n> diff --git a/help.h b/help.h\n> index b8780fbd0f..7a455beeb7 100644\n> --- a/help.h\n> +++ b/help.h\n> @@ -42,8 +42,8 @@ void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdn\n> /*\n> * call this to die(), when it is suspected that the user mistyped a\n> * ref to the command, to give suggested \"correct\" refs.\n> */\n> -void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n> +NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n\nFunny how this claims we'll call `die()`, when we'll actually call\n`exit(1)`. If we actually did call `die()`, I suppose the compiler\nshould/could figure out by itself that this function, too, won't ever\nreturn.\n\nI wonder whether the real bug here is that the implementation calls\n`exit(1)`, not `die()`. That is, the exit code is wrong (1 != 128) and\nwe're missing out on the flexibility offered by `set_die_routine()`. If\nnot that, then I'd say the documentation is buggy. Hm?\n\nIn any case, your patch seems correct. Just wondering what should be\ndone on top of it...\n\nMartin\n"},{"id":"381540","messageId":"bde881b6-7c23-6f4e-5cb0-a793a5e4f5d7@web.de","threadId":"51775","inReplyTo":"CAN0heSp4bdJt4NKELOkvdpoygbkNZVn-96FrZnu9ymE5UNOSag@mail.gmail.com","subject":"Re: [PATCH] help: make help_unknown_ref() NORETURN","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-29T20:08:18Z","receivedAt":"2019-08-29T20:08:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.08.19 um 21:40 schrieb Martin Ågren:\n> On Thu, 29 Aug 2019 at 21:15, René Scharfe wrote:\n>> diff --git a/help.h b/help.h\n>> index b8780fbd0f..7a455beeb7 100644\n>> --- a/help.h\n>> +++ b/help.h\n>> @@ -42,8 +42,8 @@ void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdn\n>> /*\n>> * call this to die(), when it is suspected that the user mistyped a\n>> * ref to the command, to give suggested \"correct\" refs.\n>> */\n>> -void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n>> +NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n>\n> Funny how this claims we'll call `die()`, when we'll actually call\n> `exit(1)`.\n\nAh, I didn't even notice that.\n\n> If we actually did call `die()`, I suppose the compiler\n> should/could figure out by itself that this function, too, won't ever\n> return.\n\nThe compiler can figure it out with exit(), too; system headers (at\nleast for glibc, but it's probably common) assign it the noreturn\nattribute. But there is no way to transmit that information to callers\nacross compilation units if not for the header file, right?\n\n> I wonder whether the real bug here is that the implementation calls\n> `exit(1)`, not `die()`. That is, the exit code is wrong (1 != 128) and\n> we're missing out on the flexibility offered by `set_die_routine()`. If\n> not that, then I'd say the documentation is buggy. Hm?\n\nThis inconsistency has been present since e56181060e (\"help: add\nhelp_unknown_ref()\", 2013-05-04). Using die() is going to be difficult\ndue to the multi-line suggestions printed by the function.\n\nRené\n"},{"id":"381560","messageId":"CAN0heSo6YdCn2cHUTiyhuZ3Z_Rk6YFg=yhWCVHhaoAC3_0=0Xg@mail.gmail.com","threadId":"51775","inReplyTo":"bde881b6-7c23-6f4e-5cb0-a793a5e4f5d7@web.de","subject":"Re: [PATCH] help: make help_unknown_ref() NORETURN","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-30T03:50:09Z","receivedAt":"2019-08-30T03:50:23Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 29 Aug 2019 at 22:08, René Scharfe wrote:\n>\n> Am 29.08.19 um 21:40 schrieb Martin Ågren:\n> > On Thu, 29 Aug 2019 at 21:15, René Scharfe wrote:\n> >> diff --git a/help.h b/help.h\n> >> index b8780fbd0f..7a455beeb7 100644\n> >> --- a/help.h\n> >> +++ b/help.h\n> >> @@ -42,8 +42,8 @@ void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdn\n> >> /*\n> >> * call this to die(), when it is suspected that the user mistyped a\n> >> * ref to the command, to give suggested \"correct\" refs.\n> >> */\n> >> -void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n> >> +NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n> >\n> > Funny how this claims we'll call `die()`, when we'll actually call\n> > `exit(1)`.\n>\n> Ah, I didn't even notice that.\n>\n> > If we actually did call `die()`, I suppose the compiler\n> > should/could figure out by itself that this function, too, won't ever\n> > return.\n>\n> The compiler can figure it out with exit(), too; system headers (at\n> least for glibc, but it's probably common) assign it the noreturn\n> attribute. But there is no way to transmit that information to callers\n> across compilation units if not for the header file, right?\n\nOf course, you're right.\n\n> > I wonder whether the real bug here is that the implementation calls\n\n(Re-reading this, \"the real bug\" might be a bit of a harsh statement. I\ndidn't mean to imply that this patch does not fix an actual problem.)\n\n> > `exit(1)`, not `die()`. That is, the exit code is wrong (1 != 128) and\n> > we're missing out on the flexibility offered by `set_die_routine()`. If\n> > not that, then I'd say the documentation is buggy. Hm?\n>\n> This inconsistency has been present since e56181060e (\"help: add\n> help_unknown_ref()\", 2013-05-04). Using die() is going to be difficult\n> due to the multi-line suggestions printed by the function.\n\nYeah, that's true. We could manually prefix each line with \"fatal: \" or\n\"error: \", then die with something like \"see above\", which is not very\ncool, or die(\"%s\", error), which is a bit repetitive. There are a few\ndecisions to be made for fixing this discrepancy.\n\nAnyway, I don't think this is something that needs to hold up your\npatch.\n\nMartin\n"}]}