| Message ID | 20210316161738.30254-23-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsck: API improvements | expand |
On 3/16/2021 12:17 PM, Ævar Arnfjörð Bjarmason wrote: > Refactor the check added in 5476e1efde (fetch-pack: print and use > dangling .gitmodules, 2021-02-22) to make use of us now passing the > "msg_id" to the user defined "error_func". We can now compare against > the FSCK_MSG_GITMODULES_MISSING instead of parsing the generated > message. > > Let's also replace register_found_gitmodules() with directly > manipulating the "gitmodules_found" member. A recent commit moved it > into "fsck_options" so we could do this here. > > Add a fsck-cb.c file similar to parse-options-cb.c, the alternative > would be to either define this directly in fsck.c as a public API, or > to create some library shared by fetch-pack.c ad builtin/index-pack. > > I expect that there won't be many of these fsck utility functions in > the future, so just having a single fsck-cb.c makes sense. I'm not convinced that having a single cb function merits its own file. But, if you expect this pattern to be expanded a couple more times, then I would say it is worth it. Do you have such plans? Thanks, -Stolee
On Tue, Mar 16 2021, Derrick Stolee wrote: > On 3/16/2021 12:17 PM, Ævar Arnfjörð Bjarmason wrote: >> Refactor the check added in 5476e1efde (fetch-pack: print and use >> dangling .gitmodules, 2021-02-22) to make use of us now passing the >> "msg_id" to the user defined "error_func". We can now compare against >> the FSCK_MSG_GITMODULES_MISSING instead of parsing the generated >> message. >> >> Let's also replace register_found_gitmodules() with directly >> manipulating the "gitmodules_found" member. A recent commit moved it >> into "fsck_options" so we could do this here. >> >> Add a fsck-cb.c file similar to parse-options-cb.c, the alternative >> would be to either define this directly in fsck.c as a public API, or >> to create some library shared by fetch-pack.c ad builtin/index-pack. >> >> I expect that there won't be many of these fsck utility functions in >> the future, so just having a single fsck-cb.c makes sense. > > I'm not convinced that having a single cb function merits its > own file. But, if you expect this pattern to be expanded a > couple more times, then I would say it is worth it. Do you have > such plans? Not really, well. Vague ones, but nothing I have even local patches for. It just seemed odd to stick random callback functions shared by related programs into fsck.h's interface, but I guess with FSCK_OPTIONS_MISSING_GITMODULES I already did that. Do you suggest just putting it into fsck.c?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > diff --git a/builtin/index-pack.c b/builtin/index-pack.c > index 5ad80b85b4..11f0fafd33 100644 > --- a/builtin/index-pack.c > +++ b/builtin/index-pack.c > @@ -120,7 +120,7 @@ static int nr_threads; > static int from_stdin; > static int strict; > static int do_fsck_object; > -static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT; > +static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES; Hmph, I do not think this is a good way to go. Specifically, fsck-cb.c with the definition of what this thing is, and in fsck.h file the normal "options" initializers being defined quite far away from where this is defined, it is hard to see what is different between the normal strict one and MISSING_GITMODULES one. Rather, it may be far simpler to keep only DEFAULT and STRICT, and override .error_func at runtime in the codepath(s) that needs to, which would make it more clear what is going on. That way, we do not need the split initializers with _ERROR_FUNC, which is another reason why the approach taken by this series is not a good idea (it does not scale---error-func may seem so special to deserve having two sets of macros that use the default one and leave the member unspecified, but it won't stay to be special forever). IOW, > @@ -1761,7 +1743,6 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) > > read_replace_refs = 0; > fsck_options.walk = mark_link; > - fsck_options.error_func = print_dangling_gitmodules; I doubt this hunk is an improvement. > diff --git a/fsck-cb.c b/fsck-cb.c > new file mode 100644 > index 0000000000..465a49235a > --- /dev/null > +++ b/fsck-cb.c > @@ -0,0 +1,16 @@ > +#include "git-compat-util.h" > +#include "fsck.h" > + > +int fsck_error_cb_print_missing_gitmodules(struct fsck_options *o, > + const struct object_id *oid, > + enum object_type object_type, > + enum fsck_msg_type msg_type, > + enum fsck_msg_id msg_id, > + const char *message) > +{ > + if (msg_id == FSCK_MSG_GITMODULES_MISSING) { > + puts(oid_to_hex(oid)); > + return 0; > + } > + return fsck_error_function(o, oid, object_type, msg_type, msg_id, message); > +} As Derrick noticed, I do not know if we want to have a separate file for this single function. Shouldn't it be part of builtin/index-pack.c, or do we want other places to do the same kind of checks? Thanks.
On 3/17/2021 9:47 AM, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Mar 16 2021, Derrick Stolee wrote: > >> On 3/16/2021 12:17 PM, Ævar Arnfjörð Bjarmason wrote: >>> I expect that there won't be many of these fsck utility functions in >>> the future, so just having a single fsck-cb.c makes sense. >> >> I'm not convinced that having a single cb function merits its >> own file. But, if you expect this pattern to be expanded a >> couple more times, then I would say it is worth it. Do you have >> such plans? > > Not really, well. Vague ones, but nothing I have even local patches for. > > It just seemed odd to stick random callback functions shared by related > programs into fsck.h's interface, but I guess with > FSCK_OPTIONS_MISSING_GITMODULES I already did that. > > Do you suggest just putting it into fsck.c? Yeah, if it is frequently paired with fsck operations, I think it makes the most sense there. And looking at it again, I'm not sure parse-options-cb.c has a good excuse for being separate from parse-options.c, but that's the current state so I wouldn't change it now. Thanks, -Stolee
diff --git a/Makefile b/Makefile index dfb0f1000f..3faa8bd0d3 100644 --- a/Makefile +++ b/Makefile @@ -882,6 +882,7 @@ LIB_OBJS += fetch-negotiator.o LIB_OBJS += fetch-pack.o LIB_OBJS += fmt-merge-msg.o LIB_OBJS += fsck.o +LIB_OBJS += fsck-cb.o LIB_OBJS += fsmonitor.o LIB_OBJS += gettext.o LIB_OBJS += gpg-interface.o diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 5ad80b85b4..11f0fafd33 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -120,7 +120,7 @@ static int nr_threads; static int from_stdin; static int strict; static int do_fsck_object; -static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT; +static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES; static int verbose; static int show_resolving_progress; static int show_stat; @@ -1713,24 +1713,6 @@ static void show_pack_info(int stat_only) } } -static int print_dangling_gitmodules(struct fsck_options *o, - const struct object_id *oid, - enum object_type object_type, - enum fsck_msg_type msg_type, - enum fsck_msg_id msg_id, - const char *message) -{ - /* - * NEEDSWORK: Plumb the MSG_ID (from fsck.c) here and use it - * instead of relying on this string check. - */ - if (starts_with(message, "gitmodulesMissing")) { - printf("%s\n", oid_to_hex(oid)); - return 0; - } - return fsck_error_function(o, oid, object_type, msg_type, msg_id, message); -} - int cmd_index_pack(int argc, const char **argv, const char *prefix) { int i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index; @@ -1761,7 +1743,6 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) read_replace_refs = 0; fsck_options.walk = mark_link; - fsck_options.error_func = print_dangling_gitmodules; reset_pack_idx_option(&opts); git_config(git_index_pack_config, &opts); diff --git a/fetch-pack.c b/fetch-pack.c index 229fd8e2c2..008a3facd4 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -38,7 +38,7 @@ static int server_supports_filtering; static int advertise_sid; static struct shallow_lock shallow_lock; static const char *alternate_shallow_file; -static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT; +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; @@ -998,7 +998,7 @@ static void fsck_gitmodules_oids(struct oidset *gitmodules_oids) oidset_iter_init(gitmodules_oids, &iter); while ((oid = oidset_iter_next(&iter))) - register_found_gitmodules(&fsck_options, oid); + oidset_insert(&fsck_options.gitmodules_found, oid); if (fsck_finish(&fsck_options)) die("fsck failed"); } diff --git a/fsck-cb.c b/fsck-cb.c new file mode 100644 index 0000000000..465a49235a --- /dev/null +++ b/fsck-cb.c @@ -0,0 +1,16 @@ +#include "git-compat-util.h" +#include "fsck.h" + +int fsck_error_cb_print_missing_gitmodules(struct fsck_options *o, + const struct object_id *oid, + enum object_type object_type, + enum fsck_msg_type msg_type, + enum fsck_msg_id msg_id, + const char *message) +{ + if (msg_id == FSCK_MSG_GITMODULES_MISSING) { + puts(oid_to_hex(oid)); + return 0; + } + return fsck_error_function(o, oid, object_type, msg_type, msg_id, message); +} diff --git a/fsck.c b/fsck.c index 565274a946..b0089844db 100644 --- a/fsck.c +++ b/fsck.c @@ -1214,11 +1214,6 @@ int fsck_error_function(struct fsck_options *o, return 1; } -void register_found_gitmodules(struct fsck_options *options, const struct object_id *oid) -{ - oidset_insert(&options->gitmodules_found, oid); -} - int fsck_finish(struct fsck_options *options) { int ret = 0; diff --git a/fsck.h b/fsck.h index bb59ef05b6..ae3107638a 100644 --- a/fsck.h +++ b/fsck.h @@ -153,9 +153,6 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options); int fsck_object(struct object *obj, void *data, unsigned long size, struct fsck_options *options); -void register_found_gitmodules(struct fsck_options *options, - const struct object_id *oid); - /* * fsck a tag, and pass info about it back to the caller. This is * exposed fsck_object() internals for git-mktag(1). @@ -204,4 +201,23 @@ const char *fsck_describe_object(struct fsck_options *options, int fsck_config_internal(const char *var, const char *value, void *cb, struct fsck_options *options); +/* + * Initializations for callbacks in fsck-cb.c + */ +#define FSCK_OPTIONS_MISSING_GITMODULES { \ + .strict = 1, \ + .error_func = fsck_error_cb_print_missing_gitmodules, \ + FSCK_OPTIONS_COMMON \ +} + +/* + * Error callbacks in fsck-cb.c + */ +int fsck_error_cb_print_missing_gitmodules(struct fsck_options *o, + const struct object_id *oid, + enum object_type object_type, + enum fsck_msg_type msg_type, + enum fsck_msg_id msg_id, + const char *message); + #endif
Refactor the check added in 5476e1efde (fetch-pack: print and use dangling .gitmodules, 2021-02-22) to make use of us now passing the "msg_id" to the user defined "error_func". We can now compare against the FSCK_MSG_GITMODULES_MISSING instead of parsing the generated message. Let's also replace register_found_gitmodules() with directly manipulating the "gitmodules_found" member. A recent commit moved it into "fsck_options" so we could do this here. Add a fsck-cb.c file similar to parse-options-cb.c, the alternative would be to either define this directly in fsck.c as a public API, or to create some library shared by fetch-pack.c ad builtin/index-pack. I expect that there won't be many of these fsck utility functions in the future, so just having a single fsck-cb.c makes sense. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 1 + builtin/index-pack.c | 21 +-------------------- fetch-pack.c | 4 ++-- fsck-cb.c | 16 ++++++++++++++++ fsck.c | 5 ----- fsck.h | 22 +++++++++++++++++++--- 6 files changed, 39 insertions(+), 30 deletions(-) create mode 100644 fsck-cb.c