git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jul 21, 2021, 07:26 UTC
Message-ID
<87im14unfd.fsf@evledraar.gmail.com>
In-Reply-To
<YPdEeBGi3RVrB/fu@nand.local>
On Tue, Jul 20 2021, Taylor Blau wrote:
Show 27 quoted lines
> 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

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

Show 9 quoted lines
> 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.
Show 12 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.

*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/
Previous: Taylor BlauNext: Jeff King
Message 26 of 40 in “commit-graph: usage fixes”
  1. 0/5 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Jul 18, 2021
  2. 2/5 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Jul 18, 2021
  3. Andrei RybakJul 18, 2021
  4. Taylor BlauJul 19, 2021
  5. 1/5 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Jul 18, 2021
  6. Taylor BlauJul 19, 2021
  7. 3/5 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Jul 18, 2021
  8. Taylor BlauJul 19, 2021
  9. Ævar Arnfjörð BjarmasonJul 20, 2021
  10. 4/5 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Jul 18, 2021
  11. Taylor BlauJul 19, 2021
  12. 5/5 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Jul 18, 2021
  13. Taylor BlauJul 19, 2021
  14. 0/6 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Jul 20, 2021
  15. 1/6 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Jul 20, 2021
  16. 2/6 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Jul 20, 2021
  17. 3/6 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Jul 20, 2021
  18. 4/6 multi-pack-index: refactor "goto usage" patternÆvar Arnfjörð Bjarmason, Jul 20, 2021
  19. Taylor BlauJul 20, 2021
  20. 6/6 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Jul 20, 2021
  21. SZEDER GáborJul 20, 2021
  22. SZEDER GáborJul 20, 2021
  23. Taylor BlauJul 20, 2021
  24. Ævar Arnfjörð BjarmasonJul 20, 2021
  25. Taylor BlauJul 20, 2021
  26. Ævar Arnfjörð BjarmasonJul 21, 2021
  27. Jeff KingJul 21, 2021
  28. Junio C HamanoJul 21, 2021
  29. 5/6 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Jul 20, 2021
  30. Taylor BlauJul 20, 2021
  31. 0/7 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Aug 23, 2021
  32. 1/7 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Aug 23, 2021
  33. 2/7 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Aug 23, 2021
  34. 3/7 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Aug 23, 2021
  35. 4/7 multi-pack-index: refactor "goto usage" patternÆvar Arnfjörð Bjarmason, Aug 23, 2021
  36. 5/7 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Aug 23, 2021
  37. 6/7 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Aug 23, 2021
  38. 7/7 commit-graph: show "unexpected subcommand" errorÆvar Arnfjörð Bjarmason, Aug 23, 2021
  39. Taylor BlauAug 30, 2021
  40. Junio C HamanoAug 30, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.