On Thu, Mar 24, 2022 at 06:34:56PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
>
> The '--write-midx -b packs non-kept objects' test in t7700-repack.sh
> uses test_subcommand_inexact to check that 'git repack' properly adds
> the '--honor-pack-keep' flag to the 'git pack-objects' subcommand.
> However, the test_subcommand_inexact helper is more flexible than
> initially designed, and this instance is the only one that makes use of
> it: there are additional arguments between 'git pack-objects' and the
> '--honor-pack-keep' flag. In order to make test_subcommand_inexact more
> strict, we need to fix this instance.
>
> This test checks that 'git repack --write-midx -a -b -d' will create a
> new pack-file that does not contain the objects within the kept pack.
> This behavior is possible because of the multi-pack-index bitmap that
> will bitmap objects against multiple packs. Without --write-midx, the
> objects in the kept pack would be duplicated so the resulting pack is
> closed under reachability and bitmaps can be created against it. This is
> discussed in more detail in e4d0c11c0 (repack: respect kept objects with
> '--write-midx -b', 2021-12-20) which also introduced this instance of
> test_subcommand_inexact.
>
> To better verify the intended post-conditions while also removing this
> instance of test_subcommand_inexact, rewrite the test to check the list
> of packed objects in the kept pack and the list of the objects in the
> newly-repacked pack-file _other_ than the kept pack. These lists should
> be disjoint.
>
> Be sure to include a non-kept pack-file and loose objects to be extra
> careful that this is properly behaving with kept packs and not just
> avoiding repacking all pack-files.
Nicely put.
> Co-authored-by: Taylor Blau <me@xxxxxxxxxxxx>
> Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
And to be clear, I am totally OK with this usage of my signed-off-by.
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 770d1432046..73452e23896 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '
> )
> '
>
> +get_sorted_objects_from_packs () {
> + git show-index <$(cat) >raw &&
It seems a little odd to me to pass the name of a single file as input
to get_sorted_objects_from_packs over stdin. I probably would have
expected something like `git show-index <"$1" >raw && ...` instead.
We may also want to s/packs/pack, since this function only will handle
one index at a time.
> + cut -d" " -f2 raw | sort
Having the sort in there is my fault, but after reading this more
carefully it's definitely unnecessary, since show-index will give us
the results in lexical order by object name already.
> +}
> +
> test_expect_success '--write-midx -b packs non-kept objects' '
> - GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
> - git repack --write-midx -a -b &&
> - test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
> + git init repo &&
> + test_when_finished "rm -fr repo" &&
> + (
> + cd repo &&
> +
> + # Create a kept pack-file
> + test_commit base &&
> + git repack -ad &&
> + find $objdir/pack -name "*.idx" >before &&
I thought that here it might be easier to say:
before="$(find $objdir/pack -name "*.idx")"
> + >$objdir/pack/$(basename $(cat before) .idx).keep &&
...and then replace "$(cat before)" with "$before", along with the
other uses of the before file below. But it gets a little funny when
you want to discover which is the new pack, where it is more natural to
dump the output of comm into a file.
> + # Get object list from the one non-kept pack-file
> + comm -13 before after >new-pack &&
You could write "new_pack=$(comm -13 before after)", but debugging this
test would be difficult if the output of comm there contained more than
one line.
> + get_sorted_objects_from_packs \
> + <new-pack \
Though we probably want to check that we only get one line anyway here,
since get_sorted_objects_from_packs will barf if we had more than one
line in file new-pack here, too.
Thanks,
Taylor
