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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2020, 00:36 UTC
Message-ID: <xmqq364jwjxg.fsf@gitster.c.googlers.com>
In-Reply-To: <20200819000015.2838345-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 24 quoted lines

>> @@ -66,6 +68,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.
>
> We should list the accepted tasks somewhere but maybe this can wait
> until after part 2.
>
>> @@ -791,7 +791,9 @@ typedef int maintenance_task_fn(struct maintenance_opts *opts);
>>  struct maintenance_task {
>>  	const char *name;
>>  	maintenance_task_fn *fn;
>> -	unsigned enabled:1;
>> +	unsigned enabled:1,
>> +		 selected:1;
>> +	int selected_order;
>>  };
>
> "selected" and "selected_order" are redundant in some cases - I think
> this would be better if selected_order is negative if this task is not
> selected, and non-negative otherwise.

It is good to get rid of redundancies.

> Apart from that, maybe this should be documented. It is unusual (to me)
> that a selection can override something being enabled, but that is the
> case here.

I had the same reaction as "(to me)" above during an earlier review round, IIRC. This definitely deserves documentation.