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

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