Re: [GSoC][PATCH v5 01/12] fsck: rename "fsck_options" to "fsck_objects_options"

From: shejialuo <shejialuo@gmail.com>
Date: Jun 28, 2024, 03:43 UTC
Message-ID: <Zn4xeoqhCeBCSNvg@ArchLinux>
In-Reply-To: <xmqqa5j6rs03.fsf@gitster.g>

On Thu, Jun 27, 2024 at 02:32:44PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> shejialuo <shejialuo@gmail.com> writes:
> 
> > -static int fsck_error_func(struct fsck_options *o UNUSED,
> > +static int fsck_error_func(struct fsck_objects_options *o UNUSED,
> >  			   const struct object_id *oid,
> >  			   enum object_type object_type,
> >  			   enum fsck_msg_type msg_type,
> 
> It is curious that the addition/renaming of fsck_objects_options is
> presumably to allow fsck_${xyzzy}_options to be added for different
> $xyzzy (the first one being "refs"), and this function is only about
> fsck_objects_options.  What name would the corresponding error
> function, called by checkers that take fsck_${xyzzy}_options, be
> given?  fsck_${xyzzy}_error_func()?  Shouldn't this be then become
> fsck_objects_error_func() or something?
>

Yes, it should be definitely changed here. Will improve in the next version.

Show 6 quoted lines

> Having said that.
> 
> Do we really need such a parallel system between "objects" and other
> kinds of things being checked that you are introducing with this
> step?  What benefit are we getting from this additional complexity?
>

I am agree that the most simple way to handle for this series is add some ref-related new members. Thus, we can reuse existing code. However, it makes me feel so weird when implementing the code using this idea. For example,

  struct fsck_options {
    struct fsck_refs_options;
    ...
  }

When we create a new "fsck_options", it will be so misleading that the caller may think we will handle both refs and objects checks by using "fsck_options". So I just introduce this parallel system. When checking objects, caller should explicitly create "fsck_objects_options", when checking refs, caller should explicitly create "fsck_refs_options".

Because in semantics, we introduce a new check here. Combination means we will check the both. Although it is simple, but it will cause a lot of trouble in the future.

Show 22 quoted lines

> I would have expected that adding ref-related new members that
> object consistency checkers has no interest in to the fsck_options
> structure would be sufficient for the purpose of this series.  Or if
> we really wanted to prepare for more complex future, use of the
> "union of variants, switched with a tag" pattern to arrange the data
> this way:
> 
> 	struct fsck_options {
> 		enum fsck_type {
> 			FSCK_OBJECTS,
> 			FSCK_REFS,
> 			...
> 		} t;
> 		union {
> 			struct fsck_objects_options objects;
> 			struct fsck_refs_options refs;
> 		} u;
> 	};
> 
> would still allow functions like fsck_error_func(), and
> fsck_set_msg_types(), etc. to work on the common "fsck_options".
>

I agree that we could use this pattern, using union will make the semantics more clear.

> I dunno.