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

On Tue, Jul 20 2021, Taylor Blau wrote:

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

quoted

quoted

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

FWIW test_bool_env is SZEDER'S, see 43a2afee82a (tests: add
'test_bool_env' to catch non-bool GIT_TEST_* values, 2019-11-22).

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?

Unlike the test-tools it was used by installed git, namely
git-sh-i18n.sh before d162b25f956 (tests: remove support for
GIT_TEST_GETTEXT_POISON, 2021-01-20), but yes, now it could just be
migrated to a test-tool.

I suppose it never really *needed* to be a built-in, i.e. the
git-sh-i18n.sh code could have just died without the helper if
GIT_TEST_GETTEXT_POISON was set, anyway...

...yes, nowadays it can simply be moved to t/helper.

<digression>

I do think in general this recent proliferation of t/helper over new
plumbing built-ins has sent git a bit in the wrong direction.

E.g. I think the likes of t/helper/test-pkt-line.c should really be a
plumbing tool, the same goes for many (but not all) the test tool, we
could just document them as being "unstable plumbing" or whatever.

But I think I've been losing that argument recently, e.g. after [1]
(which I argued we should put into git-ls-files) even things like git's
basic idea of the state of the index are exposed in some helpers, but
not corresponding plumbing.

Anyway, even if we assume that's an argument that would carry the day in
general I'd find it hard to justify git-env--helper being a thing that
should be exposed to users or post-"make install", it's purely for the
use of our own test suite.

</digression>


So yeah, it can just be moved to t/helper if you want to do that.

quoted

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.

*nod*, but just to be clear, were you going to pick up &
re-roll/re-submit the patches I have in this series? That would be good
or me, but I don't mind either way, just wondering what state this
should be on my TODO list.

1. https://lore.kernel.org/git/a1b8135c0fc890b2c2c0271c923b2f8efa8d1465.1617109864.git.gitgitgadget@gmail.com/ (local)