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

[shejialuo <shejialuo@gmail.com>]

On Tue, Jul 30, 2024 at 10:31:16AM +0200, Patrick Steinhardt wrote:
> On Mon, Jul 29, 2024 at 09:26:30PM +0800, 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.
> 
> Nit: it would be nice to mention _why_ it cannot be reused for refs.
> 
> > diff --git a/fsck.c b/fsck.c
> > index 3f32441492..1185e9a8ad 100644
> > --- a/fsck.c
> > +++ b/fsck.c
> > @@ -226,12 +226,18 @@ static int object_on_skiplist(struct fsck_options *opts,
> >  	return opts && oid && oidset_contains(&opts->skip_oids, oid);
> >  }
> >  
> > -__attribute__((format (printf, 5, 6)))
> > -static int report(struct fsck_options *options,
> > -		  const struct object_id *oid, enum object_type object_type,
> > -		  enum fsck_msg_id msg_id, const char *fmt, ...)
> > +/*
> > + * Provide a unified interface for either fscking refs or objects.
> > + * It will get the current msg error type and call the error_func callback
> > + * which is registered in the "fsck_options" struct.
> > + */
> > +static int fsck_vreport(struct fsck_options *options,
> > +			const struct object_id *oid,
> > +			enum object_type object_type,
> > +			const struct fsck_refs_info *refs_info,
> > +			enum fsck_msg_id msg_id, const char *fmt, va_list ap)
> >  {
> > -	va_list ap;
> > +	va_list ap_copy;
> >  	struct strbuf sb = STRBUF_INIT;
> >  	enum fsck_msg_type msg_type = fsck_msg_type(msg_id, options);
> >  	int result;
> 
> It is a bit weird that this new generic function receives non-generic
> inputs which are specific to the respective subsystems (objects or refs)
> that we are checking.
> 

Actually, this is one of the biggest problem when implementing the
infrastructure. The original function "report" only cares about
reporting the problem of objects. So the callback "error_func" uses the
similar prototype.

Problem comes when we want to add ref-related report. In my very former
implementation, I just created a new function "fsck_refs_report" to just
copy some codes from "report" and defines refs-related callback.

However, this is a bad way because we make duplication. If we want to
reuse the "report" function, we should add new parameters into "report"
and "error_func". This is the idea of this patch. However, as you can
see, there are so many "report" function calls in the codebase, it's bad
to change them. So I define a more common function called "fsck_vreport"
function and wrap "report" to eventually call this function.

> A better design would likely be to make `error_func()` receive a void
> pointer such that `error_func()` and then have the respective subsystems
> provide a function that knows to format the message while receiving
> either a `struct fsck_object_report *` or a `struct fsck_ref_report *`.
> 

Yes, I agree with this idea. And I think we should use only one function
called "fsck_reportf" to report any fsck-related messages. We could
design the following callback "prototype".

  typedef int (*fsck_error)(struct fsck_options *o,
                            void *info,
                            enum fsck_msg_type msg_type, enum fsck_msg_id msg_id,
                            const char *message);

Thus, we could make "fsck_reportf" generic. It will handle the common
"fsck_options" and "enum fsck_msg_id" and then it will call "fsck_error"
callback. The user could pass either refs information or objects
information.

> I don't think this is particularly worriesome though as it is still
> manageable right now. So I'm fine if we want to leave this as-is, and
> then we can iterate on this in a future patch series as required.
> 

I strongly suggest that we should use the above design for the following
reasons:

1. We only expose one interface called "fsck_reportf" which will make
the code clear. Actually, there is no different between reporting refs
and reporting objects.
2. We provide more extensibility here, because we will never change
"fsck_reportf" and "fsck_error" prototype when we want to add more info
for either refs or objects.

But do we really need this? Junio, could you please give some advice
here. How do you think about this design. In my perspective, the only
overhead here is that there are too many "report" function we should
refactor.

> > @@ -250,9 +256,9 @@ static int report(struct fsck_options *options,
> >  	prepare_msg_ids();
> >  	strbuf_addf(&sb, "%s: ", msg_id_info[msg_id].camelcased);
> >  
> > -	va_start(ap, fmt);
> > -	strbuf_vaddf(&sb, fmt, ap);
> > -	result = options->error_func(options, oid, object_type,
> > +	va_copy(ap_copy, ap);
> > +	strbuf_vaddf(&sb, fmt, ap_copy);
> > +	result = options->error_func(options, oid, object_type, refs_info,
> >  				     msg_type, msg_id, sb.buf);
> >  	strbuf_release(&sb);
> >  	va_end(ap);
> > @@ -260,6 +266,35 @@ static int report(struct fsck_options *options,
> >  	return result;
> >  }
> >  
> > +__attribute__((format (printf, 5, 6)))
> > +static int report(struct fsck_options *options,
> > +		  const struct object_id *oid, enum object_type object_type,
> > +		  enum fsck_msg_id msg_id, const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +	int result;
> > +
> > +	va_start(ap, fmt);
> > +	result = fsck_vreport(options, oid, object_type, NULL, msg_id, fmt, ap);
> > +	va_end(ap);
> > +
> > +	return result;
> > +}
> 
> As far as I can see, `report()` is now specific to reporting errors with
> objects while `fsck_vreport()` is the generic part. Do we want to rename
> the function to `fsck_report_object()` to clarify, or would that cause
> too much churn?
> 
> Hm. Seeing that we have 62 callsites of that function it may be too much
> churn indeed.
> 

Yes, there are too many references for "report" function. That's why I
wrap the "report" using "fsck_vreport".

> > +int fsck_refs_report(struct fsck_options *options,
> > +		     const struct object_id *oid,
> > +		     const struct fsck_refs_info *refs_info,
> > +		     enum fsck_msg_id msg_id, const char *fmt, ...)
> 
> Would `fsck_report_ref()` be a better name?
> 

I agree. However, if we use the above design, we will just use
"fsck_reportf" here both for refs and objects.

> What is the intent of the `oid` field? Would it be set to the object ID
> that a reference points to? What if the reference is a non-resolving
> symbolic reference? I wonder whether we can just remove it.
> 

`oid` is used to be the object ID that a reference points to. If the
reference is a symbolic link or symref, we do not care about it. The
caller should just pass `NULL`. Actually, we may not use this field.
I just suppose that we may provide the user more information. Because
when using "file-backend.c::parse_loose_ref_contents()" we will
automatically get the `oid` if the ref is a regular reference. So I just
provide `oid` here.

> Patrick