| Message ID | ZqeYm2J1LeXn_1-4@ArchLinux (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ref consistency check infra setup | expand |
On Mon, Jul 29, 2024 at 09:26:51PM +0800, shejialuo wrote: > Add refs-related options to the "fsck_options", create refs-specific > "error_func" callback "fsck_refs_error_function". We should have an explanation _why_ we are adding these functions in the commit message. > "fsck_refs_error_function" will use the "oid" parameter. When the caller > passes the oid, it will use "oid_to_hex" to get the corresponding hex > value to report to the caller. > > Last, add "FSCK_REFS_OPTIONS_DEFAULT" and "FSCK_REFS_OPTIONS_STRICT" > macros to create refs options easily. It is a bit unclear to me what you mean with "create refs options easily". Do you mean to say that `git refs check` (or whatever this will be called) will have flags like "--strict"? > Mentored-by: Patrick Steinhardt <ps@pks.im> > Mentored-by: Karthik Nayak <karthik.188@gmail.com> > Signed-off-by: shejialuo <shejialuo@gmail.com> > --- > fsck.c | 25 +++++++++++++++++++++++++ > fsck.h | 14 ++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/fsck.c b/fsck.c > index af61fa90ba..56de29b4c0 100644 > --- a/fsck.c > +++ b/fsck.c > @@ -1251,6 +1251,31 @@ int fsck_objects_error_function(struct fsck_options *o, > return 1; > } > > +int fsck_refs_error_function(struct fsck_options *options, > + const struct object_id *oid, > + enum object_type object_type UNUSED, > + const struct fsck_refs_info *refs_info, > + enum fsck_msg_type msg_type, > + enum fsck_msg_id msg_id UNUSED, > + const char *message) > +{ > + struct strbuf sb = STRBUF_INIT; > + int ret = 0; > + > + strbuf_addstr(&sb, refs_info->path); > + > + if (oid) > + strbuf_addf(&sb, " -> (%s)", oid_to_hex(oid)); Okay, so we do end up printing the object ID indeed. But wouldn't we want to potentially do the same with symbolic refs? Also, would it make more sense to put the `oid` (and potentially the `referent` when we also handle symbolic refs) into `struct fsck_refs_info`? Like this, the whole state would be self-contained in that structure, which would also make my proposal from a preceding commit more feasible where the subsystem-specific error functions only get a void pointer to this structure. It would require another refactoring on top to move the object type and OID into a `struct fsck_objects_info`, too, but that shouldn't be too involved, I guess. Patrick
On Tue, Jul 30, 2024 at 10:31:26AM +0200, Patrick Steinhardt wrote: > On Mon, Jul 29, 2024 at 09:26:51PM +0800, shejialuo wrote: > > Add refs-related options to the "fsck_options", create refs-specific > > "error_func" callback "fsck_refs_error_function". > > We should have an explanation _why_ we are adding these functions in the > commit message. > Yes, I will improve this in the next version. > > "fsck_refs_error_function" will use the "oid" parameter. When the caller > > passes the oid, it will use "oid_to_hex" to get the corresponding hex > > value to report to the caller. > > > > Last, add "FSCK_REFS_OPTIONS_DEFAULT" and "FSCK_REFS_OPTIONS_STRICT" > > macros to create refs options easily. > > It is a bit unclear to me what you mean with "create refs options > easily". Do you mean to say that `git refs check` (or whatever this will > be called) will have flags like "--strict"? > Yes, when the user passes `--strict`, all the warn type will be seen as the error type. So I create "FSCK_REFS_OPTIONS_STRICT". However, I didn't think too much here. I just followed the way the codebase does for the objects. > > Mentored-by: Patrick Steinhardt <ps@pks.im> > > Mentored-by: Karthik Nayak <karthik.188@gmail.com> > > Signed-off-by: shejialuo <shejialuo@gmail.com> > > --- > > fsck.c | 25 +++++++++++++++++++++++++ > > fsck.h | 14 ++++++++++++++ > > 2 files changed, 39 insertions(+) > > > > diff --git a/fsck.c b/fsck.c > > index af61fa90ba..56de29b4c0 100644 > > --- a/fsck.c > > +++ b/fsck.c > > @@ -1251,6 +1251,31 @@ int fsck_objects_error_function(struct fsck_options *o, > > return 1; > > } > > > > +int fsck_refs_error_function(struct fsck_options *options, > > + const struct object_id *oid, > > + enum object_type object_type UNUSED, > > + const struct fsck_refs_info *refs_info, > > + enum fsck_msg_type msg_type, > > + enum fsck_msg_id msg_id UNUSED, > > + const char *message) > > +{ > > + struct strbuf sb = STRBUF_INIT; > > + int ret = 0; > > + > > + strbuf_addstr(&sb, refs_info->path); > > + > > + if (oid) > > + strbuf_addf(&sb, " -> (%s)", oid_to_hex(oid)); > > Okay, so we do end up printing the object ID indeed. But wouldn't we > want to potentially do the same with symbolic refs? > > Also, would it make more sense to put the `oid` (and potentially the > `referent` when we also handle symbolic refs) into `struct > fsck_refs_info`? Like this, the whole state would be self-contained in > that structure, which would also make my proposal from a preceding > commit more feasible where the subsystem-specific error functions only > get a void pointer to this structure. It would require another > refactoring on top to move the object type and OID into a `struct > fsck_objects_info`, too, but that shouldn't be too involved, I guess. > Yes, I totally agree here. I didn't consider symrefs here, actually we should provide "symref -> referent" message if we have providen the "regular ref -> oid" message. I think we should do this. And I have commented on "[Patch v13][2/10]", it is a necessity we should refactor this part. > Patrick
diff --git a/fsck.c b/fsck.c index af61fa90ba..56de29b4c0 100644 --- a/fsck.c +++ b/fsck.c @@ -1251,6 +1251,31 @@ int fsck_objects_error_function(struct fsck_options *o, return 1; } +int fsck_refs_error_function(struct fsck_options *options, + const struct object_id *oid, + enum object_type object_type UNUSED, + const struct fsck_refs_info *refs_info, + enum fsck_msg_type msg_type, + enum fsck_msg_id msg_id UNUSED, + const char *message) +{ + struct strbuf sb = STRBUF_INIT; + int ret = 0; + + strbuf_addstr(&sb, refs_info->path); + + if (oid) + strbuf_addf(&sb, " -> (%s)", oid_to_hex(oid)); + + if (msg_type == FSCK_WARN) + warning("%s: %s", sb.buf, message); + else + ret = error("%s: %s", sb.buf, message); + + strbuf_release(&sb); + return ret; +} + static int fsck_blobs(struct oidset *blobs_found, struct oidset *blobs_done, enum fsck_msg_id msg_missing, enum fsck_msg_id msg_type, struct fsck_options *options, const char *blob_type) diff --git a/fsck.h b/fsck.h index f53ac339d2..a4a4ba88ee 100644 --- a/fsck.h +++ b/fsck.h @@ -136,6 +136,13 @@ int fsck_objects_error_cb_print_missing_gitmodules(struct fsck_options *o, enum fsck_msg_type msg_type, enum fsck_msg_id msg_id, const char *message); +int fsck_refs_error_function(struct fsck_options *options, + const struct object_id *oid, + enum object_type object_type, + const struct fsck_refs_info *refs_info, + enum fsck_msg_type msg_type, + enum fsck_msg_id msg_id, + const char *message); /* * The information for reporting refs-related error message @@ -181,6 +188,13 @@ struct fsck_options { .gitattributes_done = OIDSET_INIT, \ .error_func = fsck_objects_error_cb_print_missing_gitmodules, \ } +#define FSCK_REFS_OPTIONS_DEFAULT { \ + .error_func = fsck_refs_error_function, \ +} +#define FSCK_REFS_OPTIONS_STRICT { \ + .strict = 1, \ + .error_func = fsck_refs_error_function, \ +} /* descend in all linked child objects * the return value is:
Add refs-related options to the "fsck_options", create refs-specific "error_func" callback "fsck_refs_error_function". "fsck_refs_error_function" will use the "oid" parameter. When the caller passes the oid, it will use "oid_to_hex" to get the corresponding hex value to report to the caller. Last, add "FSCK_REFS_OPTIONS_DEFAULT" and "FSCK_REFS_OPTIONS_STRICT" macros to create refs options easily. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: shejialuo <shejialuo@gmail.com> --- fsck.c | 25 +++++++++++++++++++++++++ fsck.h | 14 ++++++++++++++ 2 files changed, 39 insertions(+)