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

[Junio C Hamano <gitster@pobox.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.

> 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.