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

Re: [PATCH v3 07/10] t: prepare for GIT_TEST_WRITE_REV_INDEX

From
Taylor Blau <me@ttaylorr.com>
Date
Jan 29, 2021, 01:21 UTC
Message-ID
<YBNjLqOe1NtpziV7@nand.local>
In-Reply-To
<YBNasXXZc1BEkMxH@coredump.intra.peff.net>
On Thu, Jan 28, 2021 at 07:45:37PM -0500, Jeff King wrote:
Show 37 quoted lines
> On Mon, Jan 25, 2021 at 06:37:38PM -0500, Taylor Blau wrote:
>
> > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> > index 297de502a9..2fc3aadbd1 100755
> > --- a/t/t5319-multi-pack-index.sh
> > +++ b/t/t5319-multi-pack-index.sh
> > @@ -710,8 +710,9 @@ test_expect_success 'expire respects .keep files' '
> >  		PACKA=$(ls .git/objects/pack/a-pack*\.pack | sed s/\.pack\$//) &&
> >  		touch $PACKA.keep &&
> >  		git multi-pack-index expire &&
> > -		ls -S .git/objects/pack/a-pack* | grep $PACKA >a-pack-files &&
> > -		test_line_count = 3 a-pack-files &&
> > +		test_path_is_file $PACKA.idx &&
> > +		test_path_is_file $PACKA.keep &&
> > +		test_path_is_file $PACKA.pack &&
>
> I find the post-image here more readable than the original. It probably
> runs faster, too (zero processes instead of three).
>
> > --- a/t/t5325-reverse-index.sh
> > +++ b/t/t5325-reverse-index.sh
> > @@ -3,6 +3,10 @@
> >  test_description='on-disk reverse index'
> >  . ./test-lib.sh
> >
> > +# The below tests want control over the 'pack.writeReverseIndex' setting
> > +# themselves to assert various combinations of it with other options.
> > +sane_unset GIT_TEST_WRITE_REV_INDEX
>
> I think we had a discussion a while ago about sane_unset outside of an
> &&-chain, where it does nothing that regular "unset" would not. But I
> don't know if we reached any kind of conclusion. I actually argued that
> it was fine in:
>
>   https://lore.kernel.org/git/20200630185536.GA1888406@coredump.intra.peff.net/
>
> So I guess I should probably stand by that. ;)

I think I probably took this from the trace2 tests? Not sure. I'm glad that it's not wrong, strictly speaking.

This is another instance that I'd be happy to send a follow-up patch to get rid of all of these at once, unless you feel strongly that it should be changed in this series before applying.

Show 11 quoted lines
> > --- a/t/t5604-clone-reference.sh
> > +++ b/t/t5604-clone-reference.sh
> > @@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje
> >  	for raw in $(ls T*.raw)
> >  	do
> >  		sed -e "s!/../!/Y/!; s![0-9a-f]\{38,\}!Z!" -e "/commit-graph/d" \
> > -		    -e "/multi-pack-index/d" <$raw >$raw.de-sha-1 &&
> > +		    -e "/multi-pack-index/d" -e "/rev/d" <$raw >$raw.de-sha-1 &&
>
> This one is less readable than before. :) I'm not sure how to really
> improve that, though.
Yeah, this follows from a suggestion that Ævar gave in:
  https://lore.kernel.org/git/878s8y5bdn.fsf@evledraar.gmail.com/

FWIW, I found that his suggestion was *clearer* than what was in v2. But I get that others may feel differently, too.

Show 21 quoted lines
> > --- a/t/t5702-protocol-v2.sh
> > +++ b/t/t5702-protocol-v2.sh
> > @@ -851,8 +851,10 @@ test_expect_success 'part of packfile response provided as URI' '
> >  	test -f h2found &&
> >
> >  	# Ensure that there are exactly 6 files (3 .pack and 3 .idx).
> > -	ls http_child/.git/objects/pack/* >filelist &&
> > -	test_line_count = 6 filelist
> > +	ls http_child/.git/objects/pack/*.pack >packlist &&
> > +	ls http_child/.git/objects/pack/*.idx >idxlist &&
> > +	test_line_count = 3 idxlist &&
> > +	test_line_count = 3 packlist
> >  '
>
> Hmm. Too bad we can't rely on shell brace expansion, as:
>
>   ls http_child/.git/objects/pack/*.{pack,idx}
>
> would be more readable. You could still do it in a single "ls" by
> writing out both arguments manually, but it's probably not that
> important.
Agreed on all of that.
Show 9 quoted lines
> This one is making the test a bit looser (it would miss a case where we
> somehow failed to generate the .idx). That seems like an unlikely bug,
> but I wonder if we can keep the original behavior. I guess:
>
>   ls .git/objects/pack/*.pack \
>      .git/objects/pack/*.idx |
>      sort >post_packs
>
> would work?

Sure, I see what you're saying. To be honest, I'm skeptical that we'd have a bug which failed only this one test, so I'm hesitant to send a replacement/reroll for this alone.

If you feel strongly, though, I'm happy to change it. (But, I'll err on the side of leaving it as-is, since you indicated in your response to v3's cover letter that you'd be OK if I discarded some or all of your suggestions).

Show 18 quoted lines
> >  test_expect_success 'gc --no-quiet' '
> > diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> > index 3d17e932a0..8f1caf8025 100755
> > --- a/t/t9300-fast-import.sh
> > +++ b/t/t9300-fast-import.sh
> > @@ -1632,7 +1632,10 @@ test_expect_success 'O: blank lines not necessary after other commands' '
> >  	INPUT_END
> >
> >  	git fast-import <input &&
> > -	test 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) &&
> > +	ls -la .git/objects/pack/pack-*.pack >packlist &&
> > +	ls -la .git/objects/pack/pack-*.pack >idxlist &&
> > +	test_line_count = 4 idxlist &&
> > +	test_line_count = 4 packlist &&
>
> Another case where I don't think we're losing anything (actually, we are
> gaining just slightly; if a bug somehow created 6 packs and 2 idx files,
> we'd now notice!).
Yay!
> -Peff

Thanks, Taylor

Previous: Eric SunshineNext: Jeff King
Message 57 of 82 in “pack-revindex: introduce on-disk '.rev' format”
  1. 0/8 pack-revindex: introduce on-disk '.rev' formatTaylor Blau, Jan 8, 2021
  2. 2/8 pack-write.c: prepare to write 'pack-*.rev' filesTaylor Blau, Jan 8, 2021
  3. 8/8 pack-revindex: ensure that on-disk reverse indexes are given precedenceTaylor Blau, Jan 8, 2021
  4. 3/8 builtin/index-pack.c: write reverse indexesTaylor Blau, Jan 8, 2021
  5. 1/8 packfile: prepare for the existence of '*.rev' filesTaylor Blau, Jan 8, 2021
  6. 4/8 builtin/pack-objects.c: respect 'pack.writeReverseIndex'Taylor Blau, Jan 8, 2021
  7. 7/8 t: support GIT_TEST_WRITE_REV_INDEXTaylor Blau, Jan 8, 2021
  8. Derrick StoleeJan 12, 2021
  9. Taylor BlauJan 12, 2021
  10. Ævar Arnfjörð BjarmasonJan 12, 2021
  11. Derrick StoleeJan 12, 2021
  12. Taylor BlauJan 12, 2021
  13. 6/8 t: prepare for GIT_TEST_WRITE_REV_INDEXTaylor Blau, Jan 8, 2021
  14. Ævar Arnfjörð BjarmasonJan 12, 2021
  15. Taylor BlauJan 12, 2021
  16. 5/8 Documentation/config/pack.txt: advertise 'pack.writeReverseIndex'Taylor Blau, Jan 8, 2021
  17. 0/8 pack-revindex: introduce on-disk '.rev' formatTaylor Blau, Jan 13, 2021
  18. 8/8 pack-revindex: ensure that on-disk reverse indexes are given precedenceTaylor Blau, Jan 13, 2021
  19. 6/8 t: prepare for GIT_TEST_WRITE_REV_INDEXTaylor Blau, Jan 13, 2021
  20. 1/8 packfile: prepare for the existence of '*.rev' filesTaylor Blau, Jan 13, 2021
  21. Junio C HamanoJan 14, 2021
  22. Derrick StoleeJan 14, 2021
  23. Jeff KingJan 14, 2021
  24. Taylor BlauJan 14, 2021
  25. Junio C HamanoJan 14, 2021
  26. Taylor BlauJan 14, 2021
  27. Junio C HamanoJan 14, 2021
  28. Jeff KingJan 22, 2021
  29. Taylor BlauJan 25, 2021
  30. Jeff KingJan 25, 2021
  31. Junio C HamanoJan 25, 2021
  32. Taylor BlauJan 25, 2021
  33. 5/8 Documentation/config/pack.txt: advertise 'pack.writeReverseIndex'Taylor Blau, Jan 13, 2021
  34. 4/8 builtin/pack-objects.c: respect 'pack.writeReverseIndex'Taylor Blau, Jan 13, 2021
  35. Jeff KingJan 22, 2021
  36. Jeff KingJan 23, 2021
  37. Taylor BlauJan 25, 2021
  38. Jeff KingJan 25, 2021
  39. 2/8 pack-write.c: prepare to write 'pack-*.rev' filesTaylor Blau, Jan 13, 2021
  40. Jeff KingJan 22, 2021
  41. Taylor BlauJan 25, 2021
  42. Jeff KingJan 26, 2021
  43. 7/8 t: support GIT_TEST_WRITE_REV_INDEXTaylor Blau, Jan 13, 2021
  44. 3/8 builtin/index-pack.c: write reverse indexesTaylor Blau, Jan 13, 2021
  45. Jeff KingJan 22, 2021
  46. Taylor BlauJan 25, 2021
  47. 00/10 pack-revindex: introduce on-disk '.rev' formatTaylor Blau, Jan 25, 2021
  48. 01/10 packfile: prepare for the existence of '*.rev' filesTaylor Blau, Jan 25, 2021
  49. Jeff KingJan 29, 2021
  50. Taylor BlauJan 29, 2021
  51. Jeff KingJan 30, 2021
  52. 08/10 t: support GIT_TEST_WRITE_REV_INDEXTaylor Blau, Jan 25, 2021
  53. Jeff KingJan 29, 2021
  54. 07/10 t: prepare for GIT_TEST_WRITE_REV_INDEXTaylor Blau, Jan 25, 2021
  55. Jeff KingJan 29, 2021
  56. Eric SunshineJan 29, 2021
  57. Taylor BlauJan 29, 2021
  58. Jeff KingJan 30, 2021
  59. Junio C HamanoJan 29, 2021
  60. 05/10 builtin/pack-objects.c: respect 'pack.writeReverseIndex'Taylor Blau, Jan 25, 2021
  61. 09/10 pack-revindex: ensure that on-disk reverse indexes are given precedenceTaylor Blau, Jan 25, 2021
  62. Jeff KingJan 29, 2021
  63. Taylor BlauJan 29, 2021
  64. Jeff KingJan 30, 2021
  65. 10/10 t5325: check both on-disk and in-memory reverse indexTaylor Blau, Jan 25, 2021
  66. Jeff KingJan 29, 2021
  67. Jeff KingJan 29, 2021
  68. Taylor BlauJan 29, 2021
  69. Jeff KingJan 30, 2021
  70. 06/10 Documentation/config/pack.txt: advertise 'pack.writeReverseIndex'Taylor Blau, Jan 25, 2021
  71. Jeff KingJan 29, 2021
  72. Taylor BlauJan 29, 2021
  73. Jeff KingJan 30, 2021
  74. 02/10 pack-write.c: prepare to write 'pack-*.rev' filesTaylor Blau, Jan 25, 2021
  75. 04/10 builtin/index-pack.c: write reverse indexesTaylor Blau, Jan 25, 2021
  76. 03/10 builtin/index-pack.c: allow stripping arbitrary extensionsTaylor Blau, Jan 25, 2021
  77. Jeff KingJan 29, 2021
  78. Taylor BlauJan 29, 2021
  79. Junio C HamanoJan 26, 2021
  80. Taylor BlauJan 26, 2021
  81. Jeff KingJan 29, 2021
  82. Taylor BlauJan 29, 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.