Re: [GSoC][PATCH v5 02/12] fsck: use "fsck_configs" to set up configs

From: shejialuo <shejialuo@gmail.com>
Date: Jun 28, 2024, 04:22 UTC
Message-ID: <Zn46hGUqz1ZtUpGz@ArchLinux>
In-Reply-To: <xmqqzfr6qcx8.fsf@gitster.g>

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

Show 11 quoted lines

> shejialuo <shejialuo@gmail.com> writes:
> 
> > Some fields such as "msg_type" and "skiplist" in "fsck_objects_options"
> > are not options, these fields are related to "git-config(1)" which are
> > initialized using "git_fsck_config" function.  Create a static variable
> > named "fsck_configs" in "fsck.c" which aims at handling configs. Thus we
> > don't need to reply on the "fsck_objects_options" to set up the fsck
> > error message severity.
> 
> reply???
>

Sorry, I often make mistake to type "rely" as "reply".

Show 12 quoted lines

> As configuration often is used to prepopulate options, I need a lot
> stonger justification to split these into a different structure than
> "'config' is a noun that is different from a noun 'option'".  
> 
> If we intend to have many "option" instances but what these two
> members store will be the same across these "option" instances, for
> example, that would be a lot better reason why we may want to
> separate these two members out of it, but I have a suspicion that if
> we were to use the "union with tags" approach, these two would
> become members of the common part shared between "objects' and
> "refs", i.e. the overall data structure might look more like this:
>

Actually, I feel really wired for this part. Let me elaborate on this. "fsck.c::git_fsck_config()" is used to set up the configs. It will eventually call the "fsck.c::fsck_set_msg_type_from_ids" like the following:

  void fsck_set_msg_type_from_ids(struct fsck_options *options,
                                  enum fsck_msg_id msg_id,
                                  enum fsck_msg_type msg_type)
  {
    if (!options->msg_type) {
      int i;
      enum fsck_msg_type *severity.
      ALLOC_ARRAY(severity, FSCK_MSG_MAX);
      for (i = 0; i < FSCK_MSG_MAX; i++)
        severity[i] = fsck_msg_type(i, options);
      options->msg_type = severity;
    }

   options->msg_type[msg_id] = msg_type;
  }

In the current codebase, the caller will simply create a "fsck_options" and setup the fsck error message severity. However, let's see "builtin/fskc.c", it creates the following two "fsck_options" and it only uses

  static struct fsck_options fsck_walk_options = FSCK_OPTIONS_DEFAULT;
  static struct fsck_options fsck_obj_options = FSCK_OPTIONS_DEFAULT;

However, the code only uses "fsck_obj_options" to setup the configs. So it makes me feel so strange. So I just want to make it separation. Maybe a little wrong here.

Show 17 quoted lines

> 	struct fsck_options {
> 		enum fsck_msg_type *msg_type;
> 		struct oidset oid_skiplist;
> 		enum fsck_what_check { O, R } tag;
> 		union {
> 			struct fsck_object_options o;
> 			struct fsck_ref_options r;
> 		} u;
> 	};
> 
> by moving these two members out of fsck_object_options and moving
> them to the shared part.
> 
> I dunno.  It is unclear what the real reason is for these two things
> to be extracted out and made to appear totally independent from the
> "options" thing to begin with, and I am not sure if I agree with the
> reason when it is known.