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

From: Justin Tobler <jltobler@gmail.com>
Date: Jul 9, 2024, 21:29 UTC
Message-ID: <oan7pcamqpi2z4jzfshff2sfziqgisd3jcbpzfoiuyks5g57mk@7isgeztkcgmy>
In-Reply-To: <Zo0uiz1y6hJld2Rv@ArchLinux>

On 24/07/09 08:35PM, shejialuo wrote:

Show 6 quoted lines

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

Show 31 quoted lines

> 
> Last, add "FSCK_REFS_OPTIONS_DEFAULT" and "FSCK_REFS_OPTIONS_STRICT"
> macros to create refs options easily.
> 
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  fsck.c | 23 +++++++++++++++++++++++
>  fsck.h | 15 +++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/fsck.c b/fsck.c
> index e1819964e3..c5c7e8454f 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -1252,6 +1252,29 @@ 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_reset(&sb);

Naive question, is there reason to reset `sb` immediately after `STRBUF_INIT`? My understanding is that because we initialize the buffer, the other fields should also be zeroed. If so, resetting the buffer here seems redundant.

Show 36 quoted lines

> +	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;
> +}
> +
>  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 8ce48395f6..ff52913494 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;

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.

Show 20 quoted lines

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