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

[Junio C Hamano <gitster@pobox.com>]

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

> We have code in various places that would like to call die(), but
> wants to defer the exit(128) it would invoke, e.g. to print an
> additional message, or adjust the exit code. Add a die_message()
> helper routine to bridge this gap in the API.
>
> Functionally this behaves just like the error() routine, except it'll
> print a "fatal: " prefix, and it will exit with 128 instead of -1,

exit with -> return?

> this is so that caller can pas the return value to exit(128), instead
> of having to hardcode "128".

Is it just me or do your patch always have to do about the same
amount of seemingly unnecessary and/or unadvertised changes as the
necessary and/or advertised changes?  I agree that adding
die_message() that returns 128 after giving a message is an
excellent idea, and I can see that it is necessary and sufficient to
achieve the above advertised goal, but I don't see any reason why
set/get_message_routine() must exist, especially as a part of this
step.

IOW, perhaps that half of this patch belongs to and should be
squashed into one of the later steps of this series?

> A subsequent commit will migrate various callers that benefit from
> this function over to it, for now we're just migrating trivial users
> in usage.c itself.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  git-compat-util.h |  3 +++
>  usage.c           | 46 ++++++++++++++++++++++++++++++++++------------
>  2 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 141bb86351e..c1bb32460b6 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -471,6 +471,7 @@ NORETURN void usage(const char *err);
>  NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
> +int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> @@ -505,6 +506,8 @@ static inline int const_error(void)
>  typedef void (*report_fn)(const char *, va_list params);
>  
>  void set_die_routine(NORETURN_PTR report_fn routine);
> +void set_die_message_routine(report_fn routine);
> +report_fn get_die_message_routine(void);
>  void set_error_routine(report_fn routine);
>  report_fn get_error_routine(void);
>  void set_warn_routine(report_fn routine);
> diff --git a/usage.c b/usage.c
> index c7d233b0de9..3d4b90bce1f 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)
>  	exit(129);
>  }
>  
> +static void die_message_builtin(const char *err, va_list params)
> +{
> +	trace2_cmd_error_va(err, params);
> +	vreportf("fatal: ", err, params);
> +}
> +
>  /*
>   * 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,9 @@ 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);
> +	report_fn die_message_fn = get_die_message_routine();
>  
> +	die_message_fn(err, params);
>  	exit(128);
>  }
>  
> @@ -109,6 +114,7 @@ static int die_is_recursing_builtin(void)
>   * (ugh), so keep things static. */
>  static NORETURN_PTR report_fn usage_routine = usage_builtin;
>  static NORETURN_PTR report_fn die_routine = die_builtin;
> +static report_fn die_message_routine = die_message_builtin;
>  static report_fn error_routine = error_builtin;
>  static report_fn warn_routine = warn_builtin;
>  static int (*die_is_recursing)(void) = die_is_recursing_builtin;
> @@ -118,6 +124,16 @@ void set_die_routine(NORETURN_PTR report_fn routine)
>  	die_routine = routine;
>  }
>  
> +void set_die_message_routine(report_fn routine)
> +{
> +	die_message_routine = routine;
> +}
> +
> +report_fn get_die_message_routine(void)
> +{
> +	return die_message_routine;
> +}
> +
>  void set_error_routine(report_fn routine)
>  {
>  	error_routine = routine;
> @@ -157,14 +173,23 @@ void NORETURN usage(const char *err)
>  	usagef("%s", err);
>  }
>  
> +#undef die_message
> +int die_message(const char *err, ...)
> +{
> +	va_list params;
> +
> +	va_start(params, err);
> +	die_message_routine(err, params);
> +	va_end(params);
> +	return 128;
> +}
> +
>  void NORETURN die(const char *err, ...)
>  {
>  	va_list params;
>  
> -	if (die_is_recursing()) {
> -		fputs("fatal: recursion detected in die handler\n", stderr);
> -		exit(128);
> -	}
> +	if (die_is_recursing())
> +		exit(die_message("recursion detected in die handler"));
>  
>  	va_start(params, err);
>  	die_routine(err, params);
> @@ -200,11 +225,8 @@ void NORETURN die_errno(const char *fmt, ...)
>  	char buf[1024];
>  	va_list params;
>  
> -	if (die_is_recursing()) {
> -		fputs("fatal: recursion detected in die_errno handler\n",
> -			stderr);
> -		exit(128);
> -	}
> +	if (die_is_recursing())
> +		exit(die_message("recursion detected in die_errno handler"));
>  
>  	va_start(params, fmt);
>  	die_routine(fmt_with_err(buf, sizeof(buf), fmt), params);