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

Commit Message

Junio C Hamano July 15, 2024, 4:22 p.m. UTC

Abhijeet Sonar <[e-mail]> 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 <[e-mail]>
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 <[e-mail]>
[jc: fixed up the test]
Signed-off-by: Junio C Hamano <[e-mail]>
---
 builtin/show-index.c   | 3 +++
 t/t5300-pack-object.sh | 4 ++++
 2 files changed, 7 insertions(+)

Comments

Abhijeet Sonar Oct. 26, 2024, 12:17 p.m. UTC | #1

Please excuse the neco-bump.

On 15/07/24 21:52, Junio C Hamano wrote:

> With that, your patch would become like so:
> 
> ------------ >8 ----------------------- >8 ------------

I misunderstood this as "I have made these changes to your patch on your
behalf". But looking at how this was never queued and the commit does
not appear in any upstream branch, I realised that I was supposed to
send another iteration. Apologies.

I have sent another iteration (v3) just before writing this.

Thanks

Patch

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 &&