[v2,04/18] maintenance: initialize task array

Commit Message

Johannes Schindelin via GitGitGadget July 23, 2020, 5:56 p.m. UTC

From: Derrick Stolee <[e-mail]>

In anticipation of implementing multiple maintenance tasks inside the
'maintenance' builtin, use a list of structs to describe the work to be
done.

The struct maintenance_task stores the name of the task (as given by a
future command-line argument) along with a function pointer to its
implementation and a boolean for whether the step is enabled.

A list of pointers to these structs are initialized with the full list
of implemented tasks along with a default order. For now, this list only
contains the "gc" task. This task is also the only task enabled by
default.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/gc.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

Comments

Junio C Hamano July 23, 2020, 7:57 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> 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.

>  }
>  
>  int cmd_maintenance(int argc, const char **argv, const char *prefix)
> @@ -751,6 +787,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
>  				   builtin_maintenance_options);
>  
>  	opts.quiet = !isatty(2);
> +	initialize_tasks();
>  
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_maintenance_options,

Derrick Stolee July 24, 2020, 12:23 p.m. UTC | #2

On 7/23/2020 3:57 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[e-mail]> 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.

Thanks,
-Stolee

Derrick Stolee July 24, 2020, 12:51 p.m. UTC | #3

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" <[e-mail]> 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?

Thanks,
-Stolee

Junio C Hamano July 24, 2020, 7:39 p.m. UTC | #4

Derrick Stolee <[e-mail]> writes:

> OK, my attempt has led to this final table:
>
> 	const struct maintenance_task default_tasks[] = {
> 		{
> 			"prefetch",
> 			maintenance_task_prefetch,
> 		},
>...
> 		{
> 			"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 do not know how important it is for your overall design to keep
the blueprint/master-copy table that is separate from the working
copy of the table that gets sorted, enabled/chosen bit set, etc.
IIUC, you were modifying the entries' fields at runtime, so perhaps
a pristine copy is not all that important (in which case you can
just lose "const" and do without extra copying)?  I dunno.

Taylor Blau July 25, 2020, 1:46 a.m. UTC | #5

On Fri, Jul 24, 2020 at 08:51:32AM -0400, Derrick Stolee wrote:
> 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" <[e-mail]> 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

Emily Shaffer July 29, 2020, 10:19 p.m. UTC | #6

On Thu, Jul 23, 2020 at 05:56:26PM +0000, Derrick Stolee via GitGitGadget wrote:
> +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));

[jonathan tan] I wonder why this is an array of pointers, not of
objects? If they were objects, we could use an initializer. But in a
later commit I see the array is sorted, OK.
> +
> +	tasks[num_tasks]->name = "gc";
> +	tasks[num_tasks]->fn = maintenance_task_gc;
> +	tasks[num_tasks]->enabled = 1;
> +	num_tasks++;

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index c8cde28436..c28fb0b16d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -700,6 +700,8 @@  int cmd_gc(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+#define MAX_NUM_TASKS 1
+
 static const char * const builtin_maintenance_usage[] = {
 	N_("git maintenance run [<options>]"),
 	NULL
@@ -729,9 +731,43 @@  static int maintenance_task_gc(void)
 	return result;
 }
 
+typedef int maintenance_task_fn(void);
+
+struct maintenance_task {
+	const char *name;
+	maintenance_task_fn *fn;
+	unsigned enabled:1;
+};
+
+static struct maintenance_task *tasks[MAX_NUM_TASKS];
+static int num_tasks;
+
 static int maintenance_run(void)
 {
-	return maintenance_task_gc();
+	int i;
+	int result = 0;
+
+	for (i = 0; !result && i < num_tasks; i++) {
+		if (!tasks[i]->enabled)
+			continue;
+		result = tasks[i]->fn();
+	}
+
+	return result;
+}
+
+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++;
 }
 
 int cmd_maintenance(int argc, const char **argv, const char *prefix)
@@ -751,6 +787,7 @@  int cmd_maintenance(int argc, const char **argv, const char *prefix)
 				   builtin_maintenance_options);
 
 	opts.quiet = !isatty(2);
+	initialize_tasks();
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_options,