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

From: Taylor Blau <me@ttaylorr.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Derrick Stolee <stolee@gmail.com>,
	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 18:46:31 -0400	[thread overview]
Message-ID: <20200803224631.GA73022@syl.lan> (raw)
In-Reply-To: <20200803174654.GA2473576@google.com>

On Mon, Aug 03, 2020 at 10:46:54AM -0700, Jonathan Nieder wrote:
> 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.

I think what Stolee is trying to say is less about a change that would
make the series larger, it's about changing an already-large series.

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

I'm not sure that I see your argument. What we are really discussing is
whether or not we should have a static struct outside of 'cmd_gc()', or
a zero-initialized frame local struct within 'cmd_gc()'. I fully
understand the arguments in favor of one or the other, but I struggle to
grasp that this is worth our time to debate in this much detail.

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

I assume that the part of this document you were linking to was about
meeting out-of-band to discuss an issue instead of emailing back and
forth.

I appreciate your willingness to resolve this expediently, but I would
much prefer if these discussions (that is, the ones about the technical
content of this series) happened on-list for the benefit of other
reviewers.

> Thanks,
> Jonathan

Thanks,
Taylor