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

Derrick Stolee wrote:

On 7/30/2020 8:30 PM, Jonathan Nieder wrote:

quoted

Derrick Stolee wrote:

quoted

On 7/29/2020 6:19 PM, Jonathan Nieder wrote:

quoted

quoted

quoted

[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?

[...]

quoted

quoted

quoted

quoted

+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?

[...]

quoted

quoted

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