Re: [PATCH 2/7] repack: add --full-name-hash option

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 21, 2024, 20:12 UTC
Message-ID: <Zz+UJHclSsb+Bgfo@nand.local>
In-Reply-To: <93395c93347274d075c3e29b3bd20dcc221b15be.1730775908.git.gitgitgadget@gmail.com>

On Tue, Nov 05, 2024 at 03:05:02AM +0000, Derrick Stolee via GitGitGadget wrote:

> ---
>  t/t7700-repack.sh       |  7 +++++++
>  t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)

OK, I stand by my thinking in the previous patch that this one is where the changes to builtin/repack.c belong.

Show 12 quoted lines

> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index c4c3d1a15d9..fc2cc9d37be 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -777,6 +777,13 @@ test_expect_success 'repack -ad cleans up old .tmp-* packs' '
>  	test_must_be_empty tmpfiles
>  '
>
> +test_expect_success '--full-name-hash option passes through to pack-objects' '
> +	GIT_TRACE2_EVENT="$(pwd)/full-trace.txt" \
> +		git repack -a --full-name-hash &&
> +	test_subcommand_flex git pack-objects --full-name-hash <full-trace.txt

OK. To be honest, I am not sure I would have written the same test to test trivially correct behavior, but I am not opposed to having such a test either.

I do think that test_subcommand_flex may be unnecessary though, since you could instead write this as:

    test_subcommand "git pack-objects.*--full-name-hash" <full-trace.txt

and get the same behavior.

> +'
> +
> +

Nit: extra newline here.

Thanks, Taylor