Subject: Re: [GSoC][PATCH v11 02/10] fsck: add a unified interface for reporting fsck messages

From: shejialuo <[email-masked]>
Date: Sat, 20 Jul 2024 15:24:38 +0800

On Thu, Jul 18, 2024 at 06:26:30AM -0700, Karthik Nayak wrote:
> shejialuo <[email-masked]> writes:
>
> > diff --git a/fsck.h b/fsck.h
> > index bcfb2e34cd..61ca38afd6 100644
> > --- a/fsck.h
> > +++ b/fsck.h
> > @@ -114,19 +114,25 @@ int is_valid_msg_type(const char *msg_id, const char *msg_type);
> >  typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,
> >  			      void *data, struct fsck_options *options);
> >
> > -/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */
> > +/*
> > + * callback function for reporting errors when checking either objects or refs
> > + */
> >  typedef int (*fsck_error)(struct fsck_options *o,
> >  			  const struct object_id *oid, enum object_type object_type,
> > +			  const char *ref_checkee, const char *sub_ref_checkee,
> 
> This makes me really wonder if this is the best way we can do this? This
> seems to solve for the current situation, but what happens if you want
> to also adding the reftable size or packed-refs size here? Would you
> introduce another field?
> 
> would it be better to add a single `const struct *fsck_refs_info`
> instead?
> 
> Perhaps something like:
> 
> struct fsck_refs_info {
>        char *refname;
>        union {
>              struct {
>                     ...
>              } reftable;
>              struct {
>                     ...
>              } files;
>        } u;
> }
> 
> Of course we can fill in the details as we need them.
> 

I agree, we should design an extensible data structure here. I will use
this idea. Because we don't know what we will do in the current time.
However, I think "refname" is not good, instead I decide to use
"ref_checkee", "refname" may let caller think we only check the refname.
However, we need to also check reflog.