list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: "Ævar Arnfjörð Bjarmason" <>
	Felipe Contreras <>,
	Jeff King <>
Subject: Re: [PATCH v2] builtins + test helpers: use return instead of exit() in cmd_*
Date: Wed, 09 Jun 2021 08:55:29 +0900	[thread overview]
Message-ID: <xmqqwnr3nc7i.fsf@gitster.g> (raw)
In-Reply-To: <> (=?utf-8?B?IsOGdmFyCUFybmZqw7Zyw7A=?= Bjarmason"'s message of "Tue, 8 Jun 2021 12:48:03 +0200")

Ævar Arnfjörð Bjarmason  <> writes:

> Change various cmd_* functions that claim no return an "int" to use

s/no return/to return/

> "return" instead of exit() to indicate an exit code. These were not
> marked with NORETURN,

Up to this point, it is well written.

> and by directly exit()-ing we'll skip the
> cleanup git.c would otherwise do (e.g. closing fd's, erroring if we
> can't). See run_builtin() in git.c.

But I think this is a hyperbole.  File descritors are closed when we
exit without git.c's help, thank-you-very-much ;-), and if we do
have clean-ups that are truly important, we would have arranged them
to happen in the atexit handler, so it is not a crime for functions
called from the subcommand dispatchers to exit themselves (as long
as they exit sensibly, e.g. without doing nonsense like exit(-1)).

It nevertheless is a good idea because it encourages good code
hygiene, just like marking with NORETURN if the function must exit.
Selling this change as if it were a correctness fix (i.e. we were
exiting and missed these important clean-ups that the caller wanted
to do after we return) is misleading.

> In the case of shell.c and sh-i18n--envsubst.c this was the result of
> an incomplete migration to using a cmd_main() in 3f2e2297b9 (add an
> extra level of indirection to main(), 2016-07-01).
> This was spotted by SunCC 12.5 on Solaris 10 (gcc210 on the gccfarm).
> Signed-off-by: Ævar Arnfjörð Bjarmason <>
> ---
> Clarified the commit message, and made the same s/exit/return/g change
> in shell.c and sh-i18n--envsubst.c. I also missed an "exit(2)" in a
> brach in builtin/merge-ours.c.

The range diff looks good to me.  Thanks.

  reply	other threads:[~2021-06-08 23:55 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-07 11:12 [PATCH] " Ævar Arnfjörð Bjarmason
2021-06-07 17:02 ` Felipe Contreras
2021-06-08  6:49 ` Jeff King
2021-06-08 10:53   ` Ævar Arnfjörð Bjarmason
2021-06-10 13:16   ` Phillip Wood
2021-06-10 13:19     ` Ævar Arnfjörð Bjarmason
2021-06-08 10:48 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2021-06-08 23:55   ` Junio C Hamano [this message]
2021-06-09  1:54     ` Ævar Arnfjörð Bjarmason
2021-06-09  3:38       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqwnr3nc7i.fsf@gitster.g \ \ \ \ \ \
    --subject='Re: [PATCH v2] builtins + test helpers: use return instead of exit() in cmd_*' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ \
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
 note: .onion URLs require Tor:

code repositories for project(s) associated with this inbox:

AGPL code for this site: git clone