Re: [PATCH v2] builtins + test helpers: use return instead of exit() in cmd_*

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 8, 2021, 23:55 UTC
Message-ID: <xmqqwnr3nc7i.fsf@gitster.g>
In-Reply-To: <patch-1.1-f225b78e01-20210608T104454Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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.

Show 12 quoted lines

> 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 <avarab@gmail.com>
> ---
>
> 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.