From: shejialuo <shejialuo@gmail.com>
Date: Thu, 11 Jul 2024 11:59:39 GMT
Subject: Re: [GSoC][PATCH v10 03/10] fsck: add a unified interface for reporting fsck messages
Message-ID: <Zo_JKyCHrN_lJFAx@ArchLinux>
In-Reply-To: <xmqqv81dufg0.fsf@gitster.g>

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.