git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] help: make help_unknown_ref() NORETURN
@ 2019-08-29 19:13 René Scharfe
  2019-08-29 19:40 ` Martin Ågren
  0 siblings, 1 reply; 4+ messages in thread
From: René Scharfe @ 2019-08-29 19:13 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] help: make help_unknown_ref() NORETURN
  2019-08-29 19:13 [PATCH] help: make help_unknown_ref() NORETURN René Scharfe
@ 2019-08-29 19:40 ` Martin Ågren
  2019-08-29 20:08   ` René Scharfe
  0 siblings, 1 reply; 4+ messages in thread
From: Martin Ågren @ 2019-08-29 19:40 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git Mailing List, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] help: make help_unknown_ref() NORETURN
  2019-08-29 19:40 ` Martin Ågren
@ 2019-08-29 20:08   ` René Scharfe
  2019-08-30  3:50     ` Martin Ågren
  0 siblings, 1 reply; 4+ messages in thread
From: René Scharfe @ 2019-08-29 20:08 UTC (permalink / raw)
  To: Martin Ågren; +Cc: Git Mailing List, Junio C Hamano

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é

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] help: make help_unknown_ref() NORETURN
  2019-08-29 20:08   ` René Scharfe
@ 2019-08-30  3:50     ` Martin Ågren
  0 siblings, 0 replies; 4+ messages in thread
From: Martin Ågren @ 2019-08-30  3:50 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git Mailing List, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-08-30  3:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-29 19:13 [PATCH] help: make help_unknown_ref() NORETURN René Scharfe
2019-08-29 19:40 ` Martin Ågren
2019-08-29 20:08   ` René Scharfe
2019-08-30  3:50     ` Martin Ågren

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).