On 7/30/2020 8:30 PM, Jonathan Nieder wrote:
> 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?
I mean that there is only one task right now. Until the commit-graph
task is implemented, there is no need to have a --task=<task> option.
Creating a --task=<task> option in this patch would unnecessarily bloat
the patch by implementing multiple tasks while filling in the boilerplate
of creating a new builtin.
>>> 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?
Which part do you want to be idiomatic? The fact that the parse-options
library typically refers to static global values or the fact that the
data is not organized in a struct?
Turning these into locals is _even worse_ than the current model of
static globals because that then requires updating the prototypes of
all the (static) local methods that operate on the arguments. Every
time a new option is added, that would cascade in method prototypes
all over again.
The natural thing to do to "fix" that situation is to create a struct
that holds the information from the parsed command-line arguments. But
then why is it a local parameter that is passed through all of the
local methods instead of a global (as presented here in this patch)?
> [...]
>>> 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.
This is a valuable suggestion, but this series is already too large
to make such a change in addition to the patches already here. I'd
consider this more if there were more tests that checked "did we
run this subcommand with these arguments?" Better to extract a helper
when this is actually used in multiple places.
Thanks,
-Stolee
