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