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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

Thanks and hope that helps,
Jonathan