On Tue, Jul 02, 2024 at 10:33:36AM +0000, Karthik Nayak wrote:
> Hello,
>
> shejialuo <shejialuo@xxxxxxxxx> writes:
>
> > Hi All:
> >
> > This version follows the Junio's advice. Instead of creating the
> > following data structure:
> >
> > struct fsck_options {
> > enum fsck_type {
> > FSCK_OBJECTS,
> > FSCK_REFS,
> > ...
> > } t;
> > union {
> > struct fsck_objects_options objects;
> > struct fsck_refs_options refs;
> > } u;
> > };
> >
> > I simply use the combination idea where "fsck_options" will incorporate
> > "fsck_objects_options" and "fsck_refs_options". Karthik has told me that
> > I should balance the job I should does and the extensibility for future.
> > So I use the most clear way to do this. Also Junio has said:
> >
>
> If I understood Junio's comments correctly, he was drawing out the point
> about if we even need the separation of options for refs. Since the only
> option we're adding is a verbose:
>
> struct fsck_refs_options {
> unsigned verbose:1;
> };
>
> wouldn't it be better if we simply amended `fsck_options` as so:
>
> diff --git a/fsck.h b/fsck.h
> index 6085a384f6..ea97f48acc 100644
> --- a/fsck.h
> +++ b/fsck.h
> @@ -135,6 +135,7 @@ struct fsck_options {
> fsck_walk_func walk;
> fsck_error error_func;
> unsigned strict:1;
> + unsigned verbose_refs:1;
> enum fsck_msg_type *msg_type;
> struct oidset skiplist;
> struct oidset gitmodules_found;
>
> Your approach seems to take a different path though, where we create a
> new route of creating two new structs, one for refs and another for
> objects and adding both to fsck_objects. If we're doing this, wouldn't
> it be better to use the enum+union idea, like Junio mentioned? That way
> we would have clarity around which type it represents.
>
I agree. Let's give up breaking the structs. I will send a new version
immediately.
Thanks.
> [snip]
