On Thu, Jun 27, 2024 at 02:32:44PM -0700, Junio C Hamano wrote: > shejialuo <shejialuo@xxxxxxxxx> 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. > 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. > 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.