Re: [PATCH 2/4] usage.c API users: use die_message() where appropriate

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

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

>  static NORETURN void die_nicely(const char *err, va_list params)
>  {
> +	va_list cp;
>  	static int zombie;
> -	char message[2 * PATH_MAX];
> +	report_fn die_message_fn = get_die_message_routine();
>  
> -	vsnprintf(message, sizeof(message), err, params);
> -	fputs("fatal: ", stderr);
> -	fputs(message, stderr);
> -	fputc('\n', stderr);
> +	va_copy(cp, params);
> +	die_message_fn(err, params);
>  
>  	if (!zombie) {
> +		char message[2 * PATH_MAX];
> +
>  		zombie = 1;
> +		vsnprintf(message, sizeof(message), err, cp);
>  		write_crash_report(message);
>  		end_packfile();
>  		unkeep_all_packs();

So, we used to compose the die-looking "fatal:" message by hand, but
we now grab the function "die()" that is currently in effect uses to
prepare its message, and let it compose the message for us.  It will
work even when somebody else were overriding die_message_routine to
reuse their custome die_message_routine, which is nice.  And because
this function is used to override die_routine, we do not have to be
worrying about somebody else overriding die_routine without
overriding die_message_routine.

OK.


> diff --git a/builtin/notes.c b/builtin/notes.c
> index 71c59583a17..2812d1eac40 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
>  static void write_note_data(struct note_data *d, struct object_id *oid)
>  {
>  	if (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {
> -		error(_("unable to write note object"));
> +		int status = die_message(_("unable to write note object"));
> +
>  		if (d->edit_path)
> -			error(_("the note contents have been left in %s"),
> -				d->edit_path);
> -		exit(128);
> +			die_message(_("the note contents have been left in %s"),
> +				    d->edit_path);
> +		exit(status);

OK.  This changes the message when we terminate from "error:" to
"fatal:", but I can buy that we could argue that the original was
abusing the error() to work around the lack of die_message().

> diff --git a/http-backend.c b/http-backend.c
> index 3d6e2ff17f8..982cb62c7cb 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -659,8 +659,9 @@ static NORETURN void die_webcgi(const char *err, va_list params)
>  {
>  	if (dead <= 1) {
>  		struct strbuf hdr = STRBUF_INIT;
> +		report_fn die_message_fn = get_die_message_routine();
>  
> -		vreportf("fatal: ", err, params);
> +		die_message_fn(err, params);
>  
>  		http_status(&hdr, 500, "Internal Server Error");
>  		hdr_nocache(&hdr);

OK.  As there is a known change that wants to touch this file, it is
unfortunate that you chose to do this now, but the above makes sense.
It is a quite straight-forward clean-up.

> diff --git a/parse-options.c b/parse-options.c
> index fc5b43ff0b2..8bc0a21f1d7 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -1075,6 +1075,6 @@ void NORETURN usage_msg_opt(const char *msg,
>  		   const char * const *usagestr,
>  		   const struct option *options)
>  {
> -	fprintf(stderr, "fatal: %s\n\n", msg);
> +	die_message("%s\n", msg); /* The extra \n is intentional */
>  	usage_with_options(usagestr, options);
>  }

OK.

> diff --git a/run-command.c b/run-command.c
> ...
> @@ -372,9 +363,10 @@ static void NORETURN child_die_fn(const char *err, va_list params)
>  static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
>  {
>  	static void (*old_errfn)(const char *err, va_list params);
> +	report_fn die_message_routine = get_die_message_routine();
>  
>  	old_errfn = get_error_routine();
> -	set_error_routine(fake_fatal);
> +	set_error_routine(die_message_routine);
>  	errno = cerr->syserr;

OK.  This is literally equivalent to the original.

> @@ -1082,7 +1074,9 @@ static void *run_thread(void *data)
>  
>  static NORETURN void die_async(const char *err, va_list params)
>  {
> -	vreportf("fatal: ", err, params);
> +	report_fn die_message_fn = get_die_message_routine();
> +
> +	die_message_fn(err, params);

Likewise.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index a83fbdf6398..d5e495529c8 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -514,6 +514,7 @@ static inline int const_error(void)
>  typedef void (*report_fn)(const char *, va_list params);
>  
>  void set_die_routine(NORETURN_PTR 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 74b43c5db6f..76399ba8409 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -68,7 +68,9 @@ static void die_message_builtin(const char *err, va_list params)
>   */
>  static NORETURN void die_builtin(const char *err, va_list params)
>  {
> -	die_message_builtin(err, params);
> +	report_fn die_message_fn = get_die_message_routine();
> +
> +	die_message_fn(err, params);
>  	exit(128);
>  }
>  
> @@ -112,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;
> @@ -121,6 +124,11 @@ void set_die_routine(NORETURN_PTR report_fn routine)
>  	die_routine = routine;
>  }
>  
> +report_fn get_die_message_routine(void)
> +{
> +	return die_message_routine;
> +}
> +
>  void set_error_routine(report_fn routine)
>  {
>  	error_routine = routine;
> @@ -220,7 +228,7 @@ int die_message(const char *err, ...)
>  	va_list params;
>  
>  	va_start(params, err);
> -	die_message_builtin(err, params);
> +	die_message_routine(err, params);
>  	va_end(params);
>  	return 128;
>  }

I think these hunks are better fit in the previous step.  This is a
"now we have a new set of API functions, let's convert the existing
code to take advantage of them" step, and "oops, what we presented
as a new set of API functions in the previous step was incomplete,
let's patch them up" should not be included in there.

Thanks.