From: Taylor Blau <me@ttaylorr.com>
Date: Thu, 21 Nov 2024 20:12:20 GMT
Subject: Re: [PATCH 2/7] repack: add --full-name-hash option
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.

> 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