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

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4490 bytes --]

shejialuo <shejialuo@gmail.com> writes:

> 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. In order to provide a unified interface which
> can report either objects or refs, create a new function "fsck_vreport"
> following the "report" prototype. Instead of using "...", provide
> "va_list" to allow more flexibility.
>
> When checking loose refs and reflogs, we only need to pass the checked
> name to the fsck error report function. However, for packed-refs and
> reftable refs, we need to check both the consistency of the file itself
> and the refs or reflogs contained in the file. In order to provide above
> checks, add two parameters "ref_checkee" and "sub_ref_checkee" in
> "fsck_vreport" function.

Nit: It would be nice, if you described here, what is the expected usage
of "ref_checkee" and "sub_ref_checkee".

[snip]

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

>  			  enum fsck_msg_type msg_type, enum fsck_msg_id msg_id,
>  			  const char *message);
>
>  int fsck_error_function(struct fsck_options *o,
>  			const struct object_id *oid, enum object_type object_type,
> +			const char *ref_checkee, const char *sub_ref_checkee,
>  			enum fsck_msg_type msg_type, enum fsck_msg_id msg_id,
>  			const char *message);
>  int fsck_error_cb_print_missing_gitmodules(struct fsck_options *o,
>  					   const struct object_id *oid,
>  					   enum object_type object_type,
> +					   const char *ref_checkee,
> +					   const char *sub_ref_checkee,
>  					   enum fsck_msg_type msg_type,
>  					   enum fsck_msg_id msg_id,
>  					   const char *message);
> @@ -209,6 +215,17 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,
>   */
>  int fsck_finish(struct fsck_options *options);
>
> +/*
> + * Report an error or warning for refs.
> + */
> +__attribute__((format (printf, 6, 7)))
> +int fsck_refs_report(struct fsck_options *options,
> +		     const struct object_id *oid,
> +		     const char *ref_checkee,
> +		     const char *sub_ref_checkee,
> +		     enum fsck_msg_id msg_id,
> +		     const char *fmt, ...);
> +
>  /*
>   * Subsystem for storing human-readable names for each object.
>   *
> diff --git a/object-file.c b/object-file.c
> index 065103be3e..bc63b80c48 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2470,11 +2470,13 @@ int repo_has_object_file(struct repository *r,
>   * give more context.
>   */
>  static int hash_format_check_report(struct fsck_options *opts UNUSED,
> -				     const struct object_id *oid UNUSED,
> -				     enum object_type object_type UNUSED,
> -				     enum fsck_msg_type msg_type UNUSED,
> -				     enum fsck_msg_id msg_id UNUSED,
> -				     const char *message)
> +				    const struct object_id *oid UNUSED,
> +				    enum object_type object_type UNUSED,
> +				    const char *ref_checkee UNUSED,
> +				    const char *sub_ref_checkee UNUSED,
> +				    enum fsck_msg_type msg_type UNUSED,
> +				    enum fsck_msg_id msg_id UNUSED,
> +				    const char *message)
>  {
>  	error(_("object fails fsck: %s"), message);
>  	return 1;
> --
> 2.45.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]