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

Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary

From
Taylor Blau <me@ttaylorr.com>
Date
Aug 25, 2020, 15:42 UTC
Message-ID
<20200825154224.GA9116@syl.lan>
In-Reply-To
<6a34d7ee-8c6b-8c6c-93bd-0013dccccafb@gmail.com>
On Tue, Aug 25, 2020 at 11:14:41AM -0400, Derrick Stolee wrote:
Show 113 quoted lines
> On 8/25/2020 10:41 AM, Taylor Blau wrote:
> > On Tue, Aug 25, 2020 at 09:14:19AM -0400, Derrick Stolee wrote:
> >> The code in builtin/repack.c looks good for sure. I have a quick question
> >> about this new test:
> >>
> >> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> >> +	git multi-pack-index write &&
> >> +	cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> >> +	test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> >> +
> >> +	# Write a new pack that is unknown to the multi-pack-index.
> >> +	git hash-object -w </dev/null >blob &&
> >> +	git pack-objects $objdir/pack/pack <blob &&
> >> +
> >> +	GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >> +	test_cmp_bin $objdir/pack/multi-pack-index \
> >> +		$objdir/pack/multi-pack-index.bak
> >> +'
> >> +
> >>
> >> You create an arbitrary blob, and then add it to a pack-file. Do we
> >> know that 'git repack' is definitely creating a new pack-file that makes
> >> our manually-created pack-file redundant?
> >>
> >> My suggestion is to have the test check itself:
> >>
> >> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> >> +	git multi-pack-index write &&
> >> +	cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> >> +	test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> >> +
> >> +	# Write a new pack that is unknown to the multi-pack-index.
> >> +	git hash-object -w </dev/null >blob &&
> >> +	HASH=$(git pack-objects $objdir/pack/pack <blob) &&
> >> +
> >> +	GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >> +	test_cmp_bin $objdir/pack/multi-pack-index \
> >> +		$objdir/pack/multi-pack-index.bak &&
> >> +	test_path_is_missing $objdir/pack/pack-$HASH.pack
> >> +'
> >> +
> >>
> >> This test fails for me, on the 'test_path_is_missing'. Likely, the
> >> blob is seen as already in a pack-file so is just pruned by 'git repack'
> >> instead. I thought that perhaps we need to add a new pack ourselves that
> >> overrides the small pack. Here is my attempt:
> >>
> >> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> >> 	git multi-pack-index write &&
> >> 	cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> >> 	test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> >>
> >> 	# Write a new pack that is unknown to the multi-pack-index.
> >> 	BLOB1=$(echo blob1 | git hash-object -w --stdin) &&
> >> 	BLOB2=$(echo blob2 | git hash-object -w --stdin) &&
> >> 	cat >blobs <<-EOF &&
> >> 	$BLOB1
> >> 	$BLOB2
> >> 	EOF
> >> 	HASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&
> >> 	HASH2=$(git pack-objects $objdir/pack/pack <blobs) &&
> >> 	GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >> 	test_cmp_bin $objdir/pack/multi-pack-index \
> >> 		$objdir/pack/multi-pack-index.bak &&
> >> 	test_path_is_file $objdir/pack/pack-$HASH2.pack &&
> >> 	test_path_is_missing $objdir/pack/pack-$HASH1.pack
> >> '
> >>
> >> However, this _still_ fails on the "test_path_is_missing" line, so I'm not sure
> >> how to make sure your logic is tested. I saw that 'git repack' was writing
> >> "nothing new to pack" in the output, so I also tested adding a few commits and
> >> trying to force it to repack reachable data, but I cannot seem to trigger it
> >> to create a new pack that overrides only one pack that is not in the MIDX.
> >>
> >> Likely, I just don't know how 'git rebase' works well enough to trigger this
> >> behavior. But the test as-is is not testing what you want it to test.
> >
> > I think this case might actually be impossible to tickle in a test. I
> > thought that 'git repack -d' looked for existing packs whose objects are
> > a subset of some new pack generated. But, it's much simpler than that:
> > '-d' by itself just looks for packs that were already on disk with the
> > same SHA-1 as a new pack, and it removes the old one.
>
> If 'git repack' never calls remove_redundant_pack() unless we are doing
> a "full repack", then we could simplify this logic:
>
>  static void remove_redundant_pack(const char *dir_name, const char *base_name)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> -	strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
> +	struct multi_pack_index *m = get_multi_pack_index(the_repository);
> +	strbuf_addf(&buf, "%s.pack", base_name);
> +	if (m && midx_contains_pack(m, buf.buf))
> +		clear_midx_file(the_repository);
> +	strbuf_insertf(&buf, 0, "%s/", dir_name);
>  	unlink_pack_path(buf.buf, 1);
>  	strbuf_release(&buf);
>  }
>
> to
>
>  static void remove_redundant_pack(const char *dir_name, const char *base_name)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> 	strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
> +	clear_midx_file(the_repository);
>  	unlink_pack_path(buf.buf, 1);
>  	strbuf_release(&buf);
>  }
>
> and get the same results as we are showing in these tests. This does
> move us incrementally to a better situation: don't delete the MIDX
> if we aren't deleting pack files. But, I think we can get around it.

Makes sense, but reading your whole email we are better off leaving this as-is and changing the test to exercise it more often.

Show 59 quoted lines
> > Note that 'git repack' uses 'git pack-objects' internally to find
> > objects and generate a packfile. When calling 'git pack-objects', 'git
> > repack -d' passes '--all' and '--unpacked', which means that there is no
> > way we'd generate a new pack with the same SHA-1 as an existing pack
> > ordinarily.
> >
> > So, I think this case is impossible, or at least astronomically
> > unlikely. What is more interesting (and untested) is that adding a _new_
> > pack doesn't cause us to invalidate the MIDX. Here's a patch that does
> > that:
> >
> >   diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> >   index 16a1ad040e..620f2058d6 100755
> >   --- a/t/t5319-multi-pack-index.sh
> >   +++ b/t/t5319-multi-pack-index.sh
> >   @@ -391,18 +391,27 @@ test_expect_success 'repack removes multi-pack-index when deleting packs' '
> >           test_path_is_missing $objdir/pack/multi-pack-index
> >    '
> >
> >   -test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> >   -       git multi-pack-index write &&
> >   -       cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> >   -       test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> >   -
> >   -       # Write a new pack that is unknown to the multi-pack-index.
> >   -       git hash-object -w </dev/null >blob &&
> >   -       git pack-objects $objdir/pack/pack <blob &&
> >   -
> >   -       GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >   -       test_cmp_bin $objdir/pack/multi-pack-index \
> >   -               $objdir/pack/multi-pack-index.bak
> >   +test_expect_success 'repack preserves multi-pack-index when creating packs' '
> >   +       git init preserve &&
> >   +       test_when_finished "rm -fr preserve" &&
> >   +       (
> >   +               cd preserve &&
> >   +               midx=.git/objects/pack/multi-pack-index &&
> >   +
> >   +               test_commit "initial" &&
> >   +               git repack -ad &&
> >   +               git multi-pack-index write &&
> >   +               ls .git/objects/pack | grep "\.pack$" >before &&
> >   +
> >   +               cp $midx $midx.bak &&
> >   +
> >   +               test_commit "another" &&
> >   +               GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >   +               ls .git/objects/pack | grep "\.pack$" >after &&
> >   +
> >   +               test_cmp_bin $midx.bak $midx &&
> >   +               ! test_cmp before after
> >   +       )
> >    '
>
> After looking at the callers to remote_redundant_pack() I noticed that it is only
> called after inspecting the "names" struct, which contains the names of the packs
> to group into a new pack-file. We can use a .keep file to preserve the pack-file(s) in
> the MIDX but also to ensure multiple pack-files outside of the MIDX are repacked and
> deleted. While this is very unlikely in the wild, it is definitely possible.
Great idea.
Show 12 quoted lines
> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> 	git init preserve &&
> 	test_when_finished "rm -fr preserve" &&
> 	(
> 		cd preserve &&
> 		midx=.git/objects/pack/multi-pack-index &&
>
> 		test_commit 1 &&
> 		HASH1=$(git pack-objects --all .git/objects/pack/pack) &&
> 		touch .git/objects/pack/pack-$HASH1.keep &&
>
> 		cat >pack-input <<-\EOF &&

Escaping the heredoc shouldn't be necessary, so this can be written instead as '<<-EOF'.

Show 22 quoted lines
> 		HEAD
> 		^HEAD~1
> 		EOF
>
> 		test_commit 2 &&
> 		HASH2=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
> 		touch .git/objects/pack/pack-$HASH2.keep &&
>
> 		git multi-pack-index write &&
> 		cp $midx $midx.bak &&
>
> 		test_commit 3 &&
> 		HASH3=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
>
> 		test_commit 4 &&
> 		HASH4=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
>
> 		GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -ad &&
> 		test_path_is_file .git/objects/pack/pack-$HASH1.pack &&
> 		test_path_is_file .git/objects/pack/pack-$HASH2.pack &&
> 		test_path_is_missing .git/objects/pack/pack-$HASH3.pack &&
> 		test_path_is_missing .git/objects/pack/pack-$HASH4.pack

...and we should check that 'test_cmp $midx.bak $midx' is clean, i.e., that we didn't touch the MIDX.

>        )
> '
>
> I believe this checks your condition properly enough.

Otherwise, I think that this test looks great. Thanks for suggesting it. I'll send a new patch now...

> Thanks,
> -Stolee

Thanks, Taylor

Previous: Derrick StoleeNext: Jeff King
Message 7 of 78 in “builtin/repack.c: invalidate MIDX only when necessary”
  1. builtin/repack.c: invalidate MIDX only when necessaryTaylor Blau, Aug 25, 2020
  2. Jeff KingAug 25, 2020
  3. Taylor BlauAug 25, 2020
  4. Derrick StoleeAug 25, 2020
  5. Taylor BlauAug 25, 2020
  6. Derrick StoleeAug 25, 2020
  7. Taylor BlauAug 25, 2020
  8. Jeff KingAug 25, 2020
  9. Junio C HamanoAug 25, 2020
  10. Taylor BlauAug 25, 2020
  11. Derrick StoleeAug 25, 2020
  12. Jeff KingAug 25, 2020
  13. Jeff KingAug 25, 2020
  14. Junio C HamanoAug 25, 2020
  15. Jeff KingAug 25, 2020
  16. pack-redundant: gauge the usage before proposing its removalJunio C Hamano, Aug 25, 2020
  17. Taylor BlauAug 25, 2020
  18. Junio C HamanoAug 25, 2020
  19. 0/3 War on dashed-gitJunio C Hamano, Aug 26, 2020
  20. 1/3 transport-helper: do not run git-remote-ext etc. in dashed formJunio C Hamano, Aug 26, 2020
  21. Eric SunshineAug 26, 2020
  22. Johannes SchindelinAug 26, 2020
  23. Junio C HamanoAug 26, 2020
  24. 2/3 cvsexportcommit: do not run git programs in dashed formJunio C Hamano, Aug 26, 2020
  25. Eric SunshineAug 26, 2020
  26. Junio C HamanoAug 26, 2020
  27. Junio C HamanoAug 26, 2020
  28. Junio C HamanoAug 26, 2020
  29. Johannes SchindelinAug 26, 2020
  30. 3/3 git: catch an attempt to run "git-foo"Junio C Hamano, Aug 26, 2020
  31. Junio C HamanoAug 26, 2020
  32. Johannes SchindelinAug 26, 2020
  33. Junio C HamanoAug 26, 2020
  34. Johannes SchindelinAug 28, 2020
  35. Junio C HamanoAug 28, 2020
  36. Johannes SchindelinAug 31, 2020
  37. Junio C HamanoAug 31, 2020
  38. Johannes SchindelinDec 20, 2020
  39. Junio C HamanoDec 21, 2020
  40. Johannes SchindelinDec 30, 2020
  41. Johannes SchindelinAug 26, 2020
  42. Junio C HamanoAug 26, 2020
  43. 0/2 avoid running "git-subcmd" in the dashed formJunio C Hamano, Aug 26, 2020
  44. 1/2 transport-helper: do not run git-remote-ext etc. in dashed formJunio C Hamano, Aug 26, 2020
  45. 2/2 cvsexportcommit: do not run git programs in dashed formJunio C Hamano, Aug 26, 2020
  46. 3/2 credential-cache: use child_process.argsJunio C Hamano, Aug 26, 2020
  47. run_command: teach API users to use embedded 'args' moreJunio C Hamano, Aug 26, 2020
  48. Jeff KingAug 27, 2020
  49. Junio C HamanoAug 27, 2020
  50. Eric SunshineAug 27, 2020
  51. Jeff KingAug 27, 2020
  52. Eric SunshineAug 27, 2020
  53. worktree: fix leak in check_clean_worktree()Jeff King, Aug 27, 2020
  54. Eric SunshineAug 27, 2020
  55. Junio C HamanoAug 27, 2020
  56. Jeff KingAug 27, 2020
  57. Jeff KingAug 27, 2020
  58. Junio C HamanoAug 27, 2020
  59. Jeff KingAug 27, 2020
  60. Junio C HamanoAug 27, 2020
  61. Junio C HamanoAug 31, 2020
  62. Jeff KingSep 1, 2020
  63. Junio C HamanoSep 1, 2020
  64. Derrick StoleeAug 27, 2020
  65. Junio C HamanoAug 27, 2020
  66. Jeff KingAug 28, 2020
  67. Junio C HamanoAug 28, 2020
  68. Son Luong NgocAug 25, 2020
  69. Derrick StoleeAug 25, 2020
  70. Taylor BlauAug 25, 2020
  71. builtin/repack.c: invalidate MIDX only when necessaryTaylor Blau, Aug 25, 2020
  72. Derrick StoleeAug 26, 2020
  73. Junio C HamanoAug 26, 2020
  74. Jeff KingAug 25, 2020
  75. Derrick StoleeAug 25, 2020
  76. Jeff KingAug 25, 2020
  77. Taylor BlauAug 25, 2020
  78. Jeff KingAug 25, 2020

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.