diff mbox series

[GSoC,v10,03/10] fsck: add a unified interface for reporting fsck messages

Message ID Zo6e61kSAvuvBvBS@ArchLinux (mailing list archive)
State Superseded
Headers show
Series ref consistency check infra setup | expand

Commit Message

shejialuo July 10, 2024, 2:47 p.m. UTC 
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 "vfsck_report"
by adding "checked_ref_name" parameter following the "report" prototype.
Instead of using "...", provide "va_list" to allow more flexibility.

Like "report", the "vfsck_report" function will use "error_func"
registered in "fsck_options" to report customized messages. Change
"error_func" prototype to align with the new "vfsck_report".

Then, change "report" function to use "vfsck_report" to report objects
related messages. Add a new function called "fsck_refs_report" to use
"vfsck_report" to report refs related messages.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 builtin/fsck.c  |  1 +
 builtin/mktag.c |  1 +
 fsck.c          | 56 +++++++++++++++++++++++++++++++++++++++++--------
 fsck.h          | 17 ++++++++++++++-
 object-file.c   | 11 +++++-----
 5 files changed, 71 insertions(+), 15 deletions(-)

Comments

Junio C Hamano July 10, 2024, 9:04 p.m. UTC | #1 
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 "vfsck_report"
> by adding "checked_ref_name" parameter following the "report" prototype.
> Instead of using "...", provide "va_list" to allow more flexibility.

Like strbuf_vinsertf(), it is a good idea to have "v" in the name of
a function that takes va_list, but fsck_vreport() would probably be
a better name here.  Arguably, the original report() is misnamed (as
a printf-like function that takes format string, it probably would
have wanted to be reportf() instead), but unless we are fixing that
at the same time, calling this fsck_vreportf() would probably be too
much.  Consistently misnaming it by omitting the final "f" would be
fine.

At this step it is still not clear if the previous step was really
needed; you have this "v" thing that is designed to be usable by
both reporting issues around objects and issues around refs, but we
will hopefully see why when we read later patches.
> diff --git a/object-file.c b/object-file.c
> index 065103be3e..d2c6427935 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2470,11 +2470,12 @@ 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_checked_name UNUSED,
> +				    enum fsck_msg_type msg_type UNUSED,
> +				    enum fsck_msg_id msg_id UNUSED,
> +				    const char *message)

That is somewhat annoying reindentation.  What happened here?
shejialuo July 11, 2024, 11:59 a.m. UTC | #2 
On Wed, Jul 10, 2024 at 02:04:15PM -0700, Junio C Hamano wrote:
> 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 "vfsck_report"
> > by adding "checked_ref_name" parameter following the "report" prototype.
> > Instead of using "...", provide "va_list" to allow more flexibility.
> 
> Like strbuf_vinsertf(), it is a good idea to have "v" in the name of
> a function that takes va_list, but fsck_vreport() would probably be
> a better name here.  Arguably, the original report() is misnamed (as
> a printf-like function that takes format string, it probably would
> have wanted to be reportf() instead), but unless we are fixing that
> at the same time, calling this fsck_vreportf() would probably be too
> much.  Consistently misnaming it by omitting the final "f" would be
> fine.
> 

Yes，I will rename it to "fsck_vreport".

> At this step it is still not clear if the previous step was really
> needed; you have this "v" thing that is designed to be usable by
> both reporting issues around objects and issues around refs, but we
> will hopefully see why when we read later patches.

From my perspective, I think we should put the previous commit after
this commit. I agree with you that if we put it later, it will be
much clearer and eaiser to understand.

> > diff --git a/object-file.c b/object-file.c
> > index 065103be3e..d2c6427935 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -2470,11 +2470,12 @@ 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_checked_name UNUSED,
> > +				    enum fsck_msg_type msg_type UNUSED,
> > +				    enum fsck_msg_id msg_id UNUSED,
> > +				    const char *message)
> 
> That is somewhat annoying reindentation.  What happened here?

The original code's indentation breaks. There are one more space in each
line like the following:

  static int hash_format_check_report(struct fsck_options *opts UNUSED,
                                       const struct object_id *oid UNUSED)
                                       ...

I think I could fix this by the way.
diff mbox series

Patch

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 6d86bbe1e9..de34538c4f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -92,6 +92,7 @@  static int objerror(struct object *obj, const char *err)
 static int fsck_objects_error_func(struct fsck_options *o UNUSED,
 				   const struct object_id *oid,
 				   enum object_type object_type,
+				   const char *checked_ref_name UNUSED,
 				   enum fsck_msg_type msg_type,
 				   enum fsck_msg_id msg_id UNUSED,
 				   const char *message)
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 4767f1a97e..42f945c584 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -20,6 +20,7 @@  static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;
 static int mktag_fsck_error_func(struct fsck_options *o UNUSED,
 				 const struct object_id *oid UNUSED,
 				 enum object_type object_type UNUSED,
+				 const char *checked_ref_name UNUSED,
 				 enum fsck_msg_type msg_type,
 				 enum fsck_msg_id msg_id UNUSED,
 				 const char *message)
diff --git a/fsck.c b/fsck.c
index 0aaff7f635..e1819964e3 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 vfsck_report(struct fsck_options *options,
+			const struct object_id *oid,
+			enum object_type object_type,
+			const char *checked_ref_name,
+			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;
@@ -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, checked_ref_name,
 				     msg_type, msg_id, sb.buf);
 	strbuf_release(&sb);
 	va_end(ap);
@@ -260,6 +266,36 @@  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 = vfsck_report(options, oid, object_type, NULL,
+			      msg_id, fmt, ap);
+	va_end(ap);
+	return result;
+}
+
+
+
+int fsck_refs_report(struct fsck_options *options,
+		     const struct object_id *oid,
+		     const char *checked_ref_name,
+		     enum fsck_msg_id msg_id, const char *fmt, ...)
+{
+	va_list ap;
+	int result;
+	va_start(ap, fmt);
+	result = vfsck_report(options, oid, OBJ_NONE,
+			      checked_ref_name, msg_id, fmt, ap);
+	va_end(ap);
+	return result;
+}
+
 void fsck_enable_object_names(struct fsck_options *options)
 {
 	if (!options->object_names)
@@ -1203,6 +1239,7 @@  int fsck_buffer(const struct object_id *oid, enum object_type type,
 int fsck_objects_error_function(struct fsck_options *o,
 				const struct object_id *oid,
 				enum object_type object_type UNUSED,
+				const char *checked_ref_name UNUSED,
 				enum fsck_msg_type msg_type,
 				enum fsck_msg_id msg_id UNUSED,
 				const char *message)
@@ -1306,6 +1343,7 @@  int git_fsck_config(const char *var, const char *value,
 int fsck_objects_error_cb_print_missing_gitmodules(struct fsck_options *o,
 						   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)
@@ -1314,6 +1352,6 @@  int fsck_objects_error_cb_print_missing_gitmodules(struct fsck_options *o,
 		puts(oid_to_hex(oid));
 		return 0;
 	}
-	return fsck_objects_error_function(o, oid, object_type,
+	return fsck_objects_error_function(o, oid, object_type, checked_ref_name,
 					   msg_type, msg_id, message);
 }
diff --git a/fsck.h b/fsck.h
index 41ebebbb59..f88e5faa94 100644
--- a/fsck.h
+++ b/fsck.h
@@ -114,19 +114,24 @@  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 *checked_ref_name,
 			  enum fsck_msg_type msg_type, enum fsck_msg_id msg_id,
 			  const char *message);
 
 int fsck_objects_error_function(struct fsck_options *o,
 				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);
 int fsck_objects_error_cb_print_missing_gitmodules(struct fsck_options *o,
 						   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);
@@ -209,6 +214,16 @@  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, 5, 6)))
+int fsck_refs_report(struct fsck_options *options,
+		     const struct object_id *oid,
+		     const char *checked_ref_name,
+		     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..d2c6427935 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2470,11 +2470,12 @@  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_checked_name 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;