Re: [PATCH v2 04/18] maintenance: initialize task array

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 25, 2020, 01:46 UTC
Message-ID: <20200725014647.GC35171@syl.lan>
In-Reply-To: <67e49b30-9a7f-8c18-3a06-96c516027c7a@gmail.com>

On Fri, Jul 24, 2020 at 08:51:32AM -0400, Derrick Stolee wrote:

Show 66 quoted lines

> On 7/24/2020 8:23 AM, Derrick Stolee wrote:
> > On 7/23/2020 3:57 PM, Junio C Hamano wrote:
> >> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >>
> >>> +static void initialize_tasks(void)
> >>> +{
> >>> +	int i;
> >>> +	num_tasks = 0;
> >>> +
> >>> +	for (i = 0; i < MAX_NUM_TASKS; i++)
> >>> +		tasks[i] = xcalloc(1, sizeof(struct maintenance_task));
> >>> +
> >>> +	tasks[num_tasks]->name = "gc";
> >>> +	tasks[num_tasks]->fn = maintenance_task_gc;
> >>> +	tasks[num_tasks]->enabled = 1;
> >>> +	num_tasks++;
> >>
> >> Are we going to have 47 different tasks initialized by code like
> >> this in the future?  I would have expected that you'd have a table
> >> of tasks that serves as the blueprint copy and copy it to the table
> >> to be used if there is some need to mutate the table-to-be-used.
> >
> > Making it a table will likely make it easier to read. I hadn't
> > thought of it.
> >
> > At the start, I thought that the diff would look awful as we add
> > members to the struct. However, the members that are not specified
> > are set to zero, so I should be able to craft this into something
> > not too terrible.
>
> OK, my attempt has led to this final table:
>
> 	const struct maintenance_task default_tasks[] = {
> 		{
> 			"prefetch",
> 			maintenance_task_prefetch,
> 		},
> 		{
> 			"loose-objects",
> 			maintenance_task_loose_objects,
> 			loose_object_auto_condition,
> 		},
> 		{
> 			"incremental-repack",
> 			maintenance_task_incremental_repack,
> 			incremental_repack_auto_condition,
> 		},
> 		{
> 			"gc",
> 			maintenance_task_gc,
> 			need_to_gc,
> 			1,
> 		},
> 		{
> 			"commit-graph",
> 			maintenance_task_commit_graph,
> 			should_write_commit_graph,
> 		}
> 	};
> 	num_tasks = sizeof(default_tasks) / sizeof(struct maintenance_task);
>
> This is followed by allocating and copying the data to the
> 'tasks' array, allowing it to be sorted and modified according
> to command-line arguments and config.
>
> Is this what you intended?

I'm not sure if Junio intended what I'm going to suggest, but I think that you could make looking up these "blueprint" tasks a little easier by using the designated index initializer. For what it's worth, I wasn't sure if we allow this in the codebase, but some quick perusing through Documentation/CodingGuidelines turns up 512f41cfac (clean.c: use designated initializer, 2017-07-14), which does use this style.

Maybe something like:

  enum maintenance_task_kind {
    TASK_PREFETCH = 0,
    TASK_LOOSE_OBJECTS,
    /* ... */
    TASK__COUNT
  };

  const struct maintenance_task default_tasks[TASK__COUNT] = {
    [TASK_PREFETCH] = {
      "prefetch",
      maintenance_task_prefetch,
    },
    [...] = ...
  };

and then you should be able to pick those out with 'default_tasks[TASK_PREFETCH]'. I'm not sure if you are able to rely on those tasks appearing in a certain order in which case you can feel free to discard this suggestion.

If nothing else, I'm glad that we can use the '[...] = '-style initializers :-).

> Thanks,
> -Stolee

Thanks, Taylor