git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
MÅMartin Ågren <martin.agren@gmail.com>
Date
Aug 30, 2019, 03:50 UTC
Message-ID
<CAN0heSo6YdCn2cHUTiyhuZ3Z_Rk6YFg=yhWCVHhaoAC3_0=0Xg@mail.gmail.com>
In-Reply-To
<bde881b6-7c23-6f4e-5cb0-a793a5e4f5d7@web.de>
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
Previous: René Scharfe
Message 4 of 4 in “help: make help_unknown_ref() NORETURN”
  1. help: make help_unknown_ref() NORETURNRené Scharfe, Aug 29, 2019
  2. Martin ÅgrenAug 29, 2019
  3. René ScharfeAug 29, 2019
  4. Martin ÅgrenAug 30, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.