Re: [GSoC][PATCH v9 3/9] fsck: add refs-related options and error report function

[shejialuo <shejialuo@gmail.com>]

On Tue, Jul 09, 2024 at 04:29:24PM -0500, Justin Tobler wrote:
> On 24/07/09 08:35PM, shejialuo wrote:
> > 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.
> 
> Out of curiousity, under what circumstances would the caller want to
> also pass the oid? Would it simply be to optionally provide additional
> context?
> 

Because when we check the refs, we will use "parse_loose_ref_contents"
here to check the ref contents. Below is the prototype:

  int parse_loose_ref_contents(const char *buf,
                               struct object_id *oid,
                               ...)

So we could get a oid here. However, we don't know the type of the oid.
It may not be commit object but rather a tag object. And I want to
provide more flexible operations for caller. When caller passes the oid.
The message could be the following:

  ref_name -> (oid) : fsck_error_type: user-passed message.

So, actually we have provided additional context for the caller. From
another perspective, the object check needs the "oid" parameter, we
cannot remove it from the callback "error_func" prototype. So why not
just reuse this parameter? It truly provides the caller more flexibility
without big changes.

> >  struct fsck_options {
> >  	fsck_walk_func walk;
> >  	fsck_error error_func;
> >  	unsigned strict:1;
> > +	unsigned verbose_refs:1;
> 
> What is the purpose of adding `verbose_refs` in this patch? At this
> point, I'm not seeing it used. If there is a reason to be included in
> this patch, it might be nice to mention in the commit message.
> 

The reason is that fsck builtin handles the object check but we want to
use "git refs verify" command to handle ref check and we put all the
real functionality into each file backend. And there is only one entry
point in the "git refs verify" command. So we need to use "fsck_options"
as the parameter to maintain this state across the ref checks.

Actually, "git-fsck(1)" maintains "verbose" global variable. I think I
should not add this option in this patch which may cause a lot of
confusion here. I will improve this in the next version.

> >  	enum fsck_msg_type *msg_type;
> >  	struct oidset skip_oids;
> >  	struct oidset gitmodules_found;
> > @@ -173,6 +181,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:
> > -- 
> > 2.45.2
> >