Re: [PATCH 1/4] usage.c: add a die_message() routine

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2021, 19:42 UTC
Message-ID: <xmqqee6pfrkd.fsf@gitster.g>
In-Reply-To: <patch-1.4-5a9ab85fa56-20211206T165221Z-avarab@gmail.com>

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

> +int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));

This probalby makes sense (we'll need to see the caller first).

Show 5 quoted lines

> +static void die_message_builtin(const char *err, va_list params)
> +{
> +	trace2_cmd_error_va(err, params);
> +	vreportf("fatal: ", err, params);
> +}

I thought the convention was *not* that we name the variant that takes va_list with _builtin suffix, rather, I thought that the convention is that ones with the _builtin suffix are meant to be override-able by other code.

Show 14 quoted lines

>  /*
>   * We call trace2_cmd_error_va() in the below functions first and
>   * expect it to va_copy 'params' before using it (because an 'ap' can
> @@ -62,10 +68,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
>   */
>  static NORETURN void die_builtin(const char *err, va_list params)
>  {
> -	trace2_cmd_error_va(err, params);
> -
> -	vreportf("fatal: ", err, params);
> -
> +	die_message_builtin(err, params);
>  	exit(128);
>  }

And by making die() and die_message() both override-able, doesn't it cause confusion on the caller's end which one should get replaced? Or with die_message being overridable, we should rewrite the ones that override die() to instead override die_message()?

It also makes readers wonder why this is not

	exit(die_message(err, params));

which I take it a sign that this new API is overly loose to allow a simple single thing to be done in multiple ways. Perhaps as the series progresses, the picture might improve, but if that is the case, perhaps the presentation order needs to be rethought. E.g. start without the _builtin that implies override-ability, convert the existing code that can benefit from calling die_message(), and then finally introduce _builtin that is merely an implementation detail, or something like that, perhaps?

In any case, the first step in this four patch series is not enough to evaluate if this step makes sense, so let's keep reading.

Show 14 quoted lines

> @@ -211,6 +214,17 @@ void NORETURN die_errno(const char *fmt, ...)
>  	va_end(params);
>  }
>  
> +#undef die_message
> +int die_message(const char *err, ...)
> +{
> +	va_list params;
> +
> +	va_start(params, err);
> +	die_message_builtin(err, params);
> +	va_end(params);
> +	return 128;
> +}

OK.

Thanks.

>  #undef error_errno
>  int error_errno(const char *fmt, ...)
>  {