| Message ID | 20241121204119.1440773-5-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: > With `fetch_pack_config_cb()`, fsck configuration gets populated to a > `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate > formatted fsck message configuration generation. In a subsequent commit, > this is used to wire message configuration to `unbundle()` during bundle > fetches. This is generally going in the right direction, but this particular iteration is highly disappointing for two reasons. - The callback calls git_default_config() at end. Other callers may not want it happen. Think of the reason why a new caller may want to use this callback (see the next item). - fetch_pack_config_cb() was perfectly good name back when it was hidden inside fetch-pack.c, as a private internal implementation detail, EVEN THOUGH it did not give its callers everything that tries to configure the behaviour of fetch-pack. It ONLY is about how fsck behaviour is affected. It must be renamed so that any new caller can realize that it is configuring fsck checking machinery and not general fetch-pack features. So, I would suggest making at least two changes. - rename it to a more sensible name that includes "fsck" somewhere (as it is about "fetch.fsck.*" configuration variables, "fetch" should also stay in the name). Let's tentatively call it foo(). - stop calling git_default_config() from foo(). Instead, return some special value foo() does not currently return, let's say -1 to signal that the key was something foo() was not interested in, and write a thin replacement helper static int fetch_pack_config_cb(...) { int st = foo(...); if (st < 0) return git_default_config(var, value, ctx, cb); return st; } and call that from fetch_pack_config(). No, "foo()" has neither "fetch" or "fsck" in it; I am not suggesting to use that as the final name ;-). Thanks.
On Thu, Nov 21, 2024 at 02:41:18PM -0600, Justin Tobler wrote: > During fetch-pack operations, git-index-pack(1) may be spawned and > perform fsck checks. The message severity of these checks is > configurable and propagated via appending it to the `--fsck-objects` > option. > > With `fetch_pack_config_cb()`, fsck configuration gets populated to a > `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate > formatted fsck message configuration generation. In a subsequent commit, > this is used to wire message configuration to `unbundle()` during bundle > fetches. > In my perspective, we may not separate [PATCH 4/5] and [PATCH 5/5]. Should the reason why we want to expose `fetch_pack_config_cb` is that we need to propagate the fsck severity to `unbundle`? Without the information of the last patch, we cannot know any detail thing. So, they are highly relevant. However, from the comment of the Junio, there are a lot of things need to be changed. So, just a comment. Thanks, Jialuo
On 24/11/22 10:57AM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > With `fetch_pack_config_cb()`, fsck configuration gets populated to a > > `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate > > formatted fsck message configuration generation. In a subsequent commit, > > this is used to wire message configuration to `unbundle()` during bundle > > fetches. > > This is generally going in the right direction, but this particular > iteration is highly disappointing for two reasons. > > - The callback calls git_default_config() at end. Other callers > may not want it happen. Think of the reason why a new caller may > want to use this callback (see the next item). > > - fetch_pack_config_cb() was perfectly good name back when it was > hidden inside fetch-pack.c, as a private internal implementation > detail, EVEN THOUGH it did not give its callers everything that > tries to configure the behaviour of fetch-pack. It ONLY is about > how fsck behaviour is affected. It must be renamed so that any > new caller can realize that it is configuring fsck checking > machinery and not general fetch-pack features. > > So, I would suggest making at least two changes. > > - rename it to a more sensible name that includes "fsck" somewhere > (as it is about "fetch.fsck.*" configuration variables, "fetch" > should also stay in the name). Let's tentatively call it foo(). > > - stop calling git_default_config() from foo(). Instead, return > some special value foo() does not currently return, let's say -1 > to signal that the key was something foo() was not interested in, > and write a thin replacement helper > > static int fetch_pack_config_cb(...) > { > int st = foo(...); > if (st < 0) > return git_default_config(var, value, ctx, cb); > return st; > } > > and call that from fetch_pack_config(). > > No, "foo()" has neither "fetch" or "fsck" in it; I am not suggesting > to use that as the final name ;-). > > Thanks. Thanks for the suggestions. I'll factor out the fsck configuration bit as suggested and name it something like "fetch_pack_fsck_config()". The new name should be more clear and this change will also ensure only the desired fsck configuration is being parsed which makes more sense. :) -Justin
diff --git a/fetch-pack.c b/fetch-pack.c index 73309f8043..10b66795bc 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_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; diff --git a/fetch-pack.h b/fetch-pack.h index 8243b754ce..f35a75a3c5 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -1,6 +1,7 @@ #ifndef FETCH_PACK_H #define FETCH_PACK_H +#include "config.h" #include "string-list.h" #include "protocol.h" #include "list-objects-filter-options.h" @@ -114,4 +115,7 @@ struct fetch_pack_options { .fsck_msg_types = STRBUF_INIT, \ } +int fetch_pack_config_cb(const char *var, const char *value, + const struct config_context *ctx, void *cb); + #endif
During fetch-pack operations, git-index-pack(1) may be spawned and perform fsck checks. The message severity of these checks is configurable and propagated via appending it to the `--fsck-objects` option. With `fetch_pack_config_cb()`, fsck configuration gets populated to a `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate formatted fsck message configuration generation. In a subsequent commit, this is used to wire message configuration to `unbundle()` during bundle fetches. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- fetch-pack.c | 4 ++-- fetch-pack.h | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-)