Re: [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2022, 18:37 UTC
Message-ID: <xmqqv8tks00c.fsf@gitster.g>
In-Reply-To: <patch-v2-1.6-d446e4679d4-20220531T164806Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 14 quoted lines

> diff --git a/common-main.c b/common-main.c
> index 29fb7452f8a..bb0100f6024 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -56,9 +56,24 @@ int main(int argc, const char **argv)
>  	result = cmd_main(argc, argv);
>  
>  	/*
> -	 * We define exit() to call trace2_cmd_exit_fl() in
> -	 * git-compat-util.h. Whether we reach this or exit()
> +	 * We define exit() to call common_exit(), which will in turn
> +	 * call trace2_cmd_exit_fl(). Whether we reach this or exit()
>  	 * elsewhere we'll always run our trace2 exit handler.
>  	 */

I was expecting to see this comment to speak in a more generic terms (i.e. "after the main processing is done, we clean up after ourselves, either by exiting cmd_main and coming here or explicitly calling exit()"), now we have introduced common_exit().

Show 12 quoted lines

>  	exit(result);
>  }
> +
> +int common_exit(const char *file, int line, int code)
> +{
> +	/*
> +	 * For non-POSIX systems: Take the lowest 8 bits of the "code"
> +	 * to e.g. turn -1 into 255. On a POSIX system this is
> +	 * redundant, see exit(3) and wait(2), but as it doesn't harm
> +	 * anything there we don't need to guard this with an "ifdef".
> +	 */
> +	code &= 0xff;

And I expected that the trace2 specific comment, if it is still needed, would migrate here before this call.

Not huge enough to cause a reroll, but if there are other reasons to see an updated version, it would be good to fix it while at it.

> +	trace2_cmd_exit_fl(file, line, code);
> +
> +	return code;
> +}

Show 8 quoted lines

> +/**
> + * The exit() function is defined as common_exit() in
> + * git-compat-util.h.
> + *
> + * Intercepting the exit() allows us to hook in at-exit behavior, such
> + * emitting trace2 logging before calling the real exit().
> + */
> +int common_exit(const char *file, int line, int code);

Nicely explained.

Thanks.