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

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

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

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

> @@ -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, ...)
>  {