| Message ID | 20241127233312.27710-4-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | propagate fsck message severity for bundle fetch | expand |
Justin Tobler <jltobler@gmail.com> writes: > For `fetch_pack_fsck_config()` to discern between errors and unhandled > config variables, the return code when `git_config_path()` errors is > changed to a different value also indicating success. This frees up the > previous return code to now indicate the provided config variable > was unhandled. The behavior remains functionally the same. > @@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value, > char *path ; > > if (git_config_pathname(&path, var, value)) > - return 1; > + return 0; So, after getting complaint about a misconfiguration, the caller used to be able to, if they wanted to, tell two cases apart: a misconfigured variable was ignored here, and we happily parsed the configuration. Now they no longer can tell them apart, because we return 0 for both cases. > - strbuf_addf(&fsck_msg_types, "%cskiplist=%s", > - fsck_msg_types.len ? ',' : '=', path); > + strbuf_addf(msg_types, "%cskiplist=%s", > + msg_types->len ? ',' : '=', path); > free(path); > return 0; > } > @@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value, > if (!value) > return config_error_nonbool(var); > if (is_valid_msg_type(msg_id, value)) > - strbuf_addf(&fsck_msg_types, "%c%s=%s", > - fsck_msg_types.len ? ',' : '=', msg_id, value); > + strbuf_addf(msg_types, "%c%s=%s", > + msg_types->len ? ',' : '=', msg_id, value); > else > warning("Skipping unknown msg id '%s'", msg_id); > return 0; > } > > - return git_default_config(var, value, ctx, cb); > + return 1; > +} And a 1 is returned to signal a "we didn't see what we care about". > +static int fetch_pack_config_cb(const char *var, const char *value, > + const struct config_context *ctx, void *cb) > +{ > + int ret = fetch_pack_fsck_config(var, value, &fsck_msg_types); > + if (ret > 0) > + return git_default_config(var, value, ctx, cb); > + > + return ret; > } And we catch that 1 and ask git_default_config() to use what we didn't use. OK. If I were doing this patch, I would probably have chosen not to change the value used to signal "a misconfiguration but that is not too serious so we'll ignore after warning", picked another and new value, like 2, to signal "the key is not something we care about", which would mean fetch_pack_config_cb() would call "default" only when it sees 2. But with the current callers, and with the callers after this series, it is not necessary. If we need to accomodate other new callers later, we can make such an update then. This step, together with other patches, are nicely done. Will queue. Thanks.
diff --git a/fetch-pack.c b/fetch-pack.c index fe1fb3c1b7..c095f3a84b 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1857,8 +1857,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, return ref; } -static int fetch_pack_config_cb(const char *var, const char *value, - const struct config_context *ctx, void *cb) +int fetch_pack_fsck_config(const char *var, const char *value, + struct strbuf *msg_types) { const char *msg_id; @@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value, char *path ; if (git_config_pathname(&path, var, value)) - return 1; - strbuf_addf(&fsck_msg_types, "%cskiplist=%s", - fsck_msg_types.len ? ',' : '=', path); + return 0; + strbuf_addf(msg_types, "%cskiplist=%s", + msg_types->len ? ',' : '=', path); free(path); return 0; } @@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value, if (!value) return config_error_nonbool(var); if (is_valid_msg_type(msg_id, value)) - strbuf_addf(&fsck_msg_types, "%c%s=%s", - fsck_msg_types.len ? ',' : '=', msg_id, value); + strbuf_addf(msg_types, "%c%s=%s", + msg_types->len ? ',' : '=', msg_id, value); else warning("Skipping unknown msg id '%s'", msg_id); return 0; } - return git_default_config(var, value, ctx, cb); + return 1; +} + +static int fetch_pack_config_cb(const char *var, const char *value, + const struct config_context *ctx, void *cb) +{ + int ret = fetch_pack_fsck_config(var, value, &fsck_msg_types); + if (ret > 0) + return git_default_config(var, value, ctx, cb); + + return ret; } static void fetch_pack_config(void) diff --git a/fetch-pack.h b/fetch-pack.h index b5c579cdae..9d3470366f 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -106,4 +106,15 @@ int report_unmatched_refs(struct ref **sought, int nr_sought); */ int fetch_pack_fsck_objects(void); +/* + * Check if the provided config variable pertains to fetch fsck and if so append + * the configuration to the provided strbuf. + * + * When a fetch fsck config option is successfully processed the function + * returns 0. If the provided config option is unrelated to fetch fsck, 1 is + * returned. Errors return -1. + */ +int fetch_pack_fsck_config(const char *var, const char *value, + struct strbuf *msg_types); + #endif
When `fetch_pack_config()` is invoked, fetch-pack configuration is parsed from the config. As part of this operation, fsck message severity configuration is assigned to the `fsck_msg_types` global variable. This is optionally used to configure the downstream git-index-pack(1) when the `--strict` option is specified. The same parsed fsck message severity configuration is also needed outside of fetch-pack. Instead of exposing/relying on the existing global state, split out the fsck config parsing logic into `fetch_pack_fsck_config()` and expose it. In a subsequent commit, this is used to provide fsck configuration when invoking `unbundle()`. For `fetch_pack_fsck_config()` to discern between errors and unhandled config variables, the return code when `git_config_path()` errors is changed to a different value also indicating success. This frees up the previous return code to now indicate the provided config variable was unhandled. The behavior remains functionally the same. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- fetch-pack.c | 26 ++++++++++++++++++-------- fetch-pack.h | 11 +++++++++++ 2 files changed, 29 insertions(+), 8 deletions(-)