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
