Re: [GSoC][PATCH v6 00/11] ref consistency check infra setup

On Tue, Jul 02, 2024 at 10:33:36AM +0000, Karthik Nayak wrote:

Hello,

shejialuo [off-list ref] writes:

quoted

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]