Re: [PATCH v3] show-index: fix uninitialized hash function

[Taylor Blau <me@ttaylorr.com>]

On Sat, Oct 26, 2024 at 05:39:50PM +0530, Abhijeet Sonar wrote:
> As stated in the docs, show-index should use SHA1 as the default hash algorithm
> when run outsize of a repository.  However, 'the_hash_algo' is currently left
> uninitialized if we are not in a repository and no explicit hash function is
> specified, causing a crash.  Fix it by falling back to SHA1 when it is found
> uninitialized. Also add test that verifies this behaviour.

This commit description is good, and would benefit further from a
bisection showing where the regression began. I don't think that it is a
prerequisite for us moving this patch forward, though.

> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
> ---
>  builtin/show-index.c   | 3 +++
>  t/t5300-pack-object.sh | 4 ++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/builtin/show-index.c b/builtin/show-index.c
> index f164c01bbe..978ae70470 100644
> --- a/builtin/show-index.c
> +++ b/builtin/show-index.c
> @@ -38,6 +38,9 @@ int cmd_show_index(int argc,
>  		repo_set_hash_algo(the_repository, hash_algo);
>  	}
>
> +	if (!the_hash_algo)
> +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +
>  	hashsz = the_hash_algo->rawsz;
>
>  	if (fread(top_index, 2 * 4, 1, stdin) != 1)
> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 3b9dae331a..51fed26cc4 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -523,6 +523,10 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '
>  	test_path_is_file foo.idx
>  '
>
> +test_expect_success SHA1 'show-index works OK outside a repository' '
> +	nongit git show-index <foo.idx
> +'
> +
>  test_expect_success !PTHREADS,!FAIL_PREREQS \
>  	'index-pack --threads=N or pack.threads=N warns when no pthreads' '
>  	test_must_fail git index-pack --threads=2 2>err &&
> --
> 2.47.0.107.g34b6ce9b30

These all look reasonable and as-expected to me. Patrick (CC'd) has been
reviewing similar changes elsewhere, so I'd like him to chime in as well
on whether or not this looks good to go.

Thanks,
Taylor