| Message ID | 20241121204119.1440773-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: > 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. > > In a subsequent commit, the same fetch-pack fsck message configuration > needs to be reused. To facilitate this, introduce `fetch_pack_options` > which gets written to during the `fetch_pack_config_cb()` instead of > directly modifying the `fsck_msg_types` global. It is unclear how it facilitates to replace one global with another global that has the data that was previously global as one of its members. With the above I was somehow expecting that the option struct instance is allocated on the stack of a function common to both callers of the configuration reader (i.e. fetch_pack_config()) as well as the configuration user (i.e. get_pack()). If we were to allow the latter to keep accessing the global (which is perfectly fine), wouldn't it be sufficient for the purpose of this series (which I am imagining wants to call fetch_pack_config() from the sideways and grab what came from the configuration) to just pass the fsck_msg_types strbuf through the call chain of the reaading side? That is, - fetch_pack_config()'s current callers pass the address of fsck_msg_types to a new parameter. - fetch_pack_config() passes that new parameter when calling git_config(); - fetch_pack_config_cb() uses the cb parameter and stuff its findings there; - a third-party caller calls fetch_pack_config() with its own fsck_msg_types instance (presumably in this series, it would be the opts.fsck_msg_types member introduced earlier in the bundle code). or something like that? So, the reason for existence of the shell around the fsck_msg_types needs to be explained. It is perfectly fine to say "we'll add THIS THING in a later step", if that were the case, but a reviewer tends to start reading from the front, so the presentation order matters. Leaving many questions unsolved tangling may be a good way to keep readers engaged when writing a mystery novel, but not a patch series. Having to keep too many things in head, especially when many of them are not explained well (hence raises "why should I keep these in my head?" question), is another distraction and discourages the reviewers from reading further on. Assuming that the shell structure is necessary around it, the code changes in this patch looks sensible to me. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Assuming that the shell structure is necessary around it, the code > changes in this patch looks sensible to me. Ah, another thing. It would make more sense to do good_example(args, struct foo *opt) { struct foo opt_fallback = { ... init ... }; if (!opt) opt = &opt_fallback; ... use opt->foo and opt->bar } instead of what the patch did with structure assignment, i.e. bad_example(args, struct foo *opt_) { struct foo opt = { ... init ... }; if (opt_) opt = *opt_; ... use opt.foo and opt.bar } because the latter, via structure assignment, always raises "who owns this piece of data?" question once you start adding more complex things in the "struct foo", like a strbuf that holds the fsck configuration data.
On 24/11/22 10:46AM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > 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. > > > > In a subsequent commit, the same fetch-pack fsck message configuration > > needs to be reused. To facilitate this, introduce `fetch_pack_options` > > which gets written to during the `fetch_pack_config_cb()` instead of > > directly modifying the `fsck_msg_types` global. > > It is unclear how it facilitates to replace one global with another > global that has the data that was previously global as one of its > members. With the above I was somehow expecting that the option > struct instance is allocated on the stack of a function common to > both callers of the configuration reader (i.e. fetch_pack_config()) > as well as the configuration user (i.e. get_pack()). If we were to > allow the latter to keep accessing the global (which is perfectly > fine), wouldn't it be sufficient for the purpose of this series > (which I am imagining wants to call fetch_pack_config() from the > sideways and grab what came from the configuration) to just pass the > fsck_msg_types strbuf through the call chain of the reaading side? For the purposes of this series, the addition of the `fetch_pack_options` structure as a wrapper around `fsck_msg_types` is not needed. As mentioned, it would be sufficient to just pass the `strbuf` directly and have it modified by `fetch_pack_config_cb()`. The original intent of providing the shell structure was to allow for more easy extension of the fetch-pack config parsing in the future, but it doesn't probably make much sense to do it now and its purpose wasn't explained. In the next version I'll drop the use of shell structure in favor of passing an instance of `strbuf` directly. -Justin
diff --git a/fetch-pack.c b/fetch-pack.c index fe1fb3c1b7..73309f8043 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -48,8 +48,8 @@ static int server_supports_filtering; static int advertise_sid; static struct shallow_lock shallow_lock; static const char *alternate_shallow_file; +static struct fetch_pack_options fetch_pack_options = FETCH_PACK_OPTIONS_INIT; static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES; -static struct strbuf fsck_msg_types = STRBUF_INIT; static struct string_list uri_protocols = STRING_LIST_INIT_DUP; /* Remember to update object flag allocation in object.h */ @@ -1017,7 +1017,7 @@ static int get_pack(struct fetch_pack_args *args, strvec_push(&cmd.args, "--fsck-objects"); else strvec_pushf(&cmd.args, "--strict%s", - fsck_msg_types.buf); + fetch_pack_options.fsck_msg_types.buf); } if (index_pack_args) { @@ -1860,6 +1860,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, static int fetch_pack_config_cb(const char *var, const char *value, const struct config_context *ctx, void *cb) { + struct fetch_pack_options *opts = cb; const char *msg_id; if (strcmp(var, "fetch.fsck.skiplist") == 0) { @@ -1867,8 +1868,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(&opts->fsck_msg_types, "%cskiplist=%s", + opts->fsck_msg_types.len ? ',' : '=', path); free(path); return 0; } @@ -1877,8 +1878,9 @@ 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(&opts->fsck_msg_types, "%c%s=%s", + opts->fsck_msg_types.len ? ',' : '=', + msg_id, value); else warning("Skipping unknown msg id '%s'", msg_id); return 0; @@ -1904,7 +1906,7 @@ static void fetch_pack_config(void) } } - git_config(fetch_pack_config_cb, NULL); + git_config(fetch_pack_config_cb, &fetch_pack_options); } static void fetch_pack_setup(void) diff --git a/fetch-pack.h b/fetch-pack.h index b5c579cdae..8243b754ce 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -106,4 +106,12 @@ int report_unmatched_refs(struct ref **sought, int nr_sought); */ int fetch_pack_fsck_objects(void); +struct fetch_pack_options { + struct strbuf fsck_msg_types; +}; + +#define FETCH_PACK_OPTIONS_INIT { \ + .fsck_msg_types = STRBUF_INIT, \ +} + #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. In a subsequent commit, the same fetch-pack fsck message configuration needs to be reused. To facilitate this, introduce `fetch_pack_options` which gets written to during the `fetch_pack_config_cb()` instead of directly modifying the `fsck_msg_types` global. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- fetch-pack.c | 16 +++++++++------- fetch-pack.h | 8 ++++++++ 2 files changed, 17 insertions(+), 7 deletions(-)