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

From: Taylor Blau <[email-masked]>
Date: Mon, 3 Aug 2020 19:08:14 -0400

On Mon, Aug 03, 2020 at 04:01:34PM -0700, Jonathan Nieder wrote:
> Taylor Blau wrote:
> > 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:
>
> >>>>> 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.
>
> Sorry for the lack of clarity.  The proposal Stolee is pushing back on
> above is to have a helper in test-lib-functions.sh that tells a test
> whether an event it cares about happened in traces (in this example,
> invocation of "git gc").

Serves me right for reading too quickly ;).

> I consider it important because if we are getting into the habit of
> having our tests assume the current exact byte-for-byte trace JSON
> output, then that is an assumption that is going to be painful to
> unravel.  Factoring out a helper would also make the test easier to
> read, but that is less important than "an ounce of prevention is worth
> a pound of cure" to me in this example.

I still only partially buy into this, though. I had to deal with some
rather fragile grepping through trace2 JSON in t4216 (? or something,
the log-bloom tests) recently, and found it a little fragile, but not
overly so.

I'd rather just move forward, but I do feel strongly that we discuss
these matters on the list.

> Jonathan

Thanks,
Taylor