Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 20, 2021, 21:47 UTC
Message-ID: <YPdEeBGi3RVrB/fu@nand.local>
In-Reply-To: <87lf60vfyn.fsf@evledraar.gmail.com>

On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 14 quoted lines

> > So we should definitely fix this instance via a reroll of this series,
> > but that still leaves the others up for grabs. Ævar, are those (the ones
> > in the 'multi-pack-index' and 'env--helper' builtins) something that you
> > want to clean up while you're working in this area, or would you rather
> > that I take care of it?
> >
> > I don't mind either way, just want to make sure that we don't duplicate
> > effort.
>
> I'm all for you picking it up :)
>
> If you wanted to pick up these patches (or some of them) and
> partially/entirely replace this series I'd be happy with that too,
> i.e. if it makes conflicts etc. easier.

I think either is fine; there shouldn't be any conflicts in the multi-pack-index code just eyeballing based on what you wrote.

I started working on it (which I suppose can count for me volunteering to patch it up myself ;-)), but I wondered why we have env--helper at all. When you wrote it back in b4f207f339 (env--helper: new undocumented builtin wrapping git_env_*(), 2019-06-21), you said that it wasn't added as a test-tool because it had some uses outside of tests.

But I can't find any locations. We do use env--helper (via test_bool_env, which you also introduced) in a couple of t/lib-*.sh files, but this would be far from the first test-tool that has been used in a test library.

So ISTM that this could be converted to a test-tool and removed from the list of builtins. *And* we could do that without a deprecation warning, because it was never documented in the first place.

Can you double check my thinking and/or let me know if there is a compelling reason to keep it as a builtin?

Show 8 quoted lines

> I just re-submitted this now because it's been staring at me in my
> "should re-roll at somep point" list for a while...
>
> FWIW if you're poking at this area more generally we really could do
> with some standardization around these built-in sub-commands:
>
>     git built-in --here subcommand
>     git built-in subcommand --or-here

That's probably a step too far for this loose end for me, so if you want to work on that please be my guest, but I probably don't have time for it in the near future.

Thanks, Taylor