On 7/23/2020 4:21 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>> +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")?
Good idea. I made this fix locally.
>> 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.
You already replied that you figured this out. However, I could make
it easier by adding some foreshadowing in the commit message here.
> 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;
This isn't quite right, due to the confusing nature of "enabled".
The condition here will _never_ allow selecting a disabled task.
Perhaps it would be better to rename 'enabled' to 'run_by_default'?
That would make it clear that it is choosing which tasks to run unless
specified otherwise with --task=<task> here, the config option
maintenance.<task>.enabled later, and the --auto conditions even later.
Looking even farther down the line (into the next series) there will be
similar checks for auto-conditionschecking time-based schedules.
Since this loop becomes more complicated in the future, I specifically
wanted to group the "skip this task" conditions into their own if
blocks:
1. If the user didn't specify --task=<task> explicitly and this
task is disabled, then skip this task.
2. If the user _did_ specify --task=<task> explicitly and this
task was not on the list, then skip this task.
3. If the user specified --auto and the auto condition fails,
then skip this task.
4. (Later) If the user specified --scheduled and the time since
the last run is too soon, then skip this task.
With this being the planned future, I'd prefer these be split out as
separate if conditions instead of a giant combined if. And since that
is the plan, then I won't work too hard to combine conditions 1 and 2
into a single condition.
>> +
>> 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".
Sounds good. No need for this extra message.
Thanks,
-Stolee
