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

[shejialuo <shejialuo@gmail.com>]

On Mon, Jul 08, 2024 at 07:49:43AM -0700, Karthik Nayak wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > 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.
> >
> 
> Carrying over from the previous commit, couldn't we simply do something
> like:
> 
>     diff --git a/fsck.c b/fsck.c
>     index eea7145470..32ae36a4fc 100644
>     --- a/fsck.c
>     +++ b/fsck.c
>     @@ -1202,17 +1203,33 @@ int fsck_buffer(const struct object_id
> *oid, enum object_type type,
> 
>      int fsck_error_function(struct fsck_options *o,
>      			const struct object_id *oid,
>     -			enum object_type object_type UNUSED,
>     +			enum object_type object_type,
>     +			const char *checked_ref_name,
>      			enum fsck_msg_type msg_type,
>      			enum fsck_msg_id msg_id UNUSED,
>      			const char *message)
>      {
>     +	static struct strbuf sb = STRBUF_INIT;
>     +	int ret = 0;
>     +
>     +	if (checked_ref_name) {
>     +		strbuf_addstr("ref %s", checked_ref_name);
>     +		if (oid)
>     +			strbuf_addf(&sb, " -> (%s)", oid_to_hex(oid));
>     +	} else {
>     +		strbuf_addstr("object %s", fsck_describe_object(o, oid));
>     +	}
>     +
>      	if (msg_type == FSCK_WARN) {
>     -		warning("object %s: %s", fsck_describe_object(o, oid), message);
>     -		return 0;
>     +		warning("%s: %s", sb.buf, message);
>     +		ret = 0;
>     +		goto cleanup;
>      	}
>     -	error("object %s: %s", fsck_describe_object(o, oid), message);
>     -	return 1;
>     +	error("%s: %s", sb.buf, message);
>     +
>     +cleanup:
>     +	strbuf_release(&sb);
>     +	return ret;
>      }
> 
> 

However, "fsck_error_function" will be used as the callback for
object-related checks. I don't think we should use one function to
incorporate all the situations.

For example, if we pass both "checked_ref_name" and "oid" here, how does
this one function knows whether we handle refs or objects. Although we
may use some flags here to choose the different situations, it's bad
idea from my perspective. Adding a new callback here will be cleaner.

> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> > Signed-off-by: shejialuo <shejialuo@gmail.com>
> > ---
> >  fsck.c | 22 ++++++++++++++++++++++
> >  fsck.h | 15 +++++++++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/fsck.c b/fsck.c
> > index 7182ce8e80..d1dcbdcac2 100644
> > --- a/fsck.c
> > +++ b/fsck.c
> > @@ -1252,6 +1252,28 @@ int fsck_objects_error_function(struct fsck_options *o,
> >  	return 1;
> >  }
> >
> > +int fsck_refs_error_function(struct fsck_options *options UNUSED,
> > +			     const struct object_id *oid,
> > +			     enum object_type object_type UNUSED,
> > +			     const char *checked_ref_name,
> > +			     enum fsck_msg_type msg_type,
> > +			     enum fsck_msg_id msg_id UNUSED,
> > +			     const char *message)
> > +{
> > +	static struct strbuf sb = STRBUF_INIT;
> > +
> > +	strbuf_addstr(&sb, checked_ref_name);
> > +	if (oid)
> > +		strbuf_addf(&sb, " -> (%s)", oid_to_hex(oid));
> > +
> > +	if (msg_type == FSCK_WARN) {
> > +		warning("%s: %s", sb.buf, message);
> > +		return 0;
> > +	}
> > +	error("%s: %s", sb.buf, message);
> > +	return 1;
> > +}
> > +
> 
> We don't free strbuf here.
> 

Yes, I made a mistake here. I should not define the "sb" static, will
fix it in the next version.

> >  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 f703dfb5e8..246055c0f9 100644
> > --- a/fsck.h
> > +++ b/fsck.h
> > @@ -135,11 +135,19 @@ 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 char *checked_ref_name,
> > +			     enum fsck_msg_type msg_type,
> > +			     enum fsck_msg_id msg_id,
> > +			     const char *message);
> >
> >  struct fsck_options {
> >  	fsck_walk_func walk;
> >  	fsck_error error_func;
> >  	unsigned strict:1;
> > +	unsigned verbose_refs:1;
> 
> Nit: Here we have the subject 'refs' towards the end of the name.
> 
> >  	enum fsck_msg_type *msg_type;
> >  	struct oidset oid_skiplist;
> 
> but here we have the subject 'oid' at the start of the name. Perhaps we
> can rename this to 'skip_oids'?
> 

I agree. "skip_oids" will be more meaningful. I will fix it in the next
version.

> >  	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