[PATCH 0/4] usage API: Add and use die_message()

[PATCH 0/4] usage API: Add and use die_message()

[Ævar Arnfjörð Bjarmason]

A small series to add both die_message() and die_message_errno()
functions. By using these we can make vreportf() in usage.c static.

Doing so opens the door to later feature work on vreportf() that I
submitted an RFC series for[1], but wanted to peel off this more
trivial (and more easy to review) part.

The range-diff below shows changes since the RFC version. There was a
stupid bug of the die_message_errno() helper returning -1 instead of
128 as its die_message() sibling, I also f ixed up the commit messages
a bit, and had builtin/gc.c call "return" instead of "exit()" while I
was at it.

1. https://lore.kernel.org/git/RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (4):
  usage.c: add a die_message() routine
  usage.c API users: use die_message() where appropriate
  usage.c + gc: add and use a die_message_errno()
  config API: don't use vreportf(), make it static in usage.c

 builtin/fast-import.c | 19 +++++++++++--------
 builtin/gc.c          | 21 ++++++++++++---------
 builtin/notes.c       | 15 +++++++++------
 config.c              | 22 +++++++++-------------
 config.h              | 10 ++++++----
 git-compat-util.h     |  4 +++-
 http-backend.c        |  3 ++-
 imap-send.c           |  3 ++-
 parse-options.c       |  2 +-
 run-command.c         | 16 +++++-----------
 usage.c               | 42 ++++++++++++++++++++++++++++++++++++++----
 11 files changed, 98 insertions(+), 59 deletions(-)

Range-diff:
1:  ae05d2398fb ! 1:  5a9ab85fa56 usage.c: add a die_message() routine
    @@ Commit 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,
    -    this is so that caller can pas the return value to exit(128), instead
    -    of having to hardcode "128".
    +    print a "fatal: " prefix, and it will return with 128 instead of -1,
    +    this is so that caller can pass the return value to "exit()", instead
    +    of having to hardcode "exit(128)".
     
         A subsequent commit will migrate various callers that benefit from
    -    this function over to it, for now we're just adding the routine and
    +    this function over to it. For now we're just adding the routine and
         making die_builtin() in usage.c itself use it.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
2:  b4ff6ddf986 = 2:  36c050c90c2 usage.c API users: use die_message() where appropriate
3:  f93d95ab288 ! 3:  8747afecdcd usage.c + gc: add and use a die_message_errno()
    @@ Commit message
         errors to use a "fatal: " prefix instead. This adds a sibling function
         to the die_errno() added in a preceding commit.
     
    -    Since it returns 128 instead of -1 we'll need to adjust
    -    report_last_gc_error(). Let's adjust it while we're at it to not
    -    conflate the "should skip" and "exit with this non-zero code"
    -    conditions, as the caller is no longer hardcoding "128", but relying
    -    on die_errno() to return a nen-zero exit() status.
    +    Since it returns 128 instead of -1 (like die_message()) we'll need to
    +    adjust report_last_gc_error().
    +
    +    Let's adjust it while we're at it to not conflate the "should skip"
    +    and "exit with this non-zero code" conditions, as the caller is no
    +    longer hardcoding "128", but relying on die_errno() to return a
    +    nen-zero exit() status.
    +
    +    Since we're touching this code let's also use "return ret" in place of
    +    an "exit(ret)". This is kinder to any cleanup our our "cmd_main()" in
    +    "git.c" might want to do.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
      				/* Last gc --auto failed. Skip this one. */
      				return 0;
     +			if (ret)
    -+				/* an error occurred, already reported */
    -+				exit(ret);
    ++				/* an I/O error occurred, already reported */
    ++				return ret;
      
      			if (lock_repo_for_gc(force, &pid))
      				return 0;
    @@ usage.c: int die_message(const char *err, ...)
     +	va_start(params, fmt);
     +	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
     +	va_end(params);
    -+	return -1;
    ++	return 128;
     +}
     +
      #undef error_errno
4:  40fe7cf81a8 = 4:  e0e6427cbd3 config API: don't use vreportf(), make it static in usage.c
-- 
2.34.1.898.g5a552c2e5f0

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

[Ævar Arnfjörð Bjarmason]

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 return with 128 instead of -1,
this is so that caller can pass the return value to "exit()", instead
of having to hardcode "exit(128)".

A subsequent commit will migrate various callers that benefit from
this function over to it. For now we're just adding the routine and
making die_builtin() in usage.c itself use it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-compat-util.h |  1 +
 usage.c           | 22 ++++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index c6bd2a84e55..a83fbdf6398 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -479,6 +479,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)));
diff --git a/usage.c b/usage.c
index c7d233b0de9..74b43c5db6f 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,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);
 }
 
@@ -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;
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
-- 
2.34.1.898.g5a552c2e5f0

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

[Junio C Hamano]

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

OK, it is the other way around.  This step with _builtin is
incomplete introduction of the API, even though it was "we add the
new API function without having any meaningful callers, because
combining them together into a single patch is too much to chew in a
single sitting".  A small part of 2/4 that adds the override-able
die_message_routine should be part of this step to make this step
understandable.

THanks.

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

[Ævar Arnfjörð Bjarmason]

Change code that either called error() and proceeded to exit with 128,
or emitted its own "fatal: " messages to use the die_message()
function added in a preceding commit.

In order to do that we need to add a get_die_message_routine()
function, which works like the other get_*_routine() functions in
usage.c. There is no set_die_message_rotine(), as it hasn't been
needed yet. We can add it if we ever need it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c | 12 +++++++-----
 builtin/notes.c       |  9 +++++----
 git-compat-util.h     |  1 +
 http-backend.c        |  3 ++-
 parse-options.c       |  2 +-
 run-command.c         | 16 +++++-----------
 usage.c               | 12 ++++++++++--
 7 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 20406f67754..2b2e28bad79 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -401,16 +401,18 @@ static void dump_marks(void);
 
 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();
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);
 	}
 }
 
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/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);
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);
 }
diff --git a/run-command.c b/run-command.c
index f40df01c772..a790fe9799d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])
 	child_close(fd[1]);
 }
 
-/*
- * parent will make it look like the child spewed a fatal error and died
- * this is needed to prevent changes to t0061.
- */
-static void fake_fatal(const char *err, va_list params)
-{
-	vreportf("fatal: ", err, params);
-}
-
 static void child_error_fn(const char *err, va_list params)
 {
 	const char msg[] = "error() should not be called in child\n";
@@ -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;
 
 	switch (cerr->err) {
@@ -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);
 
 	if (in_async()) {
 		struct async *async = pthread_getspecific(async_key);
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;
 }
-- 
2.34.1.898.g5a552c2e5f0

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

[Junio C Hamano]

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

[PATCH 3/4] usage.c + gc: add and use a die_message_errno()

[Ævar Arnfjörð Bjarmason]

Change code the "error: " output when we exit with 128 due to gc.log
errors to use a "fatal: " prefix instead. This adds a sibling function
to the die_errno() added in a preceding commit.

Since it returns 128 instead of -1 (like die_message()) we'll need to
adjust report_last_gc_error().

Let's adjust it while we're at it to not conflate the "should skip"
and "exit with this non-zero code" conditions, as the caller is no
longer hardcoding "128", but relying on die_errno() to return a
nen-zero exit() status.

Since we're touching this code let's also use "return ret" in place of
an "exit(ret)". This is kinder to any cleanup our our "cmd_main()" in
"git.c" might want to do.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c      | 21 ++++++++++++---------
 git-compat-util.h |  1 +
 usage.c           | 12 ++++++++++++
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bcef6a4c8da..7b451dfeefc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
  * gc should not proceed due to an error in the last run. Prints a
  * message and returns -1 if an error occurred while reading gc.log
  */
-static int report_last_gc_error(void)
+static int report_last_gc_error(int *skip)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int ret = 0;
 	ssize_t len;
 	struct stat st;
 	char *gc_log_path = git_pathdup("gc.log");
+	*skip = 0;
 
 	if (stat(gc_log_path, &st)) {
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot stat '%s'"), gc_log_path);
 		goto done;
 	}
 
@@ -493,7 +494,7 @@ static int report_last_gc_error(void)
 
 	len = strbuf_read_file(&sb, gc_log_path, 0);
 	if (len < 0)
-		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot read '%s'"), gc_log_path);
 	else if (len > 0) {
 		/*
 		 * A previous gc failed.  Report the error, and don't
@@ -507,7 +508,7 @@ static int report_last_gc_error(void)
 			       "until the file is removed.\n\n"
 			       "%s"),
 			    gc_log_path, sb.buf);
-		ret = 1;
+		*skip = 1;
 	}
 	strbuf_release(&sb);
 done:
@@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
 		}
 		if (detach_auto) {
-			int ret = report_last_gc_error();
-			if (ret < 0)
-				/* an I/O error occurred, already reported */
-				exit(128);
-			if (ret == 1)
+			int skip;
+			int ret = report_last_gc_error(&skip);
+
+			if (skip)
 				/* Last gc --auto failed. Skip this one. */
 				return 0;
+			if (ret)
+				/* an I/O error occurred, already reported */
+				return ret;
 
 			if (lock_repo_for_gc(force, &pid))
 				return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index d5e495529c8..c6c6f7d6b51 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -480,6 +480,7 @@ 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 die_message_errno(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)));
diff --git a/usage.c b/usage.c
index 76399ba8409..3d09e8eea48 100644
--- a/usage.c
+++ b/usage.c
@@ -233,6 +233,18 @@ int die_message(const char *err, ...)
 	return 128;
 }
 
+#undef die_message_errno
+int die_message_errno(const char *fmt, ...)
+{
+	char buf[1024];
+	va_list params;
+
+	va_start(params, fmt);
+	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
+	va_end(params);
+	return 128;
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
-- 
2.34.1.898.g5a552c2e5f0

Re: [PATCH 3/4] usage.c + gc: add and use a die_message_errno()

[Junio C Hamano]

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

> Change code the "error: " output when we exit with 128 due to gc.log
> errors to use a "fatal: " prefix instead. This adds a sibling function
> to the die_errno() added in a preceding commit.
>
> Since it returns 128 instead of -1 (like die_message()) we'll need to
> adjust report_last_gc_error().
>
> Let's adjust it while we're at it to not conflate the "should skip"
> and "exit with this non-zero code" conditions, as the caller is no
> longer hardcoding "128", but relying on die_errno() to return a
> nen-zero exit() status.
>
> Since we're touching this code let's also use "return ret" in place of
> an "exit(ret)". This is kinder to any cleanup our our "cmd_main()" in
> "git.c" might want to do.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/gc.c      | 21 ++++++++++++---------
>  git-compat-util.h |  1 +
>  usage.c           | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 9 deletions(-)

The structure of the series makes sense here.

1 and 2 were structured to add new set of API functions in 1 and to
make use of them in 2 (which is also OK if done right; as pointed
out, a few changes need to be moved from 2 to 1 to make 1 complete).

This makes further addition and adds its uses at the same time.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index bcef6a4c8da..7b451dfeefc 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>   * gc should not proceed due to an error in the last run. Prints a
>   * message and returns -1 if an error occurred while reading gc.log
>   */
> -static int report_last_gc_error(void)
> +static int report_last_gc_error(int *skip)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	int ret = 0;
>  	ssize_t len;
>  	struct stat st;
>  	char *gc_log_path = git_pathdup("gc.log");
> +	*skip = 0;
>  
>  	if (stat(gc_log_path, &st)) {

Insert a new statement after the blank that separates the
declaration from the statements.

>  		if (errno == ENOENT)
>  			goto done;
>  
> -		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
> +		ret = die_message_errno(_("cannot stat '%s'"), gc_log_path);
>  		goto done;
>  	}
>  
> @@ -493,7 +494,7 @@ static int report_last_gc_error(void)
>  
>  	len = strbuf_read_file(&sb, gc_log_path, 0);
>  	if (len < 0)
> -		ret = error_errno(_("cannot read '%s'"), gc_log_path);
> +		ret = die_message_errno(_("cannot read '%s'"), gc_log_path);
>  	else if (len > 0) {
>  		/*
>  		 * A previous gc failed.  Report the error, and don't
> @@ -507,7 +508,7 @@ static int report_last_gc_error(void)
>  			       "until the file is removed.\n\n"
>  			       "%s"),
>  			    gc_log_path, sb.buf);
> -		ret = 1;
> +		*skip = 1;
>  	}
>  	strbuf_release(&sb);
>  done:

Nobody called die() in here, so we always returned to the caller.
It feels a bit strange that we need, in addition to the existing
'ret' that becomes the return value, an extra out parameter *skip.

Let's see why the caller needs both.


> @@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
>  		}
>  		if (detach_auto) {
> -			int ret = report_last_gc_error();
> -			if (ret < 0)
> -				/* an I/O error occurred, already reported */
> -				exit(128);
> -			if (ret == 1)
> +			int skip;
> +			int ret = report_last_gc_error(&skip);
> +
> +			if (skip)
>  				/* Last gc --auto failed. Skip this one. */
>  				return 0;
> +			if (ret)
> +				/* an I/O error occurred, already reported */
> +				return ret;

We used to use the fact that 

 - negative return was from error_errno() to cuase exit(128) to
   emulate die.

 - return with 1 is from the "previous gc failed" block and declare
   we can safely return 0 (success).

 - return with 0 is sucess and nothing needed to be, and was, done.

So, I still do not see the point of this change to add an extra
error channel.  I do not find this argument

> ... as the caller is no
> longer hardcoding "128", but relying on die_errno() to return a
> nen-zero exit() status.

particularly a convincing one.  We are not obliged to assign the
value returned from die_message_errno() to 'ret' to begin with.
report_last_gc_error() can document what its return values mean
and the caller can react to it.

Indeed, if we assign -1 to 'ret' where we used to call error_errno(),
the caller needs no changes at all, no?

Something like this as a replacement for this part?

 builtin/gc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git c/builtin/gc.c w/builtin/gc.c
index f7270b26ea..2b54465608 100644
--- c/builtin/gc.c
+++ w/builtin/gc.c
@@ -477,47 +477,49 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 /*
  * Returns 0 if there was no previous error and gc can proceed, 1 if
  * gc should not proceed due to an error in the last run. Prints a
  * message and returns -1 if an error occurred while reading gc.log
  */
 static int report_last_gc_error(void)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int ret = 0;
+	int ret = -1; /* a pessimist */
 	ssize_t len;
 	struct stat st;
 	char *gc_log_path = git_pathdup("gc.log");
 
 	if (stat(gc_log_path, &st)) {
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
+		die_message_errno(_("cannot stat '%s'"), gc_log_path);
 		goto done;
 	}
 
-	if (st.st_mtime < gc_log_expire_time)
+	if (st.st_mtime < gc_log_expire_time) {
+		ret = 0;
 		goto done;
+	}
 
 	len = strbuf_read_file(&sb, gc_log_path, 0);
 	if (len < 0)
-		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+		die_message_errno(_("cannot read '%s'"), gc_log_path);
 	else if (len > 0) {
 		/*
 		 * A previous gc failed.  Report the error, and don't
 		 * bother with an automatic gc run since it is likely
 		 * to fail in the same way.
 		 */
 		warning(_("The last gc run reported the following. "
 			       "Please correct the root cause\n"
 			       "and remove %s\n"
 			       "Automatic cleanup will not be performed "
 			       "until the file is removed.\n\n"
 			       "%s"),
 			    gc_log_path, sb.buf);
-		ret = 1;
+		ret = 0;
 	}
 	strbuf_release(&sb);
 done:
 	free(gc_log_path);
 	return ret;
 }

[PATCH 4/4] config API: don't use vreportf(), make it static in usage.c

[Ævar Arnfjörð Bjarmason]

In preceding commits the rest of the vreportf() users outside of
usage.c have been migrated to die_message(), leaving only the
git_die_config() function added in 5a80e97c827 (config: add
`git_die_config()` to the config-set API, 2014-08-07).

Let's have its callers call error() themselves if they want to emit a
message, which is exactly what git_die_config() was doing for them
before emitting its own die() message.

This means that we can make the vreportf() in usage.c "static", and
only expose functions such as usage(), die(), warning() etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c |  7 ++++---
 builtin/notes.c       |  6 ++++--
 config.c              | 22 +++++++++-------------
 config.h              | 10 ++++++----
 git-compat-util.h     |  1 -
 imap-send.c           |  3 ++-
 usage.c               |  2 +-
 7 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 2b2e28bad79..4e2432bb491 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3456,9 +3456,10 @@ static void git_pack_config(void)
 	}
 	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
 		pack_idx_opts.version = indexversion_value;
-		if (pack_idx_opts.version > 2)
-			git_die_config("pack.indexversion",
-					"bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
+		if (pack_idx_opts.version > 2) {
+			error("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
+			git_die_config("pack.indexversion");
+		}
 	}
 	if (!git_config_get_ulong("pack.packsizelimit", &packsizelimit_value))
 		max_packsize = packsizelimit_value;
diff --git a/builtin/notes.c b/builtin/notes.c
index 2812d1eac40..60c5dab4122 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -763,8 +763,10 @@ static int git_config_get_notes_strategy(const char *key,
 
 	if (git_config_get_string(key, &value))
 		return 1;
-	if (parse_notes_merge_strategy(value, strategy))
-		git_die_config(key, _("unknown notes merge strategy %s"), value);
+	if (parse_notes_merge_strategy(value, strategy)) {
+		error(_("unknown notes merge strategy %s"), value);
+		git_die_config(key);
+	}
 
 	free(value);
 	return 0;
diff --git a/config.c b/config.c
index c5873f3a706..30f7971e0cc 100644
--- a/config.c
+++ b/config.c
@@ -2323,7 +2323,7 @@ int repo_config_get_string(struct repository *repo,
 	git_config_check_init(repo);
 	ret = git_configset_get_string(repo->config, key, dest);
 	if (ret < 0)
-		git_die_config(key, NULL);
+		git_die_config(key);
 	return ret;
 }
 
@@ -2334,7 +2334,7 @@ int repo_config_get_string_tmp(struct repository *repo,
 	git_config_check_init(repo);
 	ret = git_configset_get_string_tmp(repo->config, key, dest);
 	if (ret < 0)
-		git_die_config(key, NULL);
+		git_die_config(key);
 	return ret;
 }
 
@@ -2380,7 +2380,7 @@ int repo_config_get_pathname(struct repository *repo,
 	git_config_check_init(repo);
 	ret = git_configset_get_pathname(repo->config, key, dest);
 	if (ret < 0)
-		git_die_config(key, NULL);
+		git_die_config(key);
 	return ret;
 }
 
@@ -2452,8 +2452,10 @@ int git_config_get_expiry(const char *key, const char **output)
 		return ret;
 	if (strcmp(*output, "now")) {
 		timestamp_t now = approxidate("now");
-		if (approxidate(*output) >= now)
-			git_die_config(key, _("Invalid %s: '%s'"), key, *output);
+		if (approxidate(*output) >= now) {
+			error(_("Invalid %s: '%s'"), key, *output);
+			git_die_config(key);
+		}
 	}
 	return ret;
 }
@@ -2550,18 +2552,12 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)
 		    key, filename, linenr);
 }
 
-NORETURN __attribute__((format(printf, 2, 3)))
-void git_die_config(const char *key, const char *err, ...)
+NORETURN
+void git_die_config(const char *key)
 {
 	const struct string_list *values;
 	struct key_value_info *kv_info;
 
-	if (err) {
-		va_list params;
-		va_start(params, err);
-		vreportf("error: ", err, params);
-		va_end(params);
-	}
 	values = git_config_get_value_multi(key);
 	kv_info = values->items[values->nr - 1].util;
 	git_die_config_linenr(key, kv_info->filename, kv_info->linenr);
diff --git a/config.h b/config.h
index f119de01309..fae585d2005 100644
--- a/config.h
+++ b/config.h
@@ -626,11 +626,13 @@ struct key_value_info {
 };
 
 /**
- * First prints the error message specified by the caller in `err` and then
- * dies printing the line number and the file name of the highest priority
- * value for the configuration variable `key`.
+ * Dies printing the line number and the file name of the highest
+ * priority value for the configuration variable `key`.
+ *
+ * Consider calling error() first with a more specific formatted
+ * message of your own.
  */
-NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));
+NORETURN void git_die_config(const char *key);
 
 /**
  * Helper function which formats the die error message according to the
diff --git a/git-compat-util.h b/git-compat-util.h
index c6c6f7d6b51..bdb3977b9ec 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -474,7 +474,6 @@ static inline int git_has_dir_sep(const char *path)
 struct strbuf;
 
 /* General helper functions */
-void vreportf(const char *prefix, const char *err, va_list params);
 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)));
diff --git a/imap-send.c b/imap-send.c
index e6090a0346a..0fdfe5159eb 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1350,7 +1350,8 @@ static int git_imap_config(const char *var, const char *val, void *cb)
 		server.port = git_config_int(var, val);
 	else if (!strcmp("imap.host", var)) {
 		if (!val) {
-			git_die_config("imap.host", "Missing value for 'imap.host'");
+			error("Missing value for 'imap.host'");
+			git_die_config("imap.host");
 		} else {
 			if (starts_with(val, "imap:"))
 				val += 5;
diff --git a/usage.c b/usage.c
index 3d09e8eea48..9943dd8742e 100644
--- a/usage.c
+++ b/usage.c
@@ -6,7 +6,7 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
-void vreportf(const char *prefix, const char *err, va_list params)
+static void vreportf(const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
-- 
2.34.1.898.g5a552c2e5f0

Re: [PATCH 4/4] config API: don't use vreportf(), make it static in usage.c

[Junio C Hamano]

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

> In preceding commits the rest of the vreportf() users outside of
> usage.c have been migrated to die_message(), leaving only the
> git_die_config() function added in 5a80e97c827 (config: add
> `git_die_config()` to the config-set API, 2014-08-07).
>
> Let's have its callers call error() themselves if they want to emit a
> message, which is exactly what git_die_config() was doing for them
> before emitting its own die() message.

I do not quite get this.  If git_die_config() has been showing the
message for them, and if the existing callers can just use error(),
why not git_die_config() call error() on behalf of these callers?

> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 2b2e28bad79..4e2432bb491 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -3456,9 +3456,10 @@ static void git_pack_config(void)
>  	}
>  	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
>  		pack_idx_opts.version = indexversion_value;
> -		if (pack_idx_opts.version > 2)
> -			git_die_config("pack.indexversion",
> -					"bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
> +		if (pack_idx_opts.version > 2) {
> +			error("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
> +			git_die_config("pack.indexversion");
> +		}

This is exactly what triggered the question above, and the pattern
repeats elsewhere, too.

> @@ -2550,18 +2552,12 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)
>  		    key, filename, linenr);
>  }
>  
> -NORETURN __attribute__((format(printf, 2, 3)))
> -void git_die_config(const char *key, const char *err, ...)
> +NORETURN
> +void git_die_config(const char *key)
>  {
>  	const struct string_list *values;
>  	struct key_value_info *kv_info;
>  
> -	if (err) {
> -		va_list params;
> -		va_start(params, err);
> -		vreportf("error: ", err, params);
> -		va_end(params);

I get that we do not want to expose vreportf() to this caller, and I
agree with the goal, but wouldn't it be the matter of calling
get_error_routine() and calling it with err and params here, instead
of losing the whole block?  Is that insufficient to avoid toucing
all the callers?

> diff --git a/git-compat-util.h b/git-compat-util.h
> index c6c6f7d6b51..bdb3977b9ec 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -474,7 +474,6 @@ static inline int git_has_dir_sep(const char *path)
>  struct strbuf;
>  
>  /* General helper functions */
> -void vreportf(const char *prefix, const char *err, va_list params);

Good.

> diff --git a/usage.c b/usage.c
> index 3d09e8eea48..9943dd8742e 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -6,7 +6,7 @@
>  #include "git-compat-util.h"
>  #include "cache.h"
>  
> -void vreportf(const char *prefix, const char *err, va_list params)
> +static void vreportf(const char *prefix, const char *err, va_list params)

Good, too.

Thanks.

Re: [PATCH 4/4] config API: don't use vreportf(), make it static in usage.c

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 06 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> In preceding commits the rest of the vreportf() users outside of
>> usage.c have been migrated to die_message(), leaving only the
>> git_die_config() function added in 5a80e97c827 (config: add
>> `git_die_config()` to the config-set API, 2014-08-07).
>>
>> Let's have its callers call error() themselves if they want to emit a
>> message, which is exactly what git_die_config() was doing for them
>> before emitting its own die() message.
>
> I do not quite get this.  If git_die_config() has been showing the
> message for them, and if the existing callers can just use error(),
> why not git_die_config() call error() on behalf of these callers?
>
>> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
>> index 2b2e28bad79..4e2432bb491 100644
>> --- a/builtin/fast-import.c
>> +++ b/builtin/fast-import.c
>> @@ -3456,9 +3456,10 @@ static void git_pack_config(void)
>>  	}
>>  	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
>>  		pack_idx_opts.version = indexversion_value;
>> -		if (pack_idx_opts.version > 2)
>> -			git_die_config("pack.indexversion",
>> -					"bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
>> +		if (pack_idx_opts.version > 2) {
>> +			error("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
>> +			git_die_config("pack.indexversion");
>> +		}
>
> This is exactly what triggered the question above, and the pattern
> repeats elsewhere, too.
>
>> @@ -2550,18 +2552,12 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)
>>  		    key, filename, linenr);
>>  }
>>  
>> -NORETURN __attribute__((format(printf, 2, 3)))
>> -void git_die_config(const char *key, const char *err, ...)
>> +NORETURN
>> +void git_die_config(const char *key)
>>  {
>>  	const struct string_list *values;
>>  	struct key_value_info *kv_info;
>>  
>> -	if (err) {
>> -		va_list params;
>> -		va_start(params, err);
>> -		vreportf("error: ", err, params);
>> -		va_end(params);
>
> I get that we do not want to expose vreportf() to this caller, and I
> agree with the goal, but wouldn't it be the matter of calling
> get_error_routine() and calling it with err and params here, instead
> of losing the whole block?  Is that insufficient to avoid toucing
> all the callers?

I'll fix that in the incoming re-roll. This really didn't belong in this
series but a later one.

FWIW that change was because like this we'll accurately report the
source of the "error" when the function learns to spew <file>:<line> at
the user.

But for now I'll just have it do as you suggest...

[PATCH v2 0/6] usage API: Add and use die_message()

[Ævar Arnfjörð Bjarmason]

A small set of fixes to usage.[ch]'s API use that will go towards
enabling nicer things down the road. See [1] for the v1 summary

I believe this should address all the feedback Junio had on the
v1.

Aside from the substantially rewritten 6/6 and much simplified 4/6 the
end-state is almost the same, but things are better split up,
explained etc. now.

1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (6):
  usage.c: add a die_message() routine
  usage.c API users: use die_message() for "fatal :" + exit 128
  usage.c API users: use die_message() for error() + exit 128
  gc: return from cmd_gc(), don't call exit()
  usage.c + gc: add and use a die_message_errno()
  config API: use get_error_routine(), not vreportf()

 builtin/fast-import.c | 12 +++++++-----
 builtin/gc.c          | 14 ++++++++------
 builtin/notes.c       |  9 +++++----
 config.c              |  3 ++-
 git-compat-util.h     |  4 +++-
 http-backend.c        |  3 ++-
 parse-options.c       |  2 +-
 run-command.c         | 16 +++++-----------
 usage.c               | 42 ++++++++++++++++++++++++++++++++++++++----
 9 files changed, 71 insertions(+), 34 deletions(-)

Range-diff against v1:
1:  5a9ab85fa56 ! 1:  65ae6fe7cbe usage.c: add a die_message() routine
    @@ Commit message
         this is so that caller can pass the return value to "exit()", instead
         of having to hardcode "exit(128)".
     
    -    A subsequent commit will migrate various callers that benefit from
    -    this function over to it. For now we're just adding the routine and
    -    making die_builtin() in usage.c itself use it.
    +    Note that as with the other routines the "die_message_builtin" needs
    +    to return "void" and otherwise conform to the "report_fn"
    +    signature.
    +
    +    As we'll see in a subsequent commit callers will want to replace
    +    e.g. their default "die_routine" with a "die_message_routine".
    +
    +    For now we're just adding the routine and making die_builtin() in
    +    usage.c itself use it. In order to do that we need to add a
    +    get_die_message_routine() function, which works like the other
    +    get_*_routine() functions in usage.c. There is no
    +    set_die_message_rotine(), as it hasn't been needed yet. We can add it
    +    if we ever need it.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ git-compat-util.h: NORETURN void usage(const char *err);
      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)));
    +@@ git-compat-util.h: 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);
     
      ## usage.c ##
     @@ usage.c: static NORETURN void usage_builtin(const char *err, va_list params)
    @@ usage.c: static NORETURN void usage_builtin(const char *err, va_list params)
     -	trace2_cmd_error_va(err, params);
     -
     -	vreportf("fatal: ", err, params);
    --
    -+	die_message_builtin(err, params);
    ++	report_fn die_message_fn = get_die_message_routine();
    + 
    ++	die_message_fn(err, params);
      	exit(128);
      }
      
    +@@ usage.c: 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;
    +@@ usage.c: 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;
     @@ usage.c: void NORETURN die_errno(const char *fmt, ...)
      	va_end(params);
      }
    @@ usage.c: void NORETURN die_errno(const char *fmt, ...)
     +	va_list params;
     +
     +	va_start(params, err);
    -+	die_message_builtin(err, params);
    ++	die_message_routine(err, params);
     +	va_end(params);
     +	return 128;
     +}
2:  36c050c90c2 ! 2:  f5a98901498 usage.c API users: use die_message() where appropriate
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    usage.c API users: use die_message() where appropriate
    +    usage.c API users: use die_message() for "fatal :" + exit 128
     
    -    Change code that either called error() and proceeded to exit with 128,
    -    or emitted its own "fatal: " messages to use the die_message()
    -    function added in a preceding commit.
    +    Change code that printed its own "fatal: " message and exited with a
    +    status code of 128 to use the die_message() function added in a
    +    preceding commit.
     
    -    In order to do that we need to add a get_die_message_routine()
    -    function, which works like the other get_*_routine() functions in
    -    usage.c. There is no set_die_message_rotine(), as it hasn't been
    -    needed yet. We can add it if we ever need it.
    +    This change also demonstrates why the return value of
    +    die_message_routine() needed to be that of "report_fn". We have
    +    callers such as the run-command.c::child_err_spew() which would like
    +    to replace its error routine with the return value of
    +    "get_die_message_routine()".
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ builtin/fast-import.c: static void dump_marks(void);
      		end_packfile();
      		unkeep_all_packs();
     
    - ## builtin/notes.c ##
    -@@ builtin/notes.c: 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);
    - 	}
    - }
    - 
    -
    - ## git-compat-util.h ##
    -@@ git-compat-util.h: 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);
    -
      ## http-backend.c ##
     @@ http-backend.c: static NORETURN void die_webcgi(const char *err, va_list params)
      {
    @@ run-command.c: static void *run_thread(void *data)
      
      	if (in_async()) {
      		struct async *async = pthread_getspecific(async_key);
    -
    - ## usage.c ##
    -@@ usage.c: 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);
    - }
    - 
    -@@ usage.c: 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;
    -@@ usage.c: 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;
    -@@ usage.c: 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;
    - }
-:  ----------- > 3:  c7d67fd41fa usage.c API users: use die_message() for error() + exit 128
-:  ----------- > 4:  f224a281a10 gc: return from cmd_gc(), don't call exit()
3:  8747afecdcd ! 5:  2b4a3910654 usage.c + gc: add and use a die_message_errno()
    @@ Metadata
      ## Commit message ##
         usage.c + gc: add and use a die_message_errno()
     
    -    Change code the "error: " output when we exit with 128 due to gc.log
    -    errors to use a "fatal: " prefix instead. This adds a sibling function
    -    to the die_errno() added in a preceding commit.
    +    Change the "error: " output when we exit with 128 due to gc.log errors
    +    to use a "fatal: " prefix instead. To do this add a
    +    die_message_errno() a sibling function to the die_errno() added in a
    +    preceding commit.
     
    -    Since it returns 128 instead of -1 (like die_message()) we'll need to
    -    adjust report_last_gc_error().
    +    Before this we'd expect report_last_gc_error() to return -1 from
    +    error_errno() in this case. It already treated a status of 0 and 1
    +    specially. Let's just document that anything that's not 0 or 1 should
    +    be returned.
     
    -    Let's adjust it while we're at it to not conflate the "should skip"
    -    and "exit with this non-zero code" conditions, as the caller is no
    -    longer hardcoding "128", but relying on die_errno() to return a
    -    nen-zero exit() status.
    -
    -    Since we're touching this code let's also use "return ret" in place of
    -    an "exit(ret)". This is kinder to any cleanup our our "cmd_main()" in
    -    "git.c" might want to do.
    +    We could also retain the "ret < 0" behavior here without hardcoding
    +    128 by returning -128, and having the caller do a "return -ret", but I
    +    think this makes more sense, and preserves the path from
    +    die_message*()'s return value to the "return" without hardcoding
    +    "128".
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/gc.c ##
     @@ builtin/gc.c: static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
    + /*
    +  * Returns 0 if there was no previous error and gc can proceed, 1 if
       * gc should not proceed due to an error in the last run. Prints a
    -  * message and returns -1 if an error occurred while reading gc.log
    +- * message and returns -1 if an error occurred while reading gc.log
    ++ * message and returns with a non-[01] status code if an error occurred
    ++ * while reading gc.log
       */
    --static int report_last_gc_error(void)
    -+static int report_last_gc_error(int *skip)
    + static int report_last_gc_error(void)
      {
    - 	struct strbuf sb = STRBUF_INIT;
    - 	int ret = 0;
    - 	ssize_t len;
    - 	struct stat st;
    - 	char *gc_log_path = git_pathdup("gc.log");
    -+	*skip = 0;
    - 
    - 	if (stat(gc_log_path, &st)) {
    +@@ builtin/gc.c: static int report_last_gc_error(void)
      		if (errno == ENOENT)
      			goto done;
      
    @@ builtin/gc.c: static int report_last_gc_error(void)
      	else if (len > 0) {
      		/*
      		 * A previous gc failed.  Report the error, and don't
    -@@ builtin/gc.c: static int report_last_gc_error(void)
    - 			       "until the file is removed.\n\n"
    - 			       "%s"),
    - 			    gc_log_path, sb.buf);
    --		ret = 1;
    -+		*skip = 1;
    - 	}
    - 	strbuf_release(&sb);
    - done:
     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
    - 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
    - 		}
      		if (detach_auto) {
    --			int ret = report_last_gc_error();
    + 			int ret = report_last_gc_error();
    + 
     -			if (ret < 0)
     -				/* an I/O error occurred, already reported */
    --				exit(128);
    --			if (ret == 1)
    -+			int skip;
    -+			int ret = report_last_gc_error(&skip);
    -+
    -+			if (skip)
    +-				return 128;
    + 			if (ret == 1)
      				/* Last gc --auto failed. Skip this one. */
      				return 0;
    -+			if (ret)
    ++			else if (ret)
     +				/* an I/O error occurred, already reported */
     +				return ret;
      
4:  e0e6427cbd3 ! 6:  42132406731 config API: don't use vreportf(), make it static in usage.c
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    config API: don't use vreportf(), make it static in usage.c
    +    config API: use get_error_routine(), not vreportf()
     
    -    In preceding commits the rest of the vreportf() users outside of
    -    usage.c have been migrated to die_message(), leaving only the
    -    git_die_config() function added in 5a80e97c827 (config: add
    -    `git_die_config()` to the config-set API, 2014-08-07).
    -
    -    Let's have its callers call error() themselves if they want to emit a
    -    message, which is exactly what git_die_config() was doing for them
    -    before emitting its own die() message.
    +    Change the git_die_config() function added in 5a80e97c827 (config: add
    +    `git_die_config()` to the config-set API, 2014-08-07) to use the
    +    public callbacks in the usage.[ch] API instead of the the underlying
    +    vreportf() function.
     
    -    This means that we can make the vreportf() in usage.c "static", and
    -    only expose functions such as usage(), die(), warning() etc.
    +    In preceding commits the rest of the vreportf() users outside of
    +    usage.c was migrated to die_message(), so we can now make it "static".
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## builtin/fast-import.c ##
    -@@ builtin/fast-import.c: static void git_pack_config(void)
    - 	}
    - 	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
    - 		pack_idx_opts.version = indexversion_value;
    --		if (pack_idx_opts.version > 2)
    --			git_die_config("pack.indexversion",
    --					"bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
    -+		if (pack_idx_opts.version > 2) {
    -+			error("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
    -+			git_die_config("pack.indexversion");
    -+		}
    - 	}
    - 	if (!git_config_get_ulong("pack.packsizelimit", &packsizelimit_value))
    - 		max_packsize = packsizelimit_value;
    -
    - ## builtin/notes.c ##
    -@@ builtin/notes.c: static int git_config_get_notes_strategy(const char *key,
    - 
    - 	if (git_config_get_string(key, &value))
    - 		return 1;
    --	if (parse_notes_merge_strategy(value, strategy))
    --		git_die_config(key, _("unknown notes merge strategy %s"), value);
    -+	if (parse_notes_merge_strategy(value, strategy)) {
    -+		error(_("unknown notes merge strategy %s"), value);
    -+		git_die_config(key);
    -+	}
    - 
    - 	free(value);
    - 	return 0;
    -
      ## config.c ##
    -@@ config.c: int repo_config_get_string(struct repository *repo,
    - 	git_config_check_init(repo);
    - 	ret = git_configset_get_string(repo->config, key, dest);
    - 	if (ret < 0)
    --		git_die_config(key, NULL);
    -+		git_die_config(key);
    - 	return ret;
    - }
    - 
    -@@ config.c: int repo_config_get_string_tmp(struct repository *repo,
    - 	git_config_check_init(repo);
    - 	ret = git_configset_get_string_tmp(repo->config, key, dest);
    - 	if (ret < 0)
    --		git_die_config(key, NULL);
    -+		git_die_config(key);
    - 	return ret;
    - }
    - 
    -@@ config.c: int repo_config_get_pathname(struct repository *repo,
    - 	git_config_check_init(repo);
    - 	ret = git_configset_get_pathname(repo->config, key, dest);
    - 	if (ret < 0)
    --		git_die_config(key, NULL);
    -+		git_die_config(key);
    - 	return ret;
    - }
    - 
    -@@ config.c: int git_config_get_expiry(const char *key, const char **output)
    - 		return ret;
    - 	if (strcmp(*output, "now")) {
    - 		timestamp_t now = approxidate("now");
    --		if (approxidate(*output) >= now)
    --			git_die_config(key, _("Invalid %s: '%s'"), key, *output);
    -+		if (approxidate(*output) >= now) {
    -+			error(_("Invalid %s: '%s'"), key, *output);
    -+			git_die_config(key);
    -+		}
    - 	}
    - 	return ret;
    - }
    -@@ config.c: void git_die_config_linenr(const char *key, const char *filename, int linenr)
    - 		    key, filename, linenr);
    - }
    - 
    --NORETURN __attribute__((format(printf, 2, 3)))
    --void git_die_config(const char *key, const char *err, ...)
    -+NORETURN
    -+void git_die_config(const char *key)
    +@@ config.c: void git_die_config(const char *key, const char *err, ...)
      {
      	const struct string_list *values;
      	struct key_value_info *kv_info;
    ++	report_fn error_fn = get_error_routine();
      
    --	if (err) {
    --		va_list params;
    --		va_start(params, err);
    + 	if (err) {
    + 		va_list params;
    + 		va_start(params, err);
     -		vreportf("error: ", err, params);
    --		va_end(params);
    --	}
    ++		error_fn(err, params);
    + 		va_end(params);
    + 	}
      	values = git_config_get_value_multi(key);
    - 	kv_info = values->items[values->nr - 1].util;
    - 	git_die_config_linenr(key, kv_info->filename, kv_info->linenr);
    -
    - ## config.h ##
    -@@ config.h: struct key_value_info {
    - };
    - 
    - /**
    -- * First prints the error message specified by the caller in `err` and then
    -- * dies printing the line number and the file name of the highest priority
    -- * value for the configuration variable `key`.
    -+ * Dies printing the line number and the file name of the highest
    -+ * priority value for the configuration variable `key`.
    -+ *
    -+ * Consider calling error() first with a more specific formatted
    -+ * message of your own.
    -  */
    --NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));
    -+NORETURN void git_die_config(const char *key);
    - 
    - /**
    -  * Helper function which formats the die error message according to the
     
      ## git-compat-util.h ##
     @@ git-compat-util.h: static inline int git_has_dir_sep(const char *path)
    @@ git-compat-util.h: static inline int git_has_dir_sep(const char *path)
      NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
      NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
     
    - ## imap-send.c ##
    -@@ imap-send.c: static int git_imap_config(const char *var, const char *val, void *cb)
    - 		server.port = git_config_int(var, val);
    - 	else if (!strcmp("imap.host", var)) {
    - 		if (!val) {
    --			git_die_config("imap.host", "Missing value for 'imap.host'");
    -+			error("Missing value for 'imap.host'");
    -+			git_die_config("imap.host");
    - 		} else {
    - 			if (starts_with(val, "imap:"))
    - 				val += 5;
    -
      ## usage.c ##
     @@
      #include "git-compat-util.h"
-- 
2.34.1.898.g5a552c2e5f0

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

[Ævar Arnfjörð Bjarmason]

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 return with 128 instead of -1,
this is so that caller can pass the return value to "exit()", instead
of having to hardcode "exit(128)".

Note that as with the other routines the "die_message_builtin" needs
to return "void" and otherwise conform to the "report_fn"
signature.

As we'll see in a subsequent commit callers will want to replace
e.g. their default "die_routine" with a "die_message_routine".

For now we're just adding the routine and making die_builtin() in
usage.c itself use it. In order to do that we need to add a
get_die_message_routine() function, which works like the other
get_*_routine() functions in usage.c. There is no
set_die_message_rotine(), as it hasn't been needed yet. We can add it
if we ever need it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-compat-util.h |  2 ++
 usage.c           | 28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index c6bd2a84e55..d5e495529c8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -479,6 +479,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)));
@@ -513,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 c7d233b0de9..76399ba8409 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,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;
@@ -211,6 +222,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_routine(err, params);
+	va_end(params);
+	return 128;
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
-- 
2.34.1.898.g5a552c2e5f0

[PATCH v2 2/6] usage.c API users: use die_message() for "fatal :" + exit 128

[Ævar Arnfjörð Bjarmason]

Change code that printed its own "fatal: " message and exited with a
status code of 128 to use the die_message() function added in a
preceding commit.

This change also demonstrates why the return value of
die_message_routine() needed to be that of "report_fn". We have
callers such as the run-command.c::child_err_spew() which would like
to replace its error routine with the return value of
"get_die_message_routine()".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c | 12 +++++++-----
 http-backend.c        |  3 ++-
 parse-options.c       |  2 +-
 run-command.c         | 16 +++++-----------
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 20406f67754..2b2e28bad79 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -401,16 +401,18 @@ static void dump_marks(void);
 
 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();
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);
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);
 }
diff --git a/run-command.c b/run-command.c
index f40df01c772..a790fe9799d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])
 	child_close(fd[1]);
 }
 
-/*
- * parent will make it look like the child spewed a fatal error and died
- * this is needed to prevent changes to t0061.
- */
-static void fake_fatal(const char *err, va_list params)
-{
-	vreportf("fatal: ", err, params);
-}
-
 static void child_error_fn(const char *err, va_list params)
 {
 	const char msg[] = "error() should not be called in child\n";
@@ -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;
 
 	switch (cerr->err) {
@@ -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);
 
 	if (in_async()) {
 		struct async *async = pthread_getspecific(async_key);
-- 
2.34.1.898.g5a552c2e5f0

[PATCH v2 3/6] usage.c API users: use die_message() for error() + exit 128

[Ævar Arnfjörð Bjarmason]

Continue the migration of code that printed a message and exited with
128. In this case the caller used "error()", so we'll be changing the
output from "error: " to "fatal: ". This change is intentional and
desired.

This code is dying, so it should emit "fatal", the only reason it
didn't do so was because before the existence of "die_message()" it
would have needed to craft its own "fatal: " message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/notes.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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);
 	}
 }
 
-- 
2.34.1.898.g5a552c2e5f0

[PATCH v2 4/6] gc: return from cmd_gc(), don't call exit()

[Ævar Arnfjörð Bjarmason]

A minor code cleanup. Let's "return" from cmd_gc() instead of calling
exit(). See 338abb0f045 (builtins + test helpers: use return instead
of exit() in cmd_*, 2021-06-08) for other such cases.

While we're at it add a \n to separate the variable declaration from
the rest of the code in this block. Both of these changes make a
subsequent change smaller and easier to read.

This change isn't really needed for that subsequent change, but now
someone viewing that future behavior change won't need to wonder why
we're either still calling exit() here, or fixing it while we're at
it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bcef6a4c8da..900ccfb8d48 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -611,9 +611,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		}
 		if (detach_auto) {
 			int ret = report_last_gc_error();
+
 			if (ret < 0)
 				/* an I/O error occurred, already reported */
-				exit(128);
+				return 128;
 			if (ret == 1)
 				/* Last gc --auto failed. Skip this one. */
 				return 0;
-- 
2.34.1.898.g5a552c2e5f0

[PATCH v2 5/6] usage.c + gc: add and use a die_message_errno()

[Ævar Arnfjörð Bjarmason]

Change the "error: " output when we exit with 128 due to gc.log errors
to use a "fatal: " prefix instead. To do this add a
die_message_errno() a sibling function to the die_errno() added in a
preceding commit.

Before this we'd expect report_last_gc_error() to return -1 from
error_errno() in this case. It already treated a status of 0 and 1
specially. Let's just document that anything that's not 0 or 1 should
be returned.

We could also retain the "ret < 0" behavior here without hardcoding
128 by returning -128, and having the caller do a "return -ret", but I
think this makes more sense, and preserves the path from
die_message*()'s return value to the "return" without hardcoding
"128".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c      | 13 +++++++------
 git-compat-util.h |  1 +
 usage.c           | 12 ++++++++++++
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 900ccfb8d48..8e60ef1eaba 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -470,7 +470,8 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 /*
  * Returns 0 if there was no previous error and gc can proceed, 1 if
  * gc should not proceed due to an error in the last run. Prints a
- * message and returns -1 if an error occurred while reading gc.log
+ * message and returns with a non-[01] status code if an error occurred
+ * while reading gc.log
  */
 static int report_last_gc_error(void)
 {
@@ -484,7 +485,7 @@ static int report_last_gc_error(void)
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot stat '%s'"), gc_log_path);
 		goto done;
 	}
 
@@ -493,7 +494,7 @@ static int report_last_gc_error(void)
 
 	len = strbuf_read_file(&sb, gc_log_path, 0);
 	if (len < 0)
-		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot read '%s'"), gc_log_path);
 	else if (len > 0) {
 		/*
 		 * A previous gc failed.  Report the error, and don't
@@ -612,12 +613,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		if (detach_auto) {
 			int ret = report_last_gc_error();
 
-			if (ret < 0)
-				/* an I/O error occurred, already reported */
-				return 128;
 			if (ret == 1)
 				/* Last gc --auto failed. Skip this one. */
 				return 0;
+			else if (ret)
+				/* an I/O error occurred, already reported */
+				return ret;
 
 			if (lock_repo_for_gc(force, &pid))
 				return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index d5e495529c8..c6c6f7d6b51 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -480,6 +480,7 @@ 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 die_message_errno(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)));
diff --git a/usage.c b/usage.c
index 76399ba8409..3d09e8eea48 100644
--- a/usage.c
+++ b/usage.c
@@ -233,6 +233,18 @@ int die_message(const char *err, ...)
 	return 128;
 }
 
+#undef die_message_errno
+int die_message_errno(const char *fmt, ...)
+{
+	char buf[1024];
+	va_list params;
+
+	va_start(params, fmt);
+	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
+	va_end(params);
+	return 128;
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
-- 
2.34.1.898.g5a552c2e5f0

[PATCH v2 6/6] config API: use get_error_routine(), not vreportf()

[Ævar Arnfjörð Bjarmason]

Change the git_die_config() function added in 5a80e97c827 (config: add
`git_die_config()` to the config-set API, 2014-08-07) to use the
public callbacks in the usage.[ch] API instead of the the underlying
vreportf() function.

In preceding commits the rest of the vreportf() users outside of
usage.c was migrated to die_message(), so we can now make it "static".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c          | 3 ++-
 git-compat-util.h | 1 -
 usage.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index c5873f3a706..e96b8fdb62d 100644
--- a/config.c
+++ b/config.c
@@ -2555,11 +2555,12 @@ void git_die_config(const char *key, const char *err, ...)
 {
 	const struct string_list *values;
 	struct key_value_info *kv_info;
+	report_fn error_fn = get_error_routine();
 
 	if (err) {
 		va_list params;
 		va_start(params, err);
-		vreportf("error: ", err, params);
+		error_fn(err, params);
 		va_end(params);
 	}
 	values = git_config_get_value_multi(key);
diff --git a/git-compat-util.h b/git-compat-util.h
index c6c6f7d6b51..bdb3977b9ec 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -474,7 +474,6 @@ static inline int git_has_dir_sep(const char *path)
 struct strbuf;
 
 /* General helper functions */
-void vreportf(const char *prefix, const char *err, va_list params);
 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)));
diff --git a/usage.c b/usage.c
index 3d09e8eea48..9943dd8742e 100644
--- a/usage.c
+++ b/usage.c
@@ -6,7 +6,7 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
-void vreportf(const char *prefix, const char *err, va_list params)
+static void vreportf(const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
-- 
2.34.1.898.g5a552c2e5f0

Re: [PATCH v2 0/6] usage API: Add and use die_message()

[Junio C Hamano]

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

> A small set of fixes to usage.[ch]'s API use that will go towards
> enabling nicer things down the road. See [1] for the v1 summary
>
> I believe this should address all the feedback Junio had on the
> v1.

Thanks.  We need more reviewers, I guess, but in the meantime, you
need to make do with me ;-)

Re: [PATCH v2 0/6] usage API: Add and use die_message()

[Jonathan Tan]

> A small set of fixes to usage.[ch]'s API use that will go towards
> enabling nicer things down the road. See [1] for the v1 summary
> 
> I believe this should address all the feedback Junio had on the
> v1.
> 
> Aside from the substantially rewritten 6/6 and much simplified 4/6 the
> end-state is almost the same, but things are better split up,
> explained etc. now.
> 
> 1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/

I haven't looked at this round of patches yet, but for the convenience
of reviewers, it would have been great if you linked to a prior
discussion [1], including an email from me with comments that (as far as
I know) haven't been addressed [2].

[1] https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/
[2] https://lore.kernel.org/git/20211027215053.2257548-1-jonathantanmy@google.com/

Re: [PATCH v2 0/6] usage API: Add and use die_message()

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> A small set of fixes to usage.[ch]'s API use that will go towards
>> enabling nicer things down the road. See [1] for the v1 summary
>> 
>> I believe this should address all the feedback Junio had on the
>> v1.
>> 
>> Aside from the substantially rewritten 6/6 and much simplified 4/6 the
>> end-state is almost the same, but things are better split up,
>> explained etc. now.
>> 
>> 1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/
>
> I haven't looked at this round of patches yet, but for the convenience
> of reviewers, it would have been great if you linked to a prior
> discussion [1], including an email from me with comments that (as far as
> I know) haven't been addressed [2].
>
> [1] https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/
> [2] https://lore.kernel.org/git/20211027215053.2257548-1-jonathantanmy@google.com/

Hear, hear, and thanks.

Re: [PATCH v2 0/6] usage API: Add and use die_message()

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 22 2021, Jonathan Tan wrote:

>> A small set of fixes to usage.[ch]'s API use that will go towards
>> enabling nicer things down the road. See [1] for the v1 summary
>> 
>> I believe this should address all the feedback Junio had on the
>> v1.
>> 
>> Aside from the substantially rewritten 6/6 and much simplified 4/6 the
>> end-state is almost the same, but things are better split up,
>> explained etc. now.
>> 
>> 1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/
>
> I haven't looked at this round of patches yet, but for the convenience
> of reviewers, it would have been great if you linked to a prior
> discussion [1],

FWIW this is (indirectly) linked-to upthread. I linked to this RFC
series which this is peeled from:
https://lore.kernel.org/git/RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com/

Which in turn links to the earlier series you commented on that
die_message() is peeled off from:
https://lore.kernel.org/git/cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com/

> including an email from me with comments that (as far as
> I know) haven't been addressed [2].

Is there anything to address in the context of this current series?
I.e. you're commenting on leak detection, which isn't at all the goal of
this series.

I.e. I had these die_message() changes that I worked into that series,
but there didn't seem to be any interest in code changes that "solved"
issues of SANITIZE=leak interaction with various non-CI compiler
versions/flags, so I didn't think it was worth pursuing.

But as this series shows die_message() is also something we can use in
other contexts.

> [1] https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/
> [2] https://lore.kernel.org/git/20211027215053.2257548-1-jonathantanmy@google.com/