Re: [PATCH 1/2] gc: add tests for --cruft and friends

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2022, 16:09 UTC
Message-ID: <xmqqtu6s6lda.fsf@gitster.g>
In-Reply-To: <220804.86a68ke9d5.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 6 quoted lines

>> +test_expect_success 'gc --cruft generates a cruft pack' '
>> +	git init crufts &&
>> +	test_when_finished "rm -fr crufts" &&
>
> Let's "test_when_finished" first, then "git init", the point is to clean
> up the directory if we fail.

Good advice.

We say "rm -fr" not "rm -r" there because we do not want to see a failure to remove if "git init" failed before it manages to create the directory ;-)

Show 41 quoted lines

>
>> +	(
>> +		cd crufts &&
>> +		test_commit base &&
>> +
>> +		test_commit --no-tag foo &&
>> +		test_commit --no-tag bar &&
>> +		git reset HEAD^^ &&
>> +
>> +		git gc --cruft &&
>> +
>> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
>> +		test_path_is_file .git/objects/pack/$cruft.pack
>> +	)
>> +'
>
> ...this...
>
>> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
>> +	git init crufts &&
>> +	test_when_finished "rm -fr crufts" &&
>> +	(
>> +		cd crufts &&
>> +		test_commit base &&
>> +
>> +		test_commit --no-tag foo &&
>> +		test_commit --no-tag bar &&
>> +		git reset HEAD^^ &&
>> +
>> +		git -c gc.cruftPacks=true gc &&
>> +
>> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
>> +		test_path_is_file .git/objects/pack/$cruft.pack
>> +	)
>> +'
>> +
>
> ...whole thing seems to be copy/pasted aside from the git options.
>
> If so let's factor this into a trivial helper that invokes git "$@",
> then call it with "gc --cruft" and "-c ..."?

With shell, passing "here is a series of commands to be run in the middle of a boilerplate sequence" is indeed easy to write, but it gets harder to follow and quote correctly, which is why I'd rather not see that pattern overused.

A pair of helper functions, one of which prepares a sample history to be used, and the other checks if we created one (or more) cruft packs, may achieve the same conciseness while remaining to be more readable. I.e.

    test_when_finished "rm -fr crufts" &&
    git init crufts &&
    (
	cd crufts &&
	prepare_history &&

	git -c gc.cruftPacks=true gc &&

	cruft_packs_exist
    )

perhaps?