Re: [PATCH v2 01/18] maintenance: create basic maintenance runner

> Derrick Stolee wrote:
>> On 7/29/2020 6:19 PM, Jonathan Nieder wrote:
> 
>>> [jrnieder] How do I supply the tasks on the command line?  Are they
>>> parameters to this subcommand?  If so, it could make sense for this to
>>> say something like
>>>
>>> 	run <task>...::
>>
>> Hopefully this is documented to your satisfaction when the ability
>> to customize the tasks is implemented.
> 
> Hm, do you mean that this change is too difficult to squash into the
> first patch?

>>> What is the exit code convention for "git maintenance run"?  (Many Git
>>> commands don't document this, but since we're making a new command
>>> it seems worth building the habit.)
>>
>> Is this worth doing? Do we really want every command to document
>> that "0 means success, everything else means failure, and some of
>> those exit codes mean a specific kind of failure that is global to
>> Git"?
> 
> I mean things like "what exit code does it run if there is no
> maintenance to do?"
> 
> [...]
>>>> +static struct maintenance_opts {
>>>> +	int auto_flag;
>>>> +} opts;
>>>
>>> Packing this in a struct feels a bit unusual.  Is the struct going to
>>> be passed somewhere, or could these be individual locals in
>>> cmd_maintenance?
>>
>> This will grow, and I'd rather have one global struct than many
>> individual global items. It makes it clearer when I use
>> "opts.auto_flag" that this corresponds to whether "--auto" was
>> provided as a command-line option.
> 
> That doesn't seem idiomatic within the Git codebase.
>
> Can they be locals instead?

> [...]
>>> Perhaps this is suggesting that we need some central test helper for
>>> parsing traces so we can do this reliably in one place.  What do you
>>> think?
>>
>> I'm specifically using GIT_TRACE2_EVENT, which is intended for
>> consumption by computer, not humans. Adding whitespace or otherwise
>> changing the output format would be an unnecessary disruption that
>> is more likely to have downstream effects with external tools.
>>
>> In some way, adding extra dependencies like this in our own test
>> suite can help ensure the output format is more stable.
> 
> Hm, I thought the contract for GIT_TRACE2_EVENT is that it is JSON,
> which is a well defined format.  To that end, I would sincerely hope
> that external tools are not assuming anything beyond that the format
> is JSON.
> 
> If that wasn't the intent, I'd rather that we not use a JSON-like
> format at all.  That would make the expectations for evolving this
> interface clearer.
> 
>> If there is a better way to ask "Did my command call 'git gc' (with
>> no arguments|with these arguments)?" then I'm happy to consider it.
> 
> My proposal was just to factor this out into a function in
> test-lib-functions.sh so it's easy to evolve over time in one place.