Re: [PATCH v2 06/18] maintenance: add --task option

[Junio C Hamano <gitster@pobox.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 35b0be7d40..9204762e21 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -73,6 +73,10 @@ OPTIONS
>  --quiet::
>  	Do not report progress or other information over `stderr`.
>  
> +--task=<task>::
> +	If this option is specified one or more times, then only run the
> +	specified tasks in the specified order.
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 2cd17398ec..c58dea6fa5 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -710,6 +710,7 @@ static const char * const builtin_maintenance_usage[] = {
>  static struct maintenance_opts {
>  	int auto_flag;
>  	int quiet;
> +	int tasks_selected;
>  } opts;
>  
>  static int run_write_commit_graph(void)
> @@ -804,20 +805,38 @@ typedef int maintenance_task_fn(void);
>  struct maintenance_task {
>  	const char *name;
>  	maintenance_task_fn *fn;
> -	unsigned enabled:1;
> +	int task_order;
> +	unsigned enabled:1,
> +		 selected:1;
>  };
>  
>  static struct maintenance_task *tasks[MAX_NUM_TASKS];
>  static int num_tasks;
>  
> +static int compare_tasks_by_selection(const void *a_, const void *b_)
> +{
> +	const struct maintenance_task *a, *b;
> +	a = (const struct maintenance_task *)a_;
> +	b = (const struct maintenance_task *)b_;
> +
> +	return b->task_order - a->task_order;
> +}

It forces the reader to know intimately that task_order *is*
selection order in order to understand why this is "tasks by
selection".  Perhaps rename the field to match what it is
(i.e. "selection_order")?

>  static int maintenance_run(void)
>  {
>  	int i;
>  	int result = 0;
>  
> +	if (opts.tasks_selected)
> +		QSORT(tasks, num_tasks, compare_tasks_by_selection);
> +
>  	for (i = 0; !result && i < num_tasks; i++) {
> -		if (!tasks[i]->enabled)
> +		if (opts.tasks_selected && !tasks[i]->selected)
> +			continue;
> +
> +		if (!opts.tasks_selected && !tasks[i]->enabled)
>  			continue;

I am not sure about this.  Even if the task <x> is disabled, if the
user says --task=<x>, it is run anyway?  Doesn't make an immediate
sense to me.

As I am bad at deciphering de Morgan, I'd find it easier to read if
the first were written more like so:

		if (!(!opts.tasks_selected || tasks[i]->selected))
			continue;

That is, "do not skip any when not limited, and do not skip the ones
that are selected when limited".  And that would easily extend to

		if (!tasks[i]->enabled ||
		    !(!opts.tasks_selected || tasks[i]->selected))
			continue;
> +
>  		result = tasks[i]->fn();
>  	}

> @@ -842,6 +861,44 @@ static void initialize_tasks(void)
>  	num_tasks++;
>  }
>  
> +static int task_option_parse(const struct option *opt,
> +			     const char *arg, int unset)
> +{
> +	int i;
> +	struct maintenance_task *task = NULL;
> +
> +	BUG_ON_OPT_NEG(unset);
> +
> +	if (!arg || !strlen(arg)) {
> +		error(_("--task requires a value"));
> +		return 1;

There is no need to special case an empty string that was explicitly
given as the value---it will be caught as "'' is not a valid task".

> +	}
> +
> +	opts.tasks_selected++;
> +
> +	for (i = 0; i < MAX_NUM_TASKS; i++) {
> +		if (tasks[i] && !strcasecmp(tasks[i]->name, arg)) {
> +			task = tasks[i];
> +			break;
> +		}
> +	}
> +
> +	if (!task) {
> +		error(_("'%s' is not a valid task"), arg);
> +		return 1;
> +	}
> +
> +	if (task->selected) {
> +		error(_("task '%s' cannot be selected multiple times"), arg);
> +		return 1;
> +	}
> +
> +	task->selected = 1;
> +	task->task_order = opts.tasks_selected;
> +
> +	return 0;
> +}