{"thread":{"id":"57040","subject":"[PATCH 0/4] usage API: Add and use die_message()","startedAt":"2021-12-06T17:04:21Z","lastAt":"2021-12-24T17:06:13Z","messageCount":22,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"443172","messageId":"cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com","threadId":"57040","inReplyTo":null,"subject":"[PATCH 0/4] usage API: Add and use die_message()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-06T16:55:49Z","receivedAt":"2021-12-06T17:04:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A small series to add both die_message() and die_message_errno()\nfunctions. By using these we can make vreportf() in usage.c static.\n\nDoing so opens the door to later feature work on vreportf() that I\nsubmitted an RFC series for[1], but wanted to peel off this more\ntrivial (and more easy to review) part.\n\nThe range-diff below shows changes since the RFC version. There was a\nstupid bug of the die_message_errno() helper returning -1 instead of\n128 as its die_message() sibling, I also f ixed up the commit messages\na bit, and had builtin/gc.c call \"return\" instead of \"exit()\" while I\nwas at it.\n\n1. https://lore.kernel.org/git/RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (4):\n  usage.c: add a die_message() routine\n  usage.c API users: use die_message() where appropriate\n  usage.c + gc: add and use a die_message_errno()\n  config API: don't use vreportf(), make it static in usage.c\n\n builtin/fast-import.c | 19 +++++++++++--------\n builtin/gc.c          | 21 ++++++++++++---------\n builtin/notes.c       | 15 +++++++++------\n config.c              | 22 +++++++++-------------\n config.h              | 10 ++++++----\n git-compat-util.h     |  4 +++-\n http-backend.c        |  3 ++-\n imap-send.c           |  3 ++-\n parse-options.c       |  2 +-\n run-command.c         | 16 +++++-----------\n usage.c               | 42 ++++++++++++++++++++++++++++++++++++++----\n 11 files changed, 98 insertions(+), 59 deletions(-)\n\nRange-diff:\n1:  ae05d2398fb ! 1:  5a9ab85fa56 usage.c: add a die_message() routine\n    @@ Commit message\n         helper routine to bridge this gap in the API.\n     \n         Functionally this behaves just like the error() routine, except it'll\n    -    print a \"fatal: \" prefix, and it will exit with 128 instead of -1,\n    -    this is so that caller can pas the return value to exit(128), instead\n    -    of having to hardcode \"128\".\n    +    print a \"fatal: \" prefix, and it will return with 128 instead of -1,\n    +    this is so that caller can pass the return value to \"exit()\", instead\n    +    of having to hardcode \"exit(128)\".\n     \n         A subsequent commit will migrate various callers that benefit from\n    -    this function over to it, for now we're just adding the routine and\n    +    this function over to it. For now we're just adding the routine and\n         making die_builtin() in usage.c itself use it.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n2:  b4ff6ddf986 = 2:  36c050c90c2 usage.c API users: use die_message() where appropriate\n3:  f93d95ab288 ! 3:  8747afecdcd usage.c + gc: add and use a die_message_errno()\n    @@ Commit message\n         errors to use a \"fatal: \" prefix instead. This adds a sibling function\n         to the die_errno() added in a preceding commit.\n     \n    -    Since it returns 128 instead of -1 we'll need to adjust\n    -    report_last_gc_error(). Let's adjust it while we're at it to not\n    -    conflate the \"should skip\" and \"exit with this non-zero code\"\n    -    conditions, as the caller is no longer hardcoding \"128\", but relying\n    -    on die_errno() to return a nen-zero exit() status.\n    +    Since it returns 128 instead of -1 (like die_message()) we'll need to\n    +    adjust report_last_gc_error().\n    +\n    +    Let's adjust it while we're at it to not conflate the \"should skip\"\n    +    and \"exit with this non-zero code\" conditions, as the caller is no\n    +    longer hardcoding \"128\", but relying on die_errno() to return a\n    +    nen-zero exit() status.\n    +\n    +    Since we're touching this code let's also use \"return ret\" in place of\n    +    an \"exit(ret)\". This is kinder to any cleanup our our \"cmd_main()\" in\n    +    \"git.c\" might want to do.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)\n      \t\t\t\t/* Last gc --auto failed. Skip this one. */\n      \t\t\t\treturn 0;\n     +\t\t\tif (ret)\n    -+\t\t\t\t/* an error occurred, already reported */\n    -+\t\t\t\texit(ret);\n    ++\t\t\t\t/* an I/O error occurred, already reported */\n    ++\t\t\t\treturn ret;\n      \n      \t\t\tif (lock_repo_for_gc(force, &pid))\n      \t\t\t\treturn 0;\n    @@ usage.c: int die_message(const char *err, ...)\n     +\tva_start(params, fmt);\n     +\tdie_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);\n     +\tva_end(params);\n    -+\treturn -1;\n    ++\treturn 128;\n     +}\n     +\n      #undef error_errno\n4:  40fe7cf81a8 = 4:  e0e6427cbd3 config API: don't use vreportf(), make it static in usage.c\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443173","messageId":"patch-2.4-36c050c90c2-20211206T165221Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com","subject":"[PATCH 2/4] usage.c API users: use die_message() where appropriate","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-06T16:55:51Z","receivedAt":"2021-12-06T17:04:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code that either called error() and proceeded to exit with 128,\nor emitted its own \"fatal: \" messages to use the die_message()\nfunction added in a preceding commit.\n\nIn order to do that we need to add a get_die_message_routine()\nfunction, which works like the other get_*_routine() functions in\nusage.c. There is no set_die_message_rotine(), as it hasn't been\nneeded yet. We can add it if we ever need it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/fast-import.c | 12 +++++++-----\n builtin/notes.c       |  9 +++++----\n git-compat-util.h     |  1 +\n http-backend.c        |  3 ++-\n parse-options.c       |  2 +-\n run-command.c         | 16 +++++-----------\n usage.c               | 12 ++++++++++--\n 7 files changed, 31 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 20406f67754..2b2e28bad79 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -401,16 +401,18 @@ static void dump_marks(void);\n \n static NORETURN void die_nicely(const char *err, va_list params)\n {\n+\tva_list cp;\n \tstatic int zombie;\n-\tchar message[2 * PATH_MAX];\n+\treport_fn die_message_fn = get_die_message_routine();\n \n-\tvsnprintf(message, sizeof(message), err, params);\n-\tfputs(\"fatal: \", stderr);\n-\tfputs(message, stderr);\n-\tfputc('\\n', stderr);\n+\tva_copy(cp, params);\n+\tdie_message_fn(err, params);\n \n \tif (!zombie) {\n+\t\tchar message[2 * PATH_MAX];\n+\n \t\tzombie = 1;\n+\t\tvsnprintf(message, sizeof(message), err, cp);\n \t\twrite_crash_report(message);\n \t\tend_packfile();\n \t\tunkeep_all_packs();\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 71c59583a17..2812d1eac40 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n static void write_note_data(struct note_data *d, struct object_id *oid)\n {\n \tif (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {\n-\t\terror(_(\"unable to write note object\"));\n+\t\tint status = die_message(_(\"unable to write note object\"));\n+\n \t\tif (d->edit_path)\n-\t\t\terror(_(\"the note contents have been left in %s\"),\n-\t\t\t\td->edit_path);\n-\t\texit(128);\n+\t\t\tdie_message(_(\"the note contents have been left in %s\"),\n+\t\t\t\t    d->edit_path);\n+\t\texit(status);\n \t}\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a83fbdf6398..d5e495529c8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -514,6 +514,7 @@ static inline int const_error(void)\n typedef void (*report_fn)(const char *, va_list params);\n \n void set_die_routine(NORETURN_PTR report_fn routine);\n+report_fn get_die_message_routine(void);\n void set_error_routine(report_fn routine);\n report_fn get_error_routine(void);\n void set_warn_routine(report_fn routine);\ndiff --git a/http-backend.c b/http-backend.c\nindex 3d6e2ff17f8..982cb62c7cb 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -659,8 +659,9 @@ static NORETURN void die_webcgi(const char *err, va_list params)\n {\n \tif (dead <= 1) {\n \t\tstruct strbuf hdr = STRBUF_INIT;\n+\t\treport_fn die_message_fn = get_die_message_routine();\n \n-\t\tvreportf(\"fatal: \", err, params);\n+\t\tdie_message_fn(err, params);\n \n \t\thttp_status(&hdr, 500, \"Internal Server Error\");\n \t\thdr_nocache(&hdr);\ndiff --git a/parse-options.c b/parse-options.c\nindex fc5b43ff0b2..8bc0a21f1d7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1075,6 +1075,6 @@ void NORETURN usage_msg_opt(const char *msg,\n \t\t   const char * const *usagestr,\n \t\t   const struct option *options)\n {\n-\tfprintf(stderr, \"fatal: %s\\n\\n\", msg);\n+\tdie_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n \tusage_with_options(usagestr, options);\n }\ndiff --git a/run-command.c b/run-command.c\nindex f40df01c772..a790fe9799d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])\n \tchild_close(fd[1]);\n }\n \n-/*\n- * parent will make it look like the child spewed a fatal error and died\n- * this is needed to prevent changes to t0061.\n- */\n-static void fake_fatal(const char *err, va_list params)\n-{\n-\tvreportf(\"fatal: \", err, params);\n-}\n-\n static void child_error_fn(const char *err, va_list params)\n {\n \tconst char msg[] = \"error() should not be called in child\\n\";\n@@ -372,9 +363,10 @@ static void NORETURN child_die_fn(const char *err, va_list params)\n static void child_err_spew(struct child_process *cmd, struct child_err *cerr)\n {\n \tstatic void (*old_errfn)(const char *err, va_list params);\n+\treport_fn die_message_routine = get_die_message_routine();\n \n \told_errfn = get_error_routine();\n-\tset_error_routine(fake_fatal);\n+\tset_error_routine(die_message_routine);\n \terrno = cerr->syserr;\n \n \tswitch (cerr->err) {\n@@ -1082,7 +1074,9 @@ static void *run_thread(void *data)\n \n static NORETURN void die_async(const char *err, va_list params)\n {\n-\tvreportf(\"fatal: \", err, params);\n+\treport_fn die_message_fn = get_die_message_routine();\n+\n+\tdie_message_fn(err, params);\n \n \tif (in_async()) {\n \t\tstruct async *async = pthread_getspecific(async_key);\ndiff --git a/usage.c b/usage.c\nindex 74b43c5db6f..76399ba8409 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -68,7 +68,9 @@ static void die_message_builtin(const char *err, va_list params)\n  */\n static NORETURN void die_builtin(const char *err, va_list params)\n {\n-\tdie_message_builtin(err, params);\n+\treport_fn die_message_fn = get_die_message_routine();\n+\n+\tdie_message_fn(err, params);\n \texit(128);\n }\n \n@@ -112,6 +114,7 @@ static int die_is_recursing_builtin(void)\n  * (ugh), so keep things static. */\n static NORETURN_PTR report_fn usage_routine = usage_builtin;\n static NORETURN_PTR report_fn die_routine = die_builtin;\n+static report_fn die_message_routine = die_message_builtin;\n static report_fn error_routine = error_builtin;\n static report_fn warn_routine = warn_builtin;\n static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n@@ -121,6 +124,11 @@ void set_die_routine(NORETURN_PTR report_fn routine)\n \tdie_routine = routine;\n }\n \n+report_fn get_die_message_routine(void)\n+{\n+\treturn die_message_routine;\n+}\n+\n void set_error_routine(report_fn routine)\n {\n \terror_routine = routine;\n@@ -220,7 +228,7 @@ int die_message(const char *err, ...)\n \tva_list params;\n \n \tva_start(params, err);\n-\tdie_message_builtin(err, params);\n+\tdie_message_routine(err, params);\n \tva_end(params);\n \treturn 128;\n }\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443174","messageId":"patch-1.4-5a9ab85fa56-20211206T165221Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com","subject":"[PATCH 1/4] usage.c: add a die_message() routine","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-06T16:55:50Z","receivedAt":"2021-12-06T17:04:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We have code in various places that would like to call die(), but\nwants to defer the exit(128) it would invoke, e.g. to print an\nadditional message, or adjust the exit code. Add a die_message()\nhelper routine to bridge this gap in the API.\n\nFunctionally this behaves just like the error() routine, except it'll\nprint a \"fatal: \" prefix, and it will return with 128 instead of -1,\nthis is so that caller can pass the return value to \"exit()\", instead\nof having to hardcode \"exit(128)\".\n\nA subsequent commit will migrate various callers that benefit from\nthis function over to it. For now we're just adding the routine and\nmaking die_builtin() in usage.c itself use it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-compat-util.h |  1 +\n usage.c           | 22 ++++++++++++++++++----\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c6bd2a84e55..a83fbdf6398 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -479,6 +479,7 @@ NORETURN void usage(const char *err);\n NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/usage.c b/usage.c\nindex c7d233b0de9..74b43c5db6f 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n \texit(129);\n }\n \n+static void die_message_builtin(const char *err, va_list params)\n+{\n+\ttrace2_cmd_error_va(err, params);\n+\tvreportf(\"fatal: \", err, params);\n+}\n+\n /*\n  * We call trace2_cmd_error_va() in the below functions first and\n  * expect it to va_copy 'params' before using it (because an 'ap' can\n@@ -62,10 +68,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n  */\n static NORETURN void die_builtin(const char *err, va_list params)\n {\n-\ttrace2_cmd_error_va(err, params);\n-\n-\tvreportf(\"fatal: \", err, params);\n-\n+\tdie_message_builtin(err, params);\n \texit(128);\n }\n \n@@ -211,6 +214,17 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n+#undef die_message\n+int die_message(const char *err, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, err);\n+\tdie_message_builtin(err, params);\n+\tva_end(params);\n+\treturn 128;\n+}\n+\n #undef error_errno\n int error_errno(const char *fmt, ...)\n {\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443175","messageId":"patch-4.4-e0e6427cbd3-20211206T165221Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com","subject":"[PATCH 4/4] config API: don't use vreportf(), make it static in usage.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-06T16:55:53Z","receivedAt":"2021-12-06T17:04:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preceding commits the rest of the vreportf() users outside of\nusage.c have been migrated to die_message(), leaving only the\ngit_die_config() function added in 5a80e97c827 (config: add\n`git_die_config()` to the config-set API, 2014-08-07).\n\nLet's have its callers call error() themselves if they want to emit a\nmessage, which is exactly what git_die_config() was doing for them\nbefore emitting its own die() message.\n\nThis means that we can make the vreportf() in usage.c \"static\", and\nonly expose functions such as usage(), die(), warning() etc.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/fast-import.c |  7 ++++---\n builtin/notes.c       |  6 ++++--\n config.c              | 22 +++++++++-------------\n config.h              | 10 ++++++----\n git-compat-util.h     |  1 -\n imap-send.c           |  3 ++-\n usage.c               |  2 +-\n 7 files changed, 26 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 2b2e28bad79..4e2432bb491 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -3456,9 +3456,10 @@ static void git_pack_config(void)\n \t}\n \tif (!git_config_get_int(\"pack.indexversion\", &indexversion_value)) {\n \t\tpack_idx_opts.version = indexversion_value;\n-\t\tif (pack_idx_opts.version > 2)\n-\t\t\tgit_die_config(\"pack.indexversion\",\n-\t\t\t\t\t\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n+\t\tif (pack_idx_opts.version > 2) {\n+\t\t\terror(\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n+\t\t\tgit_die_config(\"pack.indexversion\");\n+\t\t}\n \t}\n \tif (!git_config_get_ulong(\"pack.packsizelimit\", &packsizelimit_value))\n \t\tmax_packsize = packsizelimit_value;\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 2812d1eac40..60c5dab4122 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -763,8 +763,10 @@ static int git_config_get_notes_strategy(const char *key,\n \n \tif (git_config_get_string(key, &value))\n \t\treturn 1;\n-\tif (parse_notes_merge_strategy(value, strategy))\n-\t\tgit_die_config(key, _(\"unknown notes merge strategy %s\"), value);\n+\tif (parse_notes_merge_strategy(value, strategy)) {\n+\t\terror(_(\"unknown notes merge strategy %s\"), value);\n+\t\tgit_die_config(key);\n+\t}\n \n \tfree(value);\n \treturn 0;\ndiff --git a/config.c b/config.c\nindex c5873f3a706..30f7971e0cc 100644\n--- a/config.c\n+++ b/config.c\n@@ -2323,7 +2323,7 @@ int repo_config_get_string(struct repository *repo,\n \tgit_config_check_init(repo);\n \tret = git_configset_get_string(repo->config, key, dest);\n \tif (ret < 0)\n-\t\tgit_die_config(key, NULL);\n+\t\tgit_die_config(key);\n \treturn ret;\n }\n \n@@ -2334,7 +2334,7 @@ int repo_config_get_string_tmp(struct repository *repo,\n \tgit_config_check_init(repo);\n \tret = git_configset_get_string_tmp(repo->config, key, dest);\n \tif (ret < 0)\n-\t\tgit_die_config(key, NULL);\n+\t\tgit_die_config(key);\n \treturn ret;\n }\n \n@@ -2380,7 +2380,7 @@ int repo_config_get_pathname(struct repository *repo,\n \tgit_config_check_init(repo);\n \tret = git_configset_get_pathname(repo->config, key, dest);\n \tif (ret < 0)\n-\t\tgit_die_config(key, NULL);\n+\t\tgit_die_config(key);\n \treturn ret;\n }\n \n@@ -2452,8 +2452,10 @@ int git_config_get_expiry(const char *key, const char **output)\n \t\treturn ret;\n \tif (strcmp(*output, \"now\")) {\n \t\ttimestamp_t now = approxidate(\"now\");\n-\t\tif (approxidate(*output) >= now)\n-\t\t\tgit_die_config(key, _(\"Invalid %s: '%s'\"), key, *output);\n+\t\tif (approxidate(*output) >= now) {\n+\t\t\terror(_(\"Invalid %s: '%s'\"), key, *output);\n+\t\t\tgit_die_config(key);\n+\t\t}\n \t}\n \treturn ret;\n }\n@@ -2550,18 +2552,12 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)\n \t\t    key, filename, linenr);\n }\n \n-NORETURN __attribute__((format(printf, 2, 3)))\n-void git_die_config(const char *key, const char *err, ...)\n+NORETURN\n+void git_die_config(const char *key)\n {\n \tconst struct string_list *values;\n \tstruct key_value_info *kv_info;\n \n-\tif (err) {\n-\t\tva_list params;\n-\t\tva_start(params, err);\n-\t\tvreportf(\"error: \", err, params);\n-\t\tva_end(params);\n-\t}\n \tvalues = git_config_get_value_multi(key);\n \tkv_info = values->items[values->nr - 1].util;\n \tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\ndiff --git a/config.h b/config.h\nindex f119de01309..fae585d2005 100644\n--- a/config.h\n+++ b/config.h\n@@ -626,11 +626,13 @@ struct key_value_info {\n };\n \n /**\n- * First prints the error message specified by the caller in `err` and then\n- * dies printing the line number and the file name of the highest priority\n- * value for the configuration variable `key`.\n+ * Dies printing the line number and the file name of the highest\n+ * priority value for the configuration variable `key`.\n+ *\n+ * Consider calling error() first with a more specific formatted\n+ * message of your own.\n  */\n-NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));\n+NORETURN void git_die_config(const char *key);\n \n /**\n  * Helper function which formats the die error message according to the\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c6c6f7d6b51..bdb3977b9ec 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -474,7 +474,6 @@ static inline int git_has_dir_sep(const char *path)\n struct strbuf;\n \n /* General helper functions */\n-void vreportf(const char *prefix, const char *err, va_list params);\n NORETURN void usage(const char *err);\n NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/imap-send.c b/imap-send.c\nindex e6090a0346a..0fdfe5159eb 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1350,7 +1350,8 @@ static int git_imap_config(const char *var, const char *val, void *cb)\n \t\tserver.port = git_config_int(var, val);\n \telse if (!strcmp(\"imap.host\", var)) {\n \t\tif (!val) {\n-\t\t\tgit_die_config(\"imap.host\", \"Missing value for 'imap.host'\");\n+\t\t\terror(\"Missing value for 'imap.host'\");\n+\t\t\tgit_die_config(\"imap.host\");\n \t\t} else {\n \t\t\tif (starts_with(val, \"imap:\"))\n \t\t\t\tval += 5;\ndiff --git a/usage.c b/usage.c\nindex 3d09e8eea48..9943dd8742e 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,7 +6,7 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n-void vreportf(const char *prefix, const char *err, va_list params)\n+static void vreportf(const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443176","messageId":"patch-3.4-8747afecdcd-20211206T165221Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com","subject":"[PATCH 3/4] usage.c + gc: add and use a die_message_errno()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-06T16:55:52Z","receivedAt":"2021-12-06T17:04:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code the \"error: \" output when we exit with 128 due to gc.log\nerrors to use a \"fatal: \" prefix instead. This adds a sibling function\nto the die_errno() added in a preceding commit.\n\nSince it returns 128 instead of -1 (like die_message()) we'll need to\nadjust report_last_gc_error().\n\nLet's adjust it while we're at it to not conflate the \"should skip\"\nand \"exit with this non-zero code\" conditions, as the caller is no\nlonger hardcoding \"128\", but relying on die_errno() to return a\nnen-zero exit() status.\n\nSince we're touching this code let's also use \"return ret\" in place of\nan \"exit(ret)\". This is kinder to any cleanup our our \"cmd_main()\" in\n\"git.c\" might want to do.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/gc.c      | 21 ++++++++++++---------\n git-compat-util.h |  1 +\n usage.c           | 12 ++++++++++++\n 3 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bcef6a4c8da..7b451dfeefc 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n  * gc should not proceed due to an error in the last run. Prints a\n  * message and returns -1 if an error occurred while reading gc.log\n  */\n-static int report_last_gc_error(void)\n+static int report_last_gc_error(int *skip)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret = 0;\n \tssize_t len;\n \tstruct stat st;\n \tchar *gc_log_path = git_pathdup(\"gc.log\");\n+\t*skip = 0;\n \n \tif (stat(gc_log_path, &st)) {\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n+\t\tret = die_message_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t\tgoto done;\n \t}\n \n@@ -493,7 +494,7 @@ static int report_last_gc_error(void)\n \n \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (len < 0)\n-\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\t\tret = die_message_errno(_(\"cannot read '%s'\"), gc_log_path);\n \telse if (len > 0) {\n \t\t/*\n \t\t * A previous gc failed.  Report the error, and don't\n@@ -507,7 +508,7 @@ static int report_last_gc_error(void)\n \t\t\t       \"until the file is removed.\\n\\n\"\n \t\t\t       \"%s\"),\n \t\t\t    gc_log_path, sb.buf);\n-\t\tret = 1;\n+\t\t*skip = 1;\n \t}\n \tstrbuf_release(&sb);\n done:\n@@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n-\t\t\tint ret = report_last_gc_error();\n-\t\t\tif (ret < 0)\n-\t\t\t\t/* an I/O error occurred, already reported */\n-\t\t\t\texit(128);\n-\t\t\tif (ret == 1)\n+\t\t\tint skip;\n+\t\t\tint ret = report_last_gc_error(&skip);\n+\n+\t\t\tif (skip)\n \t\t\t\t/* Last gc --auto failed. Skip this one. */\n \t\t\t\treturn 0;\n+\t\t\tif (ret)\n+\t\t\t\t/* an I/O error occurred, already reported */\n+\t\t\t\treturn ret;\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d5e495529c8..c6c6f7d6b51 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -480,6 +480,7 @@ NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/usage.c b/usage.c\nindex 76399ba8409..3d09e8eea48 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -233,6 +233,18 @@ int die_message(const char *err, ...)\n \treturn 128;\n }\n \n+#undef die_message_errno\n+int die_message_errno(const char *fmt, ...)\n+{\n+\tchar buf[1024];\n+\tva_list params;\n+\n+\tva_start(params, fmt);\n+\tdie_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);\n+\tva_end(params);\n+\treturn 128;\n+}\n+\n #undef error_errno\n int error_errno(const char *fmt, ...)\n {\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443193","messageId":"xmqqee6pfrkd.fsf@gitster.g","threadId":"57040","inReplyTo":"patch-1.4-5a9ab85fa56-20211206T165221Z-avarab@gmail.com","subject":"Re: [PATCH 1/4] usage.c: add a die_message() routine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-06T19:42:42Z","receivedAt":"2021-12-06T19:42:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> +int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n\nThis probalby makes sense (we'll need to see the caller first).\n\n> +static void die_message_builtin(const char *err, va_list params)\n> +{\n> +\ttrace2_cmd_error_va(err, params);\n> +\tvreportf(\"fatal: \", err, params);\n> +}\n\nI thought the convention was *not* that we name the variant that\ntakes va_list with _builtin suffix, rather, I thought that the\nconvention is that ones with the _builtin suffix are meant to be\noverride-able by other code.\n\n>  /*\n>   * We call trace2_cmd_error_va() in the below functions first and\n>   * expect it to va_copy 'params' before using it (because an 'ap' can\n> @@ -62,10 +68,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n>   */\n>  static NORETURN void die_builtin(const char *err, va_list params)\n>  {\n> -\ttrace2_cmd_error_va(err, params);\n> -\n> -\tvreportf(\"fatal: \", err, params);\n> -\n> +\tdie_message_builtin(err, params);\n>  \texit(128);\n>  }\n\nAnd by making die() and die_message() both override-able, doesn't it\ncause confusion on the caller's end which one should get replaced?\nOr with die_message being overridable, we should rewrite the ones\nthat override die() to instead override die_message()?\n\nIt also makes readers wonder why this is not\n\n\texit(die_message(err, params));\n\nwhich I take it a sign that this new API is overly loose to allow a\nsimple single thing to be done in multiple ways.  Perhaps as the\nseries progresses, the picture might improve, but if that is the\ncase, perhaps the presentation order needs to be rethought.\nE.g. start without the _builtin that implies override-ability,\nconvert the existing code that can benefit from calling die_message(),\nand then finally introduce _builtin that is merely an implementation\ndetail, or something like that, perhaps?\n\nIn any case, the first step in this four patch series is not enough\nto evaluate if this step makes sense, so let's keep reading.\n\n> @@ -211,6 +214,17 @@ void NORETURN die_errno(const char *fmt, ...)\n>  \tva_end(params);\n>  }\n>  \n> +#undef die_message\n> +int die_message(const char *err, ...)\n> +{\n> +\tva_list params;\n> +\n> +\tva_start(params, err);\n> +\tdie_message_builtin(err, params);\n> +\tva_end(params);\n> +\treturn 128;\n> +}\n\nOK.\n\nThanks.\n\n\n>  #undef error_errno\n>  int error_errno(const char *fmt, ...)\n>  {\n"},{"id":"443194","messageId":"xmqqa6hdfreu.fsf@gitster.g","threadId":"57040","inReplyTo":"xmqqee6pfrkd.fsf@gitster.g","subject":"Re: [PATCH 1/4] usage.c: add a die_message() routine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-06T19:46:01Z","receivedAt":"2021-12-06T19:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> It also makes readers wonder why this is not\n>\n> \texit(die_message(err, params));\n>\n> which I take it a sign that this new API is overly loose to allow a\n> simple single thing to be done in multiple ways.  Perhaps as the\n> series progresses, the picture might improve, but if that is the\n> case, perhaps the presentation order needs to be rethought.\n> E.g. start without the _builtin that implies override-ability,\n> convert the existing code that can benefit from calling die_message(),\n> and then finally introduce _builtin that is merely an implementation\n> detail, or something like that, perhaps?\n>\n> In any case, the first step in this four patch series is not enough\n> to evaluate if this step makes sense, so let's keep reading.\n\nOK, it is the other way around.  This step with _builtin is\nincomplete introduction of the API, even though it was \"we add the\nnew API function without having any meaningful callers, because\ncombining them together into a single patch is too much to chew in a\nsingle sitting\".  A small part of 2/4 that adds the override-able\ndie_message_routine should be part of this step to make this step\nunderstandable.\n\nTHanks.\n\n"},{"id":"443195","messageId":"xmqqy24xec6v.fsf@gitster.g","threadId":"57040","inReplyTo":"patch-2.4-36c050c90c2-20211206T165221Z-avarab@gmail.com","subject":"Re: [PATCH 2/4] usage.c API users: use die_message() where appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-06T20:00:08Z","receivedAt":"2021-12-06T20:00:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n>  static NORETURN void die_nicely(const char *err, va_list params)\n>  {\n> +\tva_list cp;\n>  \tstatic int zombie;\n> -\tchar message[2 * PATH_MAX];\n> +\treport_fn die_message_fn = get_die_message_routine();\n>  \n> -\tvsnprintf(message, sizeof(message), err, params);\n> -\tfputs(\"fatal: \", stderr);\n> -\tfputs(message, stderr);\n> -\tfputc('\\n', stderr);\n> +\tva_copy(cp, params);\n> +\tdie_message_fn(err, params);\n>  \n>  \tif (!zombie) {\n> +\t\tchar message[2 * PATH_MAX];\n> +\n>  \t\tzombie = 1;\n> +\t\tvsnprintf(message, sizeof(message), err, cp);\n>  \t\twrite_crash_report(message);\n>  \t\tend_packfile();\n>  \t\tunkeep_all_packs();\n\nSo, we used to compose the die-looking \"fatal:\" message by hand, but\nwe now grab the function \"die()\" that is currently in effect uses to\nprepare its message, and let it compose the message for us.  It will\nwork even when somebody else were overriding die_message_routine to\nreuse their custome die_message_routine, which is nice.  And because\nthis function is used to override die_routine, we do not have to be\nworrying about somebody else overriding die_routine without\noverriding die_message_routine.\n\nOK.\n\n\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 71c59583a17..2812d1eac40 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n>  static void write_note_data(struct note_data *d, struct object_id *oid)\n>  {\n>  \tif (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {\n> -\t\terror(_(\"unable to write note object\"));\n> +\t\tint status = die_message(_(\"unable to write note object\"));\n> +\n>  \t\tif (d->edit_path)\n> -\t\t\terror(_(\"the note contents have been left in %s\"),\n> -\t\t\t\td->edit_path);\n> -\t\texit(128);\n> +\t\t\tdie_message(_(\"the note contents have been left in %s\"),\n> +\t\t\t\t    d->edit_path);\n> +\t\texit(status);\n\nOK.  This changes the message when we terminate from \"error:\" to\n\"fatal:\", but I can buy that we could argue that the original was\nabusing the error() to work around the lack of die_message().\n\n> diff --git a/http-backend.c b/http-backend.c\n> index 3d6e2ff17f8..982cb62c7cb 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -659,8 +659,9 @@ static NORETURN void die_webcgi(const char *err, va_list params)\n>  {\n>  \tif (dead <= 1) {\n>  \t\tstruct strbuf hdr = STRBUF_INIT;\n> +\t\treport_fn die_message_fn = get_die_message_routine();\n>  \n> -\t\tvreportf(\"fatal: \", err, params);\n> +\t\tdie_message_fn(err, params);\n>  \n>  \t\thttp_status(&hdr, 500, \"Internal Server Error\");\n>  \t\thdr_nocache(&hdr);\n\nOK.  As there is a known change that wants to touch this file, it is\nunfortunate that you chose to do this now, but the above makes sense.\nIt is a quite straight-forward clean-up.\n\n> diff --git a/parse-options.c b/parse-options.c\n> index fc5b43ff0b2..8bc0a21f1d7 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -1075,6 +1075,6 @@ void NORETURN usage_msg_opt(const char *msg,\n>  \t\t   const char * const *usagestr,\n>  \t\t   const struct option *options)\n>  {\n> -\tfprintf(stderr, \"fatal: %s\\n\\n\", msg);\n> +\tdie_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n>  \tusage_with_options(usagestr, options);\n>  }\n\nOK.\n\n> diff --git a/run-command.c b/run-command.c\n> ...\n> @@ -372,9 +363,10 @@ static void NORETURN child_die_fn(const char *err, va_list params)\n>  static void child_err_spew(struct child_process *cmd, struct child_err *cerr)\n>  {\n>  \tstatic void (*old_errfn)(const char *err, va_list params);\n> +\treport_fn die_message_routine = get_die_message_routine();\n>  \n>  \told_errfn = get_error_routine();\n> -\tset_error_routine(fake_fatal);\n> +\tset_error_routine(die_message_routine);\n>  \terrno = cerr->syserr;\n\nOK.  This is literally equivalent to the original.\n\n> @@ -1082,7 +1074,9 @@ static void *run_thread(void *data)\n>  \n>  static NORETURN void die_async(const char *err, va_list params)\n>  {\n> -\tvreportf(\"fatal: \", err, params);\n> +\treport_fn die_message_fn = get_die_message_routine();\n> +\n> +\tdie_message_fn(err, params);\n\nLikewise.\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index a83fbdf6398..d5e495529c8 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -514,6 +514,7 @@ static inline int const_error(void)\n>  typedef void (*report_fn)(const char *, va_list params);\n>  \n>  void set_die_routine(NORETURN_PTR report_fn routine);\n> +report_fn get_die_message_routine(void);\n>  void set_error_routine(report_fn routine);\n>  report_fn get_error_routine(void);\n>  void set_warn_routine(report_fn routine);\n> diff --git a/usage.c b/usage.c\n> index 74b43c5db6f..76399ba8409 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -68,7 +68,9 @@ static void die_message_builtin(const char *err, va_list params)\n>   */\n>  static NORETURN void die_builtin(const char *err, va_list params)\n>  {\n> -\tdie_message_builtin(err, params);\n> +\treport_fn die_message_fn = get_die_message_routine();\n> +\n> +\tdie_message_fn(err, params);\n>  \texit(128);\n>  }\n>  \n> @@ -112,6 +114,7 @@ static int die_is_recursing_builtin(void)\n>   * (ugh), so keep things static. */\n>  static NORETURN_PTR report_fn usage_routine = usage_builtin;\n>  static NORETURN_PTR report_fn die_routine = die_builtin;\n> +static report_fn die_message_routine = die_message_builtin;\n>  static report_fn error_routine = error_builtin;\n>  static report_fn warn_routine = warn_builtin;\n>  static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n> @@ -121,6 +124,11 @@ void set_die_routine(NORETURN_PTR report_fn routine)\n>  \tdie_routine = routine;\n>  }\n>  \n> +report_fn get_die_message_routine(void)\n> +{\n> +\treturn die_message_routine;\n> +}\n> +\n>  void set_error_routine(report_fn routine)\n>  {\n>  \terror_routine = routine;\n> @@ -220,7 +228,7 @@ int die_message(const char *err, ...)\n>  \tva_list params;\n>  \n>  \tva_start(params, err);\n> -\tdie_message_builtin(err, params);\n> +\tdie_message_routine(err, params);\n>  \tva_end(params);\n>  \treturn 128;\n>  }\n\nI think these hunks are better fit in the previous step.  This is a\n\"now we have a new set of API functions, let's convert the existing\ncode to take advantage of them\" step, and \"oops, what we presented\nas a new set of API functions in the previous step was incomplete,\nlet's patch them up\" should not be included in there.\n\nThanks.\n"},{"id":"443198","messageId":"xmqqo85te8j2.fsf@gitster.g","threadId":"57040","inReplyTo":"patch-3.4-8747afecdcd-20211206T165221Z-avarab@gmail.com","subject":"Re: [PATCH 3/4] usage.c + gc: add and use a die_message_errno()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-06T21:19:13Z","receivedAt":"2021-12-06T21:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change code the \"error: \" output when we exit with 128 due to gc.log\n> errors to use a \"fatal: \" prefix instead. This adds a sibling function\n> to the die_errno() added in a preceding commit.\n>\n> Since it returns 128 instead of -1 (like die_message()) we'll need to\n> adjust report_last_gc_error().\n>\n> Let's adjust it while we're at it to not conflate the \"should skip\"\n> and \"exit with this non-zero code\" conditions, as the caller is no\n> longer hardcoding \"128\", but relying on die_errno() to return a\n> nen-zero exit() status.\n>\n> Since we're touching this code let's also use \"return ret\" in place of\n> an \"exit(ret)\". This is kinder to any cleanup our our \"cmd_main()\" in\n> \"git.c\" might want to do.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/gc.c      | 21 ++++++++++++---------\n>  git-compat-util.h |  1 +\n>  usage.c           | 12 ++++++++++++\n>  3 files changed, 25 insertions(+), 9 deletions(-)\n\nThe structure of the series makes sense here.\n\n1 and 2 were structured to add new set of API functions in 1 and to\nmake use of them in 2 (which is also OK if done right; as pointed\nout, a few changes need to be moved from 2 to 1 to make 1 complete).\n\nThis makes further addition and adds its uses at the same time.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index bcef6a4c8da..7b451dfeefc 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>   * gc should not proceed due to an error in the last run. Prints a\n>   * message and returns -1 if an error occurred while reading gc.log\n>   */\n> -static int report_last_gc_error(void)\n> +static int report_last_gc_error(int *skip)\n>  {\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \tint ret = 0;\n>  \tssize_t len;\n>  \tstruct stat st;\n>  \tchar *gc_log_path = git_pathdup(\"gc.log\");\n> +\t*skip = 0;\n>  \n>  \tif (stat(gc_log_path, &st)) {\n\nInsert a new statement after the blank that separates the\ndeclaration from the statements.\n\n>  \t\tif (errno == ENOENT)\n>  \t\t\tgoto done;\n>  \n> -\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> +\t\tret = die_message_errno(_(\"cannot stat '%s'\"), gc_log_path);\n>  \t\tgoto done;\n>  \t}\n>  \n> @@ -493,7 +494,7 @@ static int report_last_gc_error(void)\n>  \n>  \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n>  \tif (len < 0)\n> -\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n> +\t\tret = die_message_errno(_(\"cannot read '%s'\"), gc_log_path);\n>  \telse if (len > 0) {\n>  \t\t/*\n>  \t\t * A previous gc failed.  Report the error, and don't\n> @@ -507,7 +508,7 @@ static int report_last_gc_error(void)\n>  \t\t\t       \"until the file is removed.\\n\\n\"\n>  \t\t\t       \"%s\"),\n>  \t\t\t    gc_log_path, sb.buf);\n> -\t\tret = 1;\n> +\t\t*skip = 1;\n>  \t}\n>  \tstrbuf_release(&sb);\n>  done:\n\nNobody called die() in here, so we always returned to the caller.\nIt feels a bit strange that we need, in addition to the existing\n'ret' that becomes the return value, an extra out parameter *skip.\n\nLet's see why the caller needs both.\n\n\n> @@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n>  \t\t}\n>  \t\tif (detach_auto) {\n> -\t\t\tint ret = report_last_gc_error();\n> -\t\t\tif (ret < 0)\n> -\t\t\t\t/* an I/O error occurred, already reported */\n> -\t\t\t\texit(128);\n> -\t\t\tif (ret == 1)\n> +\t\t\tint skip;\n> +\t\t\tint ret = report_last_gc_error(&skip);\n> +\n> +\t\t\tif (skip)\n>  \t\t\t\t/* Last gc --auto failed. Skip this one. */\n>  \t\t\t\treturn 0;\n> +\t\t\tif (ret)\n> +\t\t\t\t/* an I/O error occurred, already reported */\n> +\t\t\t\treturn ret;\n\nWe used to use the fact that \n\n - negative return was from error_errno() to cuase exit(128) to\n   emulate die.\n\n - return with 1 is from the \"previous gc failed\" block and declare\n   we can safely return 0 (success).\n\n - return with 0 is sucess and nothing needed to be, and was, done.\n\nSo, I still do not see the point of this change to add an extra\nerror channel.  I do not find this argument\n\n> ... as the caller is no\n> longer hardcoding \"128\", but relying on die_errno() to return a\n> nen-zero exit() status.\n\nparticularly a convincing one.  We are not obliged to assign the\nvalue returned from die_message_errno() to 'ret' to begin with.\nreport_last_gc_error() can document what its return values mean\nand the caller can react to it.\n\nIndeed, if we assign -1 to 'ret' where we used to call error_errno(),\nthe caller needs no changes at all, no?\n\nSomething like this as a replacement for this part?\n\n builtin/gc.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git c/builtin/gc.c w/builtin/gc.c\nindex f7270b26ea..2b54465608 100644\n--- c/builtin/gc.c\n+++ w/builtin/gc.c\n@@ -477,47 +477,49 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n /*\n  * Returns 0 if there was no previous error and gc can proceed, 1 if\n  * gc should not proceed due to an error in the last run. Prints a\n  * message and returns -1 if an error occurred while reading gc.log\n  */\n static int report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret = 0;\n+\tint ret = -1; /* a pessimist */\n \tssize_t len;\n \tstruct stat st;\n \tchar *gc_log_path = git_pathdup(\"gc.log\");\n \n \tif (stat(gc_log_path, &st)) {\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n+\t\tdie_message_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t\tgoto done;\n \t}\n \n-\tif (st.st_mtime < gc_log_expire_time)\n+\tif (st.st_mtime < gc_log_expire_time) {\n+\t\tret = 0;\n \t\tgoto done;\n+\t}\n \n \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (len < 0)\n-\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\t\tdie_message_errno(_(\"cannot read '%s'\"), gc_log_path);\n \telse if (len > 0) {\n \t\t/*\n \t\t * A previous gc failed.  Report the error, and don't\n \t\t * bother with an automatic gc run since it is likely\n \t\t * to fail in the same way.\n \t\t */\n \t\twarning(_(\"The last gc run reported the following. \"\n \t\t\t       \"Please correct the root cause\\n\"\n \t\t\t       \"and remove %s\\n\"\n \t\t\t       \"Automatic cleanup will not be performed \"\n \t\t\t       \"until the file is removed.\\n\\n\"\n \t\t\t       \"%s\"),\n \t\t\t    gc_log_path, sb.buf);\n-\t\tret = 1;\n+\t\tret = 0;\n \t}\n \tstrbuf_release(&sb);\n done:\n \tfree(gc_log_path);\n \treturn ret;\n }\n"},{"id":"443199","messageId":"xmqqfsr5e87e.fsf@gitster.g","threadId":"57040","inReplyTo":"patch-4.4-e0e6427cbd3-20211206T165221Z-avarab@gmail.com","subject":"Re: [PATCH 4/4] config API: don't use vreportf(), make it static in usage.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-06T21:26:13Z","receivedAt":"2021-12-06T21:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> In preceding commits the rest of the vreportf() users outside of\n> usage.c have been migrated to die_message(), leaving only the\n> git_die_config() function added in 5a80e97c827 (config: add\n> `git_die_config()` to the config-set API, 2014-08-07).\n>\n> Let's have its callers call error() themselves if they want to emit a\n> message, which is exactly what git_die_config() was doing for them\n> before emitting its own die() message.\n\nI do not quite get this.  If git_die_config() has been showing the\nmessage for them, and if the existing callers can just use error(),\nwhy not git_die_config() call error() on behalf of these callers?\n\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 2b2e28bad79..4e2432bb491 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/fast-import.c\n> @@ -3456,9 +3456,10 @@ static void git_pack_config(void)\n>  \t}\n>  \tif (!git_config_get_int(\"pack.indexversion\", &indexversion_value)) {\n>  \t\tpack_idx_opts.version = indexversion_value;\n> -\t\tif (pack_idx_opts.version > 2)\n> -\t\t\tgit_die_config(\"pack.indexversion\",\n> -\t\t\t\t\t\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n> +\t\tif (pack_idx_opts.version > 2) {\n> +\t\t\terror(\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n> +\t\t\tgit_die_config(\"pack.indexversion\");\n> +\t\t}\n\nThis is exactly what triggered the question above, and the pattern\nrepeats elsewhere, too.\n\n> @@ -2550,18 +2552,12 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)\n>  \t\t    key, filename, linenr);\n>  }\n>  \n> -NORETURN __attribute__((format(printf, 2, 3)))\n> -void git_die_config(const char *key, const char *err, ...)\n> +NORETURN\n> +void git_die_config(const char *key)\n>  {\n>  \tconst struct string_list *values;\n>  \tstruct key_value_info *kv_info;\n>  \n> -\tif (err) {\n> -\t\tva_list params;\n> -\t\tva_start(params, err);\n> -\t\tvreportf(\"error: \", err, params);\n> -\t\tva_end(params);\n\nI get that we do not want to expose vreportf() to this caller, and I\nagree with the goal, but wouldn't it be the matter of calling\nget_error_routine() and calling it with err and params here, instead\nof losing the whole block?  Is that insufficient to avoid toucing\nall the callers?\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index c6c6f7d6b51..bdb3977b9ec 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -474,7 +474,6 @@ static inline int git_has_dir_sep(const char *path)\n>  struct strbuf;\n>  \n>  /* General helper functions */\n> -void vreportf(const char *prefix, const char *err, va_list params);\n\nGood.\n\n> diff --git a/usage.c b/usage.c\n> index 3d09e8eea48..9943dd8742e 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -6,7 +6,7 @@\n>  #include \"git-compat-util.h\"\n>  #include \"cache.h\"\n>  \n> -void vreportf(const char *prefix, const char *err, va_list params)\n> +static void vreportf(const char *prefix, const char *err, va_list params)\n\nGood, too.\n\nThanks.\n"},{"id":"443316","messageId":"211207.86fsr4nvbk.gmgdl@evledraar.gmail.com","threadId":"57040","inReplyTo":"xmqqfsr5e87e.fsf@gitster.g","subject":"Re: [PATCH 4/4] config API: don't use vreportf(), make it static in usage.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:05:39Z","receivedAt":"2021-12-07T18:06:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 06 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> In preceding commits the rest of the vreportf() users outside of\n>> usage.c have been migrated to die_message(), leaving only the\n>> git_die_config() function added in 5a80e97c827 (config: add\n>> `git_die_config()` to the config-set API, 2014-08-07).\n>>\n>> Let's have its callers call error() themselves if they want to emit a\n>> message, which is exactly what git_die_config() was doing for them\n>> before emitting its own die() message.\n>\n> I do not quite get this.  If git_die_config() has been showing the\n> message for them, and if the existing callers can just use error(),\n> why not git_die_config() call error() on behalf of these callers?\n>\n>> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n>> index 2b2e28bad79..4e2432bb491 100644\n>> --- a/builtin/fast-import.c\n>> +++ b/builtin/fast-import.c\n>> @@ -3456,9 +3456,10 @@ static void git_pack_config(void)\n>>  \t}\n>>  \tif (!git_config_get_int(\"pack.indexversion\", &indexversion_value)) {\n>>  \t\tpack_idx_opts.version = indexversion_value;\n>> -\t\tif (pack_idx_opts.version > 2)\n>> -\t\t\tgit_die_config(\"pack.indexversion\",\n>> -\t\t\t\t\t\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n>> +\t\tif (pack_idx_opts.version > 2) {\n>> +\t\t\terror(\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n>> +\t\t\tgit_die_config(\"pack.indexversion\");\n>> +\t\t}\n>\n> This is exactly what triggered the question above, and the pattern\n> repeats elsewhere, too.\n>\n>> @@ -2550,18 +2552,12 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)\n>>  \t\t    key, filename, linenr);\n>>  }\n>>  \n>> -NORETURN __attribute__((format(printf, 2, 3)))\n>> -void git_die_config(const char *key, const char *err, ...)\n>> +NORETURN\n>> +void git_die_config(const char *key)\n>>  {\n>>  \tconst struct string_list *values;\n>>  \tstruct key_value_info *kv_info;\n>>  \n>> -\tif (err) {\n>> -\t\tva_list params;\n>> -\t\tva_start(params, err);\n>> -\t\tvreportf(\"error: \", err, params);\n>> -\t\tva_end(params);\n>\n> I get that we do not want to expose vreportf() to this caller, and I\n> agree with the goal, but wouldn't it be the matter of calling\n> get_error_routine() and calling it with err and params here, instead\n> of losing the whole block?  Is that insufficient to avoid toucing\n> all the callers?\n\nI'll fix that in the incoming re-roll. This really didn't belong in this\nseries but a later one.\n\nFWIW that change was because like this we'll accurately report the\nsource of the \"error\" when the function learns to spew <file>:<line> at\nthe user.\n\nBut for now I'll just have it do as you suggest...\n"},{"id":"443328","messageId":"patch-v2-1.6-65ae6fe7cbe-20211207T182419Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"[PATCH v2 1/6] usage.c: add a die_message() routine","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:26:29Z","receivedAt":"2021-12-07T18:26:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We have code in various places that would like to call die(), but\nwants to defer the exit(128) it would invoke, e.g. to print an\nadditional message, or adjust the exit code. Add a die_message()\nhelper routine to bridge this gap in the API.\n\nFunctionally this behaves just like the error() routine, except it'll\nprint a \"fatal: \" prefix, and it will return with 128 instead of -1,\nthis is so that caller can pass the return value to \"exit()\", instead\nof having to hardcode \"exit(128)\".\n\nNote that as with the other routines the \"die_message_builtin\" needs\nto return \"void\" and otherwise conform to the \"report_fn\"\nsignature.\n\nAs we'll see in a subsequent commit callers will want to replace\ne.g. their default \"die_routine\" with a \"die_message_routine\".\n\nFor now we're just adding the routine and making die_builtin() in\nusage.c itself use it. In order to do that we need to add a\nget_die_message_routine() function, which works like the other\nget_*_routine() functions in usage.c. There is no\nset_die_message_rotine(), as it hasn't been needed yet. We can add it\nif we ever need it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-compat-util.h |  2 ++\n usage.c           | 28 +++++++++++++++++++++++++---\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c6bd2a84e55..d5e495529c8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -479,6 +479,7 @@ NORETURN void usage(const char *err);\n NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n@@ -513,6 +514,7 @@ static inline int const_error(void)\n typedef void (*report_fn)(const char *, va_list params);\n \n void set_die_routine(NORETURN_PTR report_fn routine);\n+report_fn get_die_message_routine(void);\n void set_error_routine(report_fn routine);\n report_fn get_error_routine(void);\n void set_warn_routine(report_fn routine);\ndiff --git a/usage.c b/usage.c\nindex c7d233b0de9..76399ba8409 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n \texit(129);\n }\n \n+static void die_message_builtin(const char *err, va_list params)\n+{\n+\ttrace2_cmd_error_va(err, params);\n+\tvreportf(\"fatal: \", err, params);\n+}\n+\n /*\n  * We call trace2_cmd_error_va() in the below functions first and\n  * expect it to va_copy 'params' before using it (because an 'ap' can\n@@ -62,10 +68,9 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n  */\n static NORETURN void die_builtin(const char *err, va_list params)\n {\n-\ttrace2_cmd_error_va(err, params);\n-\n-\tvreportf(\"fatal: \", err, params);\n+\treport_fn die_message_fn = get_die_message_routine();\n \n+\tdie_message_fn(err, params);\n \texit(128);\n }\n \n@@ -109,6 +114,7 @@ static int die_is_recursing_builtin(void)\n  * (ugh), so keep things static. */\n static NORETURN_PTR report_fn usage_routine = usage_builtin;\n static NORETURN_PTR report_fn die_routine = die_builtin;\n+static report_fn die_message_routine = die_message_builtin;\n static report_fn error_routine = error_builtin;\n static report_fn warn_routine = warn_builtin;\n static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n@@ -118,6 +124,11 @@ void set_die_routine(NORETURN_PTR report_fn routine)\n \tdie_routine = routine;\n }\n \n+report_fn get_die_message_routine(void)\n+{\n+\treturn die_message_routine;\n+}\n+\n void set_error_routine(report_fn routine)\n {\n \terror_routine = routine;\n@@ -211,6 +222,17 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n+#undef die_message\n+int die_message(const char *err, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, err);\n+\tdie_message_routine(err, params);\n+\tva_end(params);\n+\treturn 128;\n+}\n+\n #undef error_errno\n int error_errno(const char *fmt, ...)\n {\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443329","messageId":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com","subject":"[PATCH v2 0/6] usage API: Add and use die_message()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:26:28Z","receivedAt":"2021-12-07T18:26:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A small set of fixes to usage.[ch]'s API use that will go towards\nenabling nicer things down the road. See [1] for the v1 summary\n\nI believe this should address all the feedback Junio had on the\nv1.\n\nAside from the substantially rewritten 6/6 and much simplified 4/6 the\nend-state is almost the same, but things are better split up,\nexplained etc. now.\n\n1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (6):\n  usage.c: add a die_message() routine\n  usage.c API users: use die_message() for \"fatal :\" + exit 128\n  usage.c API users: use die_message() for error() + exit 128\n  gc: return from cmd_gc(), don't call exit()\n  usage.c + gc: add and use a die_message_errno()\n  config API: use get_error_routine(), not vreportf()\n\n builtin/fast-import.c | 12 +++++++-----\n builtin/gc.c          | 14 ++++++++------\n builtin/notes.c       |  9 +++++----\n config.c              |  3 ++-\n git-compat-util.h     |  4 +++-\n http-backend.c        |  3 ++-\n parse-options.c       |  2 +-\n run-command.c         | 16 +++++-----------\n usage.c               | 42 ++++++++++++++++++++++++++++++++++++++----\n 9 files changed, 71 insertions(+), 34 deletions(-)\n\nRange-diff against v1:\n1:  5a9ab85fa56 ! 1:  65ae6fe7cbe usage.c: add a die_message() routine\n    @@ Commit message\n         this is so that caller can pass the return value to \"exit()\", instead\n         of having to hardcode \"exit(128)\".\n     \n    -    A subsequent commit will migrate various callers that benefit from\n    -    this function over to it. For now we're just adding the routine and\n    -    making die_builtin() in usage.c itself use it.\n    +    Note that as with the other routines the \"die_message_builtin\" needs\n    +    to return \"void\" and otherwise conform to the \"report_fn\"\n    +    signature.\n    +\n    +    As we'll see in a subsequent commit callers will want to replace\n    +    e.g. their default \"die_routine\" with a \"die_message_routine\".\n    +\n    +    For now we're just adding the routine and making die_builtin() in\n    +    usage.c itself use it. In order to do that we need to add a\n    +    get_die_message_routine() function, which works like the other\n    +    get_*_routine() functions in usage.c. There is no\n    +    set_die_message_rotine(), as it hasn't been needed yet. We can add it\n    +    if we ever need it.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    @@ git-compat-util.h: NORETURN void usage(const char *err);\n      int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n      int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n      void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n    +@@ git-compat-util.h: static inline int const_error(void)\n    + typedef void (*report_fn)(const char *, va_list params);\n    + \n    + void set_die_routine(NORETURN_PTR report_fn routine);\n    ++report_fn get_die_message_routine(void);\n    + void set_error_routine(report_fn routine);\n    + report_fn get_error_routine(void);\n    + void set_warn_routine(report_fn routine);\n     \n      ## usage.c ##\n     @@ usage.c: static NORETURN void usage_builtin(const char *err, va_list params)\n    @@ usage.c: static NORETURN void usage_builtin(const char *err, va_list params)\n     -\ttrace2_cmd_error_va(err, params);\n     -\n     -\tvreportf(\"fatal: \", err, params);\n    --\n    -+\tdie_message_builtin(err, params);\n    ++\treport_fn die_message_fn = get_die_message_routine();\n    + \n    ++\tdie_message_fn(err, params);\n      \texit(128);\n      }\n      \n    +@@ usage.c: static int die_is_recursing_builtin(void)\n    +  * (ugh), so keep things static. */\n    + static NORETURN_PTR report_fn usage_routine = usage_builtin;\n    + static NORETURN_PTR report_fn die_routine = die_builtin;\n    ++static report_fn die_message_routine = die_message_builtin;\n    + static report_fn error_routine = error_builtin;\n    + static report_fn warn_routine = warn_builtin;\n    + static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n    +@@ usage.c: void set_die_routine(NORETURN_PTR report_fn routine)\n    + \tdie_routine = routine;\n    + }\n    + \n    ++report_fn get_die_message_routine(void)\n    ++{\n    ++\treturn die_message_routine;\n    ++}\n    ++\n    + void set_error_routine(report_fn routine)\n    + {\n    + \terror_routine = routine;\n     @@ usage.c: void NORETURN die_errno(const char *fmt, ...)\n      \tva_end(params);\n      }\n    @@ usage.c: void NORETURN die_errno(const char *fmt, ...)\n     +\tva_list params;\n     +\n     +\tva_start(params, err);\n    -+\tdie_message_builtin(err, params);\n    ++\tdie_message_routine(err, params);\n     +\tva_end(params);\n     +\treturn 128;\n     +}\n2:  36c050c90c2 ! 2:  f5a98901498 usage.c API users: use die_message() where appropriate\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    usage.c API users: use die_message() where appropriate\n    +    usage.c API users: use die_message() for \"fatal :\" + exit 128\n     \n    -    Change code that either called error() and proceeded to exit with 128,\n    -    or emitted its own \"fatal: \" messages to use the die_message()\n    -    function added in a preceding commit.\n    +    Change code that printed its own \"fatal: \" message and exited with a\n    +    status code of 128 to use the die_message() function added in a\n    +    preceding commit.\n     \n    -    In order to do that we need to add a get_die_message_routine()\n    -    function, which works like the other get_*_routine() functions in\n    -    usage.c. There is no set_die_message_rotine(), as it hasn't been\n    -    needed yet. We can add it if we ever need it.\n    +    This change also demonstrates why the return value of\n    +    die_message_routine() needed to be that of \"report_fn\". We have\n    +    callers such as the run-command.c::child_err_spew() which would like\n    +    to replace its error routine with the return value of\n    +    \"get_die_message_routine()\".\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    @@ builtin/fast-import.c: static void dump_marks(void);\n      \t\tend_packfile();\n      \t\tunkeep_all_packs();\n     \n    - ## builtin/notes.c ##\n    -@@ builtin/notes.c: static void prepare_note_data(const struct object_id *object, struct note_data *\n    - static void write_note_data(struct note_data *d, struct object_id *oid)\n    - {\n    - \tif (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {\n    --\t\terror(_(\"unable to write note object\"));\n    -+\t\tint status = die_message(_(\"unable to write note object\"));\n    -+\n    - \t\tif (d->edit_path)\n    --\t\t\terror(_(\"the note contents have been left in %s\"),\n    --\t\t\t\td->edit_path);\n    --\t\texit(128);\n    -+\t\t\tdie_message(_(\"the note contents have been left in %s\"),\n    -+\t\t\t\t    d->edit_path);\n    -+\t\texit(status);\n    - \t}\n    - }\n    - \n    -\n    - ## git-compat-util.h ##\n    -@@ git-compat-util.h: static inline int const_error(void)\n    - typedef void (*report_fn)(const char *, va_list params);\n    - \n    - void set_die_routine(NORETURN_PTR report_fn routine);\n    -+report_fn get_die_message_routine(void);\n    - void set_error_routine(report_fn routine);\n    - report_fn get_error_routine(void);\n    - void set_warn_routine(report_fn routine);\n    -\n      ## http-backend.c ##\n     @@ http-backend.c: static NORETURN void die_webcgi(const char *err, va_list params)\n      {\n    @@ run-command.c: static void *run_thread(void *data)\n      \n      \tif (in_async()) {\n      \t\tstruct async *async = pthread_getspecific(async_key);\n    -\n    - ## usage.c ##\n    -@@ usage.c: static void die_message_builtin(const char *err, va_list params)\n    -  */\n    - static NORETURN void die_builtin(const char *err, va_list params)\n    - {\n    --\tdie_message_builtin(err, params);\n    -+\treport_fn die_message_fn = get_die_message_routine();\n    -+\n    -+\tdie_message_fn(err, params);\n    - \texit(128);\n    - }\n    - \n    -@@ usage.c: static int die_is_recursing_builtin(void)\n    -  * (ugh), so keep things static. */\n    - static NORETURN_PTR report_fn usage_routine = usage_builtin;\n    - static NORETURN_PTR report_fn die_routine = die_builtin;\n    -+static report_fn die_message_routine = die_message_builtin;\n    - static report_fn error_routine = error_builtin;\n    - static report_fn warn_routine = warn_builtin;\n    - static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n    -@@ usage.c: void set_die_routine(NORETURN_PTR report_fn routine)\n    - \tdie_routine = routine;\n    - }\n    - \n    -+report_fn get_die_message_routine(void)\n    -+{\n    -+\treturn die_message_routine;\n    -+}\n    -+\n    - void set_error_routine(report_fn routine)\n    - {\n    - \terror_routine = routine;\n    -@@ usage.c: int die_message(const char *err, ...)\n    - \tva_list params;\n    - \n    - \tva_start(params, err);\n    --\tdie_message_builtin(err, params);\n    -+\tdie_message_routine(err, params);\n    - \tva_end(params);\n    - \treturn 128;\n    - }\n-:  ----------- > 3:  c7d67fd41fa usage.c API users: use die_message() for error() + exit 128\n-:  ----------- > 4:  f224a281a10 gc: return from cmd_gc(), don't call exit()\n3:  8747afecdcd ! 5:  2b4a3910654 usage.c + gc: add and use a die_message_errno()\n    @@ Metadata\n      ## Commit message ##\n         usage.c + gc: add and use a die_message_errno()\n     \n    -    Change code the \"error: \" output when we exit with 128 due to gc.log\n    -    errors to use a \"fatal: \" prefix instead. This adds a sibling function\n    -    to the die_errno() added in a preceding commit.\n    +    Change the \"error: \" output when we exit with 128 due to gc.log errors\n    +    to use a \"fatal: \" prefix instead. To do this add a\n    +    die_message_errno() a sibling function to the die_errno() added in a\n    +    preceding commit.\n     \n    -    Since it returns 128 instead of -1 (like die_message()) we'll need to\n    -    adjust report_last_gc_error().\n    +    Before this we'd expect report_last_gc_error() to return -1 from\n    +    error_errno() in this case. It already treated a status of 0 and 1\n    +    specially. Let's just document that anything that's not 0 or 1 should\n    +    be returned.\n     \n    -    Let's adjust it while we're at it to not conflate the \"should skip\"\n    -    and \"exit with this non-zero code\" conditions, as the caller is no\n    -    longer hardcoding \"128\", but relying on die_errno() to return a\n    -    nen-zero exit() status.\n    -\n    -    Since we're touching this code let's also use \"return ret\" in place of\n    -    an \"exit(ret)\". This is kinder to any cleanup our our \"cmd_main()\" in\n    -    \"git.c\" might want to do.\n    +    We could also retain the \"ret < 0\" behavior here without hardcoding\n    +    128 by returning -128, and having the caller do a \"return -ret\", but I\n    +    think this makes more sense, and preserves the path from\n    +    die_message*()'s return value to the \"return\" without hardcoding\n    +    \"128\".\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## builtin/gc.c ##\n     @@ builtin/gc.c: static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n    + /*\n    +  * Returns 0 if there was no previous error and gc can proceed, 1 if\n       * gc should not proceed due to an error in the last run. Prints a\n    -  * message and returns -1 if an error occurred while reading gc.log\n    +- * message and returns -1 if an error occurred while reading gc.log\n    ++ * message and returns with a non-[01] status code if an error occurred\n    ++ * while reading gc.log\n       */\n    --static int report_last_gc_error(void)\n    -+static int report_last_gc_error(int *skip)\n    + static int report_last_gc_error(void)\n      {\n    - \tstruct strbuf sb = STRBUF_INIT;\n    - \tint ret = 0;\n    - \tssize_t len;\n    - \tstruct stat st;\n    - \tchar *gc_log_path = git_pathdup(\"gc.log\");\n    -+\t*skip = 0;\n    - \n    - \tif (stat(gc_log_path, &st)) {\n    +@@ builtin/gc.c: static int report_last_gc_error(void)\n      \t\tif (errno == ENOENT)\n      \t\t\tgoto done;\n      \n    @@ builtin/gc.c: static int report_last_gc_error(void)\n      \telse if (len > 0) {\n      \t\t/*\n      \t\t * A previous gc failed.  Report the error, and don't\n    -@@ builtin/gc.c: static int report_last_gc_error(void)\n    - \t\t\t       \"until the file is removed.\\n\\n\"\n    - \t\t\t       \"%s\"),\n    - \t\t\t    gc_log_path, sb.buf);\n    --\t\tret = 1;\n    -+\t\t*skip = 1;\n    - \t}\n    - \tstrbuf_release(&sb);\n    - done:\n     @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)\n    - \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n    - \t\t}\n      \t\tif (detach_auto) {\n    --\t\t\tint ret = report_last_gc_error();\n    + \t\t\tint ret = report_last_gc_error();\n    + \n     -\t\t\tif (ret < 0)\n     -\t\t\t\t/* an I/O error occurred, already reported */\n    --\t\t\t\texit(128);\n    --\t\t\tif (ret == 1)\n    -+\t\t\tint skip;\n    -+\t\t\tint ret = report_last_gc_error(&skip);\n    -+\n    -+\t\t\tif (skip)\n    +-\t\t\t\treturn 128;\n    + \t\t\tif (ret == 1)\n      \t\t\t\t/* Last gc --auto failed. Skip this one. */\n      \t\t\t\treturn 0;\n    -+\t\t\tif (ret)\n    ++\t\t\telse if (ret)\n     +\t\t\t\t/* an I/O error occurred, already reported */\n     +\t\t\t\treturn ret;\n      \n4:  e0e6427cbd3 ! 6:  42132406731 config API: don't use vreportf(), make it static in usage.c\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    config API: don't use vreportf(), make it static in usage.c\n    +    config API: use get_error_routine(), not vreportf()\n     \n    -    In preceding commits the rest of the vreportf() users outside of\n    -    usage.c have been migrated to die_message(), leaving only the\n    -    git_die_config() function added in 5a80e97c827 (config: add\n    -    `git_die_config()` to the config-set API, 2014-08-07).\n    -\n    -    Let's have its callers call error() themselves if they want to emit a\n    -    message, which is exactly what git_die_config() was doing for them\n    -    before emitting its own die() message.\n    +    Change the git_die_config() function added in 5a80e97c827 (config: add\n    +    `git_die_config()` to the config-set API, 2014-08-07) to use the\n    +    public callbacks in the usage.[ch] API instead of the the underlying\n    +    vreportf() function.\n     \n    -    This means that we can make the vreportf() in usage.c \"static\", and\n    -    only expose functions such as usage(), die(), warning() etc.\n    +    In preceding commits the rest of the vreportf() users outside of\n    +    usage.c was migrated to die_message(), so we can now make it \"static\".\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    - ## builtin/fast-import.c ##\n    -@@ builtin/fast-import.c: static void git_pack_config(void)\n    - \t}\n    - \tif (!git_config_get_int(\"pack.indexversion\", &indexversion_value)) {\n    - \t\tpack_idx_opts.version = indexversion_value;\n    --\t\tif (pack_idx_opts.version > 2)\n    --\t\t\tgit_die_config(\"pack.indexversion\",\n    --\t\t\t\t\t\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n    -+\t\tif (pack_idx_opts.version > 2) {\n    -+\t\t\terror(\"bad pack.indexversion=%\"PRIu32, pack_idx_opts.version);\n    -+\t\t\tgit_die_config(\"pack.indexversion\");\n    -+\t\t}\n    - \t}\n    - \tif (!git_config_get_ulong(\"pack.packsizelimit\", &packsizelimit_value))\n    - \t\tmax_packsize = packsizelimit_value;\n    -\n    - ## builtin/notes.c ##\n    -@@ builtin/notes.c: static int git_config_get_notes_strategy(const char *key,\n    - \n    - \tif (git_config_get_string(key, &value))\n    - \t\treturn 1;\n    --\tif (parse_notes_merge_strategy(value, strategy))\n    --\t\tgit_die_config(key, _(\"unknown notes merge strategy %s\"), value);\n    -+\tif (parse_notes_merge_strategy(value, strategy)) {\n    -+\t\terror(_(\"unknown notes merge strategy %s\"), value);\n    -+\t\tgit_die_config(key);\n    -+\t}\n    - \n    - \tfree(value);\n    - \treturn 0;\n    -\n      ## config.c ##\n    -@@ config.c: int repo_config_get_string(struct repository *repo,\n    - \tgit_config_check_init(repo);\n    - \tret = git_configset_get_string(repo->config, key, dest);\n    - \tif (ret < 0)\n    --\t\tgit_die_config(key, NULL);\n    -+\t\tgit_die_config(key);\n    - \treturn ret;\n    - }\n    - \n    -@@ config.c: int repo_config_get_string_tmp(struct repository *repo,\n    - \tgit_config_check_init(repo);\n    - \tret = git_configset_get_string_tmp(repo->config, key, dest);\n    - \tif (ret < 0)\n    --\t\tgit_die_config(key, NULL);\n    -+\t\tgit_die_config(key);\n    - \treturn ret;\n    - }\n    - \n    -@@ config.c: int repo_config_get_pathname(struct repository *repo,\n    - \tgit_config_check_init(repo);\n    - \tret = git_configset_get_pathname(repo->config, key, dest);\n    - \tif (ret < 0)\n    --\t\tgit_die_config(key, NULL);\n    -+\t\tgit_die_config(key);\n    - \treturn ret;\n    - }\n    - \n    -@@ config.c: int git_config_get_expiry(const char *key, const char **output)\n    - \t\treturn ret;\n    - \tif (strcmp(*output, \"now\")) {\n    - \t\ttimestamp_t now = approxidate(\"now\");\n    --\t\tif (approxidate(*output) >= now)\n    --\t\t\tgit_die_config(key, _(\"Invalid %s: '%s'\"), key, *output);\n    -+\t\tif (approxidate(*output) >= now) {\n    -+\t\t\terror(_(\"Invalid %s: '%s'\"), key, *output);\n    -+\t\t\tgit_die_config(key);\n    -+\t\t}\n    - \t}\n    - \treturn ret;\n    - }\n    -@@ config.c: void git_die_config_linenr(const char *key, const char *filename, int linenr)\n    - \t\t    key, filename, linenr);\n    - }\n    - \n    --NORETURN __attribute__((format(printf, 2, 3)))\n    --void git_die_config(const char *key, const char *err, ...)\n    -+NORETURN\n    -+void git_die_config(const char *key)\n    +@@ config.c: void git_die_config(const char *key, const char *err, ...)\n      {\n      \tconst struct string_list *values;\n      \tstruct key_value_info *kv_info;\n    ++\treport_fn error_fn = get_error_routine();\n      \n    --\tif (err) {\n    --\t\tva_list params;\n    --\t\tva_start(params, err);\n    + \tif (err) {\n    + \t\tva_list params;\n    + \t\tva_start(params, err);\n     -\t\tvreportf(\"error: \", err, params);\n    --\t\tva_end(params);\n    --\t}\n    ++\t\terror_fn(err, params);\n    + \t\tva_end(params);\n    + \t}\n      \tvalues = git_config_get_value_multi(key);\n    - \tkv_info = values->items[values->nr - 1].util;\n    - \tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n    -\n    - ## config.h ##\n    -@@ config.h: struct key_value_info {\n    - };\n    - \n    - /**\n    -- * First prints the error message specified by the caller in `err` and then\n    -- * dies printing the line number and the file name of the highest priority\n    -- * value for the configuration variable `key`.\n    -+ * Dies printing the line number and the file name of the highest\n    -+ * priority value for the configuration variable `key`.\n    -+ *\n    -+ * Consider calling error() first with a more specific formatted\n    -+ * message of your own.\n    -  */\n    --NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));\n    -+NORETURN void git_die_config(const char *key);\n    - \n    - /**\n    -  * Helper function which formats the die error message according to the\n     \n      ## git-compat-util.h ##\n     @@ git-compat-util.h: static inline int git_has_dir_sep(const char *path)\n    @@ git-compat-util.h: static inline int git_has_dir_sep(const char *path)\n      NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n      NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n     \n    - ## imap-send.c ##\n    -@@ imap-send.c: static int git_imap_config(const char *var, const char *val, void *cb)\n    - \t\tserver.port = git_config_int(var, val);\n    - \telse if (!strcmp(\"imap.host\", var)) {\n    - \t\tif (!val) {\n    --\t\t\tgit_die_config(\"imap.host\", \"Missing value for 'imap.host'\");\n    -+\t\t\terror(\"Missing value for 'imap.host'\");\n    -+\t\t\tgit_die_config(\"imap.host\");\n    - \t\t} else {\n    - \t\t\tif (starts_with(val, \"imap:\"))\n    - \t\t\t\tval += 5;\n    -\n      ## usage.c ##\n     @@\n      #include \"git-compat-util.h\"\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443330","messageId":"patch-v2-2.6-f5a98901498-20211207T182419Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"[PATCH v2 2/6] usage.c API users: use die_message() for \"fatal :\" + exit 128","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:26:30Z","receivedAt":"2021-12-07T18:26:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code that printed its own \"fatal: \" message and exited with a\nstatus code of 128 to use the die_message() function added in a\npreceding commit.\n\nThis change also demonstrates why the return value of\ndie_message_routine() needed to be that of \"report_fn\". We have\ncallers such as the run-command.c::child_err_spew() which would like\nto replace its error routine with the return value of\n\"get_die_message_routine()\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/fast-import.c | 12 +++++++-----\n http-backend.c        |  3 ++-\n parse-options.c       |  2 +-\n run-command.c         | 16 +++++-----------\n 4 files changed, 15 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 20406f67754..2b2e28bad79 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -401,16 +401,18 @@ static void dump_marks(void);\n \n static NORETURN void die_nicely(const char *err, va_list params)\n {\n+\tva_list cp;\n \tstatic int zombie;\n-\tchar message[2 * PATH_MAX];\n+\treport_fn die_message_fn = get_die_message_routine();\n \n-\tvsnprintf(message, sizeof(message), err, params);\n-\tfputs(\"fatal: \", stderr);\n-\tfputs(message, stderr);\n-\tfputc('\\n', stderr);\n+\tva_copy(cp, params);\n+\tdie_message_fn(err, params);\n \n \tif (!zombie) {\n+\t\tchar message[2 * PATH_MAX];\n+\n \t\tzombie = 1;\n+\t\tvsnprintf(message, sizeof(message), err, cp);\n \t\twrite_crash_report(message);\n \t\tend_packfile();\n \t\tunkeep_all_packs();\ndiff --git a/http-backend.c b/http-backend.c\nindex 3d6e2ff17f8..982cb62c7cb 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -659,8 +659,9 @@ static NORETURN void die_webcgi(const char *err, va_list params)\n {\n \tif (dead <= 1) {\n \t\tstruct strbuf hdr = STRBUF_INIT;\n+\t\treport_fn die_message_fn = get_die_message_routine();\n \n-\t\tvreportf(\"fatal: \", err, params);\n+\t\tdie_message_fn(err, params);\n \n \t\thttp_status(&hdr, 500, \"Internal Server Error\");\n \t\thdr_nocache(&hdr);\ndiff --git a/parse-options.c b/parse-options.c\nindex fc5b43ff0b2..8bc0a21f1d7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1075,6 +1075,6 @@ void NORETURN usage_msg_opt(const char *msg,\n \t\t   const char * const *usagestr,\n \t\t   const struct option *options)\n {\n-\tfprintf(stderr, \"fatal: %s\\n\\n\", msg);\n+\tdie_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n \tusage_with_options(usagestr, options);\n }\ndiff --git a/run-command.c b/run-command.c\nindex f40df01c772..a790fe9799d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])\n \tchild_close(fd[1]);\n }\n \n-/*\n- * parent will make it look like the child spewed a fatal error and died\n- * this is needed to prevent changes to t0061.\n- */\n-static void fake_fatal(const char *err, va_list params)\n-{\n-\tvreportf(\"fatal: \", err, params);\n-}\n-\n static void child_error_fn(const char *err, va_list params)\n {\n \tconst char msg[] = \"error() should not be called in child\\n\";\n@@ -372,9 +363,10 @@ static void NORETURN child_die_fn(const char *err, va_list params)\n static void child_err_spew(struct child_process *cmd, struct child_err *cerr)\n {\n \tstatic void (*old_errfn)(const char *err, va_list params);\n+\treport_fn die_message_routine = get_die_message_routine();\n \n \told_errfn = get_error_routine();\n-\tset_error_routine(fake_fatal);\n+\tset_error_routine(die_message_routine);\n \terrno = cerr->syserr;\n \n \tswitch (cerr->err) {\n@@ -1082,7 +1074,9 @@ static void *run_thread(void *data)\n \n static NORETURN void die_async(const char *err, va_list params)\n {\n-\tvreportf(\"fatal: \", err, params);\n+\treport_fn die_message_fn = get_die_message_routine();\n+\n+\tdie_message_fn(err, params);\n \n \tif (in_async()) {\n \t\tstruct async *async = pthread_getspecific(async_key);\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443331","messageId":"patch-v2-3.6-c7d67fd41fa-20211207T182419Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"[PATCH v2 3/6] usage.c API users: use die_message() for error() + exit 128","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:26:31Z","receivedAt":"2021-12-07T18:26:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue the migration of code that printed a message and exited with\n128. In this case the caller used \"error()\", so we'll be changing the\noutput from \"error: \" to \"fatal: \". This change is intentional and\ndesired.\n\nThis code is dying, so it should emit \"fatal\", the only reason it\ndidn't do so was because before the existence of \"die_message()\" it\nwould have needed to craft its own \"fatal: \" message.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/notes.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 71c59583a17..2812d1eac40 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n static void write_note_data(struct note_data *d, struct object_id *oid)\n {\n \tif (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {\n-\t\terror(_(\"unable to write note object\"));\n+\t\tint status = die_message(_(\"unable to write note object\"));\n+\n \t\tif (d->edit_path)\n-\t\t\terror(_(\"the note contents have been left in %s\"),\n-\t\t\t\td->edit_path);\n-\t\texit(128);\n+\t\t\tdie_message(_(\"the note contents have been left in %s\"),\n+\t\t\t\t    d->edit_path);\n+\t\texit(status);\n \t}\n }\n \n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443332","messageId":"patch-v2-4.6-f224a281a10-20211207T182419Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"[PATCH v2 4/6] gc: return from cmd_gc(), don't call exit()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:26:32Z","receivedAt":"2021-12-07T18:26:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A minor code cleanup. Let's \"return\" from cmd_gc() instead of calling\nexit(). See 338abb0f045 (builtins + test helpers: use return instead\nof exit() in cmd_*, 2021-06-08) for other such cases.\n\nWhile we're at it add a \\n to separate the variable declaration from\nthe rest of the code in this block. Both of these changes make a\nsubsequent change smaller and easier to read.\n\nThis change isn't really needed for that subsequent change, but now\nsomeone viewing that future behavior change won't need to wonder why\nwe're either still calling exit() here, or fixing it while we're at\nit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/gc.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bcef6a4c8da..900ccfb8d48 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -611,9 +611,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (detach_auto) {\n \t\t\tint ret = report_last_gc_error();\n+\n \t\t\tif (ret < 0)\n \t\t\t\t/* an I/O error occurred, already reported */\n-\t\t\t\texit(128);\n+\t\t\t\treturn 128;\n \t\t\tif (ret == 1)\n \t\t\t\t/* Last gc --auto failed. Skip this one. */\n \t\t\t\treturn 0;\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443333","messageId":"patch-v2-5.6-2b4a3910654-20211207T182419Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"[PATCH v2 5/6] usage.c + gc: add and use a die_message_errno()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:26:33Z","receivedAt":"2021-12-07T18:26:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"error: \" output when we exit with 128 due to gc.log errors\nto use a \"fatal: \" prefix instead. To do this add a\ndie_message_errno() a sibling function to the die_errno() added in a\npreceding commit.\n\nBefore this we'd expect report_last_gc_error() to return -1 from\nerror_errno() in this case. It already treated a status of 0 and 1\nspecially. Let's just document that anything that's not 0 or 1 should\nbe returned.\n\nWe could also retain the \"ret < 0\" behavior here without hardcoding\n128 by returning -128, and having the caller do a \"return -ret\", but I\nthink this makes more sense, and preserves the path from\ndie_message*()'s return value to the \"return\" without hardcoding\n\"128\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/gc.c      | 13 +++++++------\n git-compat-util.h |  1 +\n usage.c           | 12 ++++++++++++\n 3 files changed, 20 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 900ccfb8d48..8e60ef1eaba 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -470,7 +470,8 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n /*\n  * Returns 0 if there was no previous error and gc can proceed, 1 if\n  * gc should not proceed due to an error in the last run. Prints a\n- * message and returns -1 if an error occurred while reading gc.log\n+ * message and returns with a non-[01] status code if an error occurred\n+ * while reading gc.log\n  */\n static int report_last_gc_error(void)\n {\n@@ -484,7 +485,7 @@ static int report_last_gc_error(void)\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n+\t\tret = die_message_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t\tgoto done;\n \t}\n \n@@ -493,7 +494,7 @@ static int report_last_gc_error(void)\n \n \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (len < 0)\n-\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\t\tret = die_message_errno(_(\"cannot read '%s'\"), gc_log_path);\n \telse if (len > 0) {\n \t\t/*\n \t\t * A previous gc failed.  Report the error, and don't\n@@ -612,12 +613,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tif (detach_auto) {\n \t\t\tint ret = report_last_gc_error();\n \n-\t\t\tif (ret < 0)\n-\t\t\t\t/* an I/O error occurred, already reported */\n-\t\t\t\treturn 128;\n \t\t\tif (ret == 1)\n \t\t\t\t/* Last gc --auto failed. Skip this one. */\n \t\t\t\treturn 0;\n+\t\t\telse if (ret)\n+\t\t\t\t/* an I/O error occurred, already reported */\n+\t\t\t\treturn ret;\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d5e495529c8..c6c6f7d6b51 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -480,6 +480,7 @@ NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/usage.c b/usage.c\nindex 76399ba8409..3d09e8eea48 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -233,6 +233,18 @@ int die_message(const char *err, ...)\n \treturn 128;\n }\n \n+#undef die_message_errno\n+int die_message_errno(const char *fmt, ...)\n+{\n+\tchar buf[1024];\n+\tva_list params;\n+\n+\tva_start(params, fmt);\n+\tdie_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);\n+\tva_end(params);\n+\treturn 128;\n+}\n+\n #undef error_errno\n int error_errno(const char *fmt, ...)\n {\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443334","messageId":"patch-v2-6.6-42132406731-20211207T182419Z-avarab@gmail.com","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"[PATCH v2 6/6] config API: use get_error_routine(), not vreportf()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T18:26:34Z","receivedAt":"2021-12-07T18:26:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the git_die_config() function added in 5a80e97c827 (config: add\n`git_die_config()` to the config-set API, 2014-08-07) to use the\npublic callbacks in the usage.[ch] API instead of the the underlying\nvreportf() function.\n\nIn preceding commits the rest of the vreportf() users outside of\nusage.c was migrated to die_message(), so we can now make it \"static\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n config.c          | 3 ++-\n git-compat-util.h | 1 -\n usage.c           | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c5873f3a706..e96b8fdb62d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2555,11 +2555,12 @@ void git_die_config(const char *key, const char *err, ...)\n {\n \tconst struct string_list *values;\n \tstruct key_value_info *kv_info;\n+\treport_fn error_fn = get_error_routine();\n \n \tif (err) {\n \t\tva_list params;\n \t\tva_start(params, err);\n-\t\tvreportf(\"error: \", err, params);\n+\t\terror_fn(err, params);\n \t\tva_end(params);\n \t}\n \tvalues = git_config_get_value_multi(key);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c6c6f7d6b51..bdb3977b9ec 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -474,7 +474,6 @@ static inline int git_has_dir_sep(const char *path)\n struct strbuf;\n \n /* General helper functions */\n-void vreportf(const char *prefix, const char *err, va_list params);\n NORETURN void usage(const char *err);\n NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/usage.c b/usage.c\nindex 3d09e8eea48..9943dd8742e 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,7 +6,7 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n-void vreportf(const char *prefix, const char *err, va_list params)\n+static void vreportf(const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443372","messageId":"xmqqpmq85cre.fsf@gitster.g","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/6] usage API: Add and use die_message()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-07T21:24:53Z","receivedAt":"2021-12-07T21:25:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> A small set of fixes to usage.[ch]'s API use that will go towards\n> enabling nicer things down the road. See [1] for the v1 summary\n>\n> I believe this should address all the feedback Junio had on the\n> v1.\n\nThanks.  We need more reviewers, I guess, but in the meantime, you\nneed to make do with me ;-)\n\n"},{"id":"444812","messageId":"20211222185715.1090524-1-jonathantanmy@google.com","threadId":"57040","inReplyTo":"cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/6] usage API: Add and use die_message()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-12-22T18:57:14Z","receivedAt":"2021-12-22T18:57:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> A small set of fixes to usage.[ch]'s API use that will go towards\n> enabling nicer things down the road. See [1] for the v1 summary\n> \n> I believe this should address all the feedback Junio had on the\n> v1.\n> \n> Aside from the substantially rewritten 6/6 and much simplified 4/6 the\n> end-state is almost the same, but things are better split up,\n> explained etc. now.\n> \n> 1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/\n\nI haven't looked at this round of patches yet, but for the convenience\nof reviewers, it would have been great if you linked to a prior\ndiscussion [1], including an email from me with comments that (as far as\nI know) haven't been addressed [2].\n\n[1] https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/\n[2] https://lore.kernel.org/git/20211027215053.2257548-1-jonathantanmy@google.com/\n"},{"id":"444820","messageId":"xmqq4k70jtr4.fsf@gitster.g","threadId":"57040","inReplyTo":"20211222185715.1090524-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/6] usage API: Add and use die_message()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-22T19:59:27Z","receivedAt":"2021-12-22T19:59:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> A small set of fixes to usage.[ch]'s API use that will go towards\n>> enabling nicer things down the road. See [1] for the v1 summary\n>> \n>> I believe this should address all the feedback Junio had on the\n>> v1.\n>> \n>> Aside from the substantially rewritten 6/6 and much simplified 4/6 the\n>> end-state is almost the same, but things are better split up,\n>> explained etc. now.\n>> \n>> 1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/\n>\n> I haven't looked at this round of patches yet, but for the convenience\n> of reviewers, it would have been great if you linked to a prior\n> discussion [1], including an email from me with comments that (as far as\n> I know) haven't been addressed [2].\n>\n> [1] https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/\n> [2] https://lore.kernel.org/git/20211027215053.2257548-1-jonathantanmy@google.com/\n\nHear, hear, and thanks.\n"},{"id":"444907","messageId":"211224.86r1a2gcg0.gmgdl@evledraar.gmail.com","threadId":"57040","inReplyTo":"20211222185715.1090524-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/6] usage API: Add and use die_message()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-24T17:01:01Z","receivedAt":"2021-12-24T17:06:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 22 2021, Jonathan Tan wrote:\n\n>> A small set of fixes to usage.[ch]'s API use that will go towards\n>> enabling nicer things down the road. See [1] for the v1 summary\n>> \n>> I believe this should address all the feedback Junio had on the\n>> v1.\n>> \n>> Aside from the substantially rewritten 6/6 and much simplified 4/6 the\n>> end-state is almost the same, but things are better split up,\n>> explained etc. now.\n>> \n>> 1. https://lore.kernel.org/git/cover-0.4-00000000000-20211206T165221Z-avarab@gmail.com/\n>\n> I haven't looked at this round of patches yet, but for the convenience\n> of reviewers, it would have been great if you linked to a prior\n> discussion [1],\n\nFWIW this is (indirectly) linked-to upthread. I linked to this RFC\nseries which this is peeled from:\nhttps://lore.kernel.org/git/RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com/\n\nWhich in turn links to the earlier series you commented on that\ndie_message() is peeled off from:\nhttps://lore.kernel.org/git/cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com/\n\n> including an email from me with comments that (as far as\n> I know) haven't been addressed [2].\n\nIs there anything to address in the context of this current series?\nI.e. you're commenting on leak detection, which isn't at all the goal of\nthis series.\n\nI.e. I had these die_message() changes that I worked into that series,\nbut there didn't seem to be any interest in code changes that \"solved\"\nissues of SANITIZE=leak interaction with various non-CI compiler\nversions/flags, so I didn't think it was worth pursuing.\n\nBut as this series shows die_message() is also something we can use in\nother contexts.\n\n> [1] https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/\n> [2] https://lore.kernel.org/git/20211027215053.2257548-1-jonathantanmy@google.com/\n\n"}]}