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

From: Taylor Blau <me@ttaylorr.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Andrei Rybak" <rybak.a.v@gmail.com>
Subject: Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
Date: Tue, 20 Jul 2021 17:47:36 -0400	[thread overview]
Message-ID: <YPdEeBGi3RVrB/fu@nand.local> (raw)
In-Reply-To: <87lf60vfyn.fsf@evledraar.gmail.com>

On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > 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?

> 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