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

[Junio C Hamano <gitster@pobox.com>]

Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:

>  t/t8101-show-index-hash-function.sh | 15 +++++++++++++++
>  2 files changed, 18 insertions(+)
>  create mode 100755 t/t8101-show-index-hash-function.sh

Thanks.  But let's not waste the scarce resource that is a test
number for a single oddball test (and as t/README says t8xxx series
is for forensics Porcelains).

> +test_expect_success 'show-index: should not fail outside a repository' '
> +    git init --object-format=sha1 && (
> +        echo "" | git hash-object -w --stdin | git pack-objects test &&

Our tests run in an already initialized repository, and some test
configuration would use sha256 to initialize that repository.  So
the above is not a good idea, unless you use a new directory.  We
often create a new directory inside the initial directory the test
begins in, and then in a subshell chdir into the directory.

We frown upon a pipeline that has "git" as an upstream, because the
exit status from such invocation of "git" will be hidden.

	git init --object-format=sha1 sample &&
	(
		cd sample &&
		O=$(git hash-object -w /dev/null) &&
		T=$(echo "$O" | git pack-objects test) &&

would give you a pair of files "test-$T.idx" and "test-$T.pack" and
it will notice if hash-object or pack-objects fail.

> +        rm -rf .git &&

This alone does *not* necessarily make the directory you are using
for test completely unassociated with any repository.  If you are
working with the source code of Git from a repository (as opposed to
extracted tar archive), with that "rm -fr", you may have made the
directory not a Git repository, but then that directory is now a
mere subdirectory "t/trash directory.t8101-show-index-hash-function"
of the repository that houses the Git source code (unless you are
using the --root=<directory> option to run the tests).

When we test behaviour of commands outside a repository, we use the
GIT_CEILING_DIRECTORIES feature, often via the nongit helper function
that is defined in t/test-lib-functions.sh (which becomes available
to tests by doing ". ./test-lib.sh".

In t5300-pack-object.sh we see these bits already.

    test_expect_success 'index-pack --stdin complains of non-repo' '
            nongit test_must_fail git index-pack \
                    --object-format=$(test_oid algo) --stdin <foo.pack &&
            test_path_is_missing non-repo/.git
    '

    test_expect_success 'index-pack <pack> works in non-repo' '
            nongit git index-pack \
                    --object-format=$(test_oid algo) ../foo.pack &&
            test_path_is_file foo.idx
    '

I wonder if it is sufficient to add a new test after these two
steps, something like

    test_expect_success SHA1 'show-index works OK outside a repository' '
	    nongit git show-index <foo.idx
    '

perhaps?

With that, your patch would become like so:

------------ >8 ----------------------- >8 ------------
From: Abhijeet Sonar <abhijeet.nkt@gmail.com>
Date: Mon, 15 Jul 2024 15:53:43 +0530
Subject: [PATCH] show-index: fix uninitialized hash function

As stated in the docs, show-index should use SHA1 as the default
hash algorithm when run outside a repository.

However, 'the_hash_algo' is 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.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
[jc: fixed up the test]
Signed-off-by: Junio C Hamano <gitster@pobox.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 540dc3dad1..bb6d9e3c40 100644
--- a/builtin/show-index.c
+++ b/builtin/show-index.c
@@ -35,6 +35,9 @@ int cmd_show_index(int argc, const char **argv, const char *prefix)
 		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 4ad023c846..83933eca5d 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.46.0-rc0-140-g824782812f