On Tue, Jul 09, 2024 at 03:24:50PM -0500, Justin Tobler wrote:
> On 24/07/09 08:35PM, shejialuo wrote:
> > The static function "report" provided by "fsck.c" aims at checking fsck
> > error type and calling the callback "error_func" to report the message.
> > However, "report" function is only related to object database which
> > cannot be reused for refs. In order to provide a unified interface which
> > can report either objects or refs, create a new function "vfsck_report"
> > by adding "checked_ref_name" parameter following the "report" prototype.
> > Instead of using "...", provide "va_list" to allow more flexibility.
> >
> > Like "report", the "vfsck_report" function will use "error_func"
> > registered in "fsck_options" to report customized messages. Change
> > "error_func" prototype to align with the new "vfsck_report".
> >
> > Then, change "report" function to use "vfsck_report" to report objects
> > related messages. Add a new function called "fsck_refs_report" to use
> > "vfsck_report" to report refs related messages.
> >
> > Mentored-by: Patrick Steinhardt <ps@xxxxxx>
> > Mentored-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> > Signed-off-by: shejialuo <shejialuo@xxxxxxxxx>
> > ---
> > builtin/fsck.c | 15 ++++-----
> > builtin/mktag.c | 1 +
> > fsck.c | 81 ++++++++++++++++++++++++++++++++++++-------------
> > fsck.h | 40 +++++++++++++++---------
> > object-file.c | 11 ++++---
> > 5 files changed, 101 insertions(+), 47 deletions(-)
> >
> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > index d13a226c2e..de34538c4f 100644
> > --- a/builtin/fsck.c
> > +++ b/builtin/fsck.c
> > @@ -89,12 +89,13 @@ static int objerror(struct object *obj, const char *err)
> > return -1;
> > }
> >
> > -static int fsck_error_func(struct fsck_options *o UNUSED,
> > - const struct object_id *oid,
> > - enum object_type object_type,
> > - enum fsck_msg_type msg_type,
> > - enum fsck_msg_id msg_id UNUSED,
> > - const char *message)
> > +static int fsck_objects_error_func(struct fsck_options *o UNUSED,
> > + const struct object_id *oid,
> > + enum object_type object_type,
> > + const char *checked_ref_name UNUSED,
> > + enum fsck_msg_type msg_type,
> > + enum fsck_msg_id msg_id UNUSED,
> > + const char *message)
>
> This is just a suggestion, but I think it would be slightly easier to
> review if the `*_error_func()` renames were done in a separate preceding
> patch. That way the purpose of the renames can also be clearly
> explained.
>
I agree with this, will change in the next version.
> > {
> > switch (msg_type) {
> > case FSCK_WARN:
> > @@ -938,7 +939,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> >
> > fsck_walk_options.walk = mark_object;
> > fsck_obj_options.walk = mark_used;
> > - fsck_obj_options.error_func = fsck_error_func;
> > + fsck_obj_options.error_func = fsck_objects_error_func;
> > if (check_strict)
> > fsck_obj_options.strict = 1;
> >
> [snip]
> > @@ -166,7 +171,7 @@ struct fsck_options {
> > .gitmodules_done = OIDSET_INIT, \
> > .gitattributes_found = OIDSET_INIT, \
> > .gitattributes_done = OIDSET_INIT, \
> > - .error_func = fsck_error_cb_print_missing_gitmodules, \
> > + .error_func = fsck_objects_error_cb_print_missing_gitmodules, \
> > }
> >
> > /* descend in all linked child objects
> > @@ -209,6 +214,13 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,
> > */
> > int fsck_finish(struct fsck_options *options);
> >
> > +__attribute__((format (printf, 5, 6)))
> > +int fsck_refs_report(struct fsck_options *options,
> > + const struct object_id *oid,
> > + const char *checked_ref_name,
> > + enum fsck_msg_id msg_id,
> > + const char *fmt, ...);
> > +
>
> I think I mentioned this in a previous reply, but it was missed. Not a
> big deal, but it might be nice to document `int fsck_refs_report()`
> here.
>
I will improve this in the next version.
> -Justin
Thanks,
Jialuo
