| Message ID | 20241127005707.319881-4-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | propagate fsck message severity for bundle fetch | expand |
On Tue, Nov 26, 2024 at 06:57:06PM -0600, Justin Tobler wrote: > diff --git a/fetch-pack.c b/fetch-pack.c > index fe1fb3c1b7..e7d4f6e6e2 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -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) Okay, this now reads a lot nicer. But I'm sceptical whether we should return -1 for the case where the value wasn't handled because we now cannot discern the case where the function returns an error from the case where it simply didn't handle the value. In fact, we cannot even use positive values right now as far as I can see: - We return 0 on success. - We return 1 in case `git_config_pathname()` indicates an error. - We return -1 when there is no value with "fetch.fsck.". I'd propose to have a look at whether the positive return value from the second case is actually used anywhere. If not, we can refactor this case so that we always return negative on errors. And then we could further adapt the function to return positive in case it didn't handle the value. > diff --git a/fetch-pack.h b/fetch-pack.h > index b5c579cdae..c667b6fbf3 100644 > --- a/fetch-pack.h > +++ b/fetch-pack.h > @@ -106,4 +106,7 @@ int report_unmatched_refs(struct ref **sought, int nr_sought); > */ > int fetch_pack_fsck_objects(void); > > +int fetch_pack_fsck_config(const char *var, const char *value, > + struct strbuf *msg_types); We should also add some docs here, at the least to document the error codes. Patrick
On 24/11/27 06:44AM, Patrick Steinhardt wrote: > On Tue, Nov 26, 2024 at 06:57:06PM -0600, Justin Tobler wrote: > > diff --git a/fetch-pack.c b/fetch-pack.c > > index fe1fb3c1b7..e7d4f6e6e2 100644 > > --- a/fetch-pack.c > > +++ b/fetch-pack.c > > @@ -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) > > Okay, this now reads a lot nicer. But I'm sceptical whether we should > return -1 for the case where the value wasn't handled because we now > cannot discern the case where the function returns an error from the > case where it simply didn't handle the value. > > In fact, we cannot even use positive values right now as far as I can > see: > > - We return 0 on success. > > - We return 1 in case `git_config_pathname()` indicates an error. > > - We return -1 when there is no value with "fetch.fsck.". > > I'd propose to have a look at whether the positive return value from the > second case is actually used anywhere. If not, we can refactor this case > so that we always return negative on errors. And then we could further > adapt the function to return positive in case it didn't handle the > value. The `fetch_pack_fsck_config()` function is only used through callback functions for `git_config()`. The `config_fn_t` callback is expected to return 0 for success, or -1 if the variable could not be parsed properly. Taking a look at `configset_iter()`, in practice any return value >= 0 is treated as success. This means that because the `git_config_pathname()` errors in `fetch_pack_fsck_config()` return 1, an error is printed, but the operation is able to proceed. # Skip list is missing path value. $ git -c fetch.fsck.skipList fetch error: missing value for 'fetch.fsck.skiplist' <fetch continues...> This skip list flag behavior is consistent across other fsck operations. Changing it here to return -1 would make config parsing more strict and result in a missing skip list path causing the whole operation to die. Since returning 1 is treated as a success anyway, we could change it to return 0. This would result in functionally the same behavior and free up `fetch_pack_fsck_config()` to return 1 to indicate that there is no value with "fetch.fsck.". This would allow us to properly differentiate between the "didn't handle the value" and error cases. I'll send another version making this change and add some comments to document the behavior. -Justin
diff --git a/fetch-pack.c b/fetch-pack.c index fe1fb3c1b7..e7d4f6e6e2 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; @@ -1867,8 +1867,8 @@ static int fetch_pack_config_cb(const char *var, const char *value, if (git_config_pathname(&path, var, value)) return 1; - 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; +} + +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..c667b6fbf3 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -106,4 +106,7 @@ int report_unmatched_refs(struct ref **sought, int nr_sought); */ int fetch_pack_fsck_objects(void); +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()`. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- fetch-pack.c | 24 +++++++++++++++++------- fetch-pack.h | 3 +++ 2 files changed, 20 insertions(+), 7 deletions(-)