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

From: Jonathan Nieder <jrnieder@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Johannes.Schindelin@gmx.de,
	sandals@crustytoothpaste.net, steadmon@google.com, peff@peff.net,
	congdanhqx@gmail.com, phillip.wood123@gmail.com,
	emilyshaffer@google.com, sluongng@gmail.com,
	jonathantanmy@google.com,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v2 01/18] maintenance: create basic maintenance runner
Date: Mon, 3 Aug 2020 10:46:54 -0700	[thread overview]
Message-ID: <20200803174654.GA2473576@google.com> (raw)
In-Reply-To: <a176ddf5-b45b-fb25-8740-96efbd324edf@gmail.com>

Derrick Stolee wrote:
> 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.
[...]
> 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.

Ah, that wasn't clear to me from the docs.

You're saying that "git maintenance run" runs the "gc" task?  Can the
documentation say so?

[...]
>>>>> +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?

parse-options has no requirement about the values being global.  Some
older code does that, but newer code tends to use locals when
appropriate.

Putting it in a struct is fine as long as that struct is being passed
around.  What isn't idiomatic in Git is using a global struct for
namespacing.

[...]
> 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)?

I'm having trouble parsing that last sentence.  You're saying a global
is preferable over passing around a pointer to a local "opts" struct?

[...]
>>> 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.

Hm, it's not clear to me that this would make the series significantly
larger.

And on the contrary, it would make the code less fragile.  I think this
is important.

https://chromium.googlesource.com/chromium/src/+/master/docs/cl_respect.md#remember-communication-can-be-hard:
if you'd like to meet out of band, let me know, and I'd be happy to go
into this further.

Thanks,
Jonathan