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

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 19, 2020, 15:09 UTC
Message-ID: <3a5328c0-15f0-a427-1f21-2c8e3ef8277a@gmail.com>
In-Reply-To: <xmqq364jwjxg.fsf@gitster.c.googlers.com>

On 8/18/2020 8:36 PM, Junio C Hamano wrote:

Show 12 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
>>> @@ -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.

It's hard to see from the patch-context, but there is a section titled "TASKS" that lists the 'gc' and 'commit-graph' tasks from the earlier patches. Would a reference to that section be helpful?

	See the TASKS section for the list of available tasks.

Show 22 quoted lines

>>> @@ -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.

Will do. Thanks.

-Stolee