[PATCH] show-index: fix uninitialized hash function

[PATCH] show-index: fix uninitialized hash function

[Abhijeet Sonar]

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 funciton is
specified, causing a crash.  Fix it by falling back to SHA1 when it is found
uninitialized.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
---
 builtin/show-index.c | 3 +++
 1 file changed, 3 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)
-- 
2.45.2.827.g557ae147e6

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

[Junio C Hamano]

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

> 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 funciton is
> specified, causing a crash.  Fix it by falling back to SHA1 when it is found
> uninitialized.
>
> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
> ---
>  builtin/show-index.c | 3 +++
>  1 file changed, 3 insertions(+)

Nicely described.

We'd probably want to protect this with a new test, so that
regardless of the choice of GIT_TEST_DEFAULT_HASH, the command
should behave as advertised.

Having said that, I am not sure if --object-format specified on the
command line, or picked up from the repository, makes much sense in
the context of the command, especially for the longer term [*].  The
command is designed to read from its standard input a byte-stream,
which is assumed to be an .idx file of _any_ origin, so ideally it
should be able to tell what hash the incoming data uses and use that
hash algorithm, without being told from the command line?

But that longer-term worry has nothing to do with the validity of
this patch (but the lack of test does).  Thanks.

[Footnote]

 * Perhaps the file format does not make it obvious what hash
   algorithm it uses, so it may be hard to auto-detect without
   additional code.  But if that is the case, it would be something
   we may want to eventually fix.

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

[Eric Sunshine]

On Fri, Jul 12, 2024 at 10:24 AM Abhijeet Sonar <abhijeet.nkt@gmail.com> 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 funciton is

s/funciton/function/

> specified, causing a crash.  Fix it by falling back to SHA1 when it is found
> uninitialized.
>
> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>

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

[Abhijeet Sonar]

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.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
---
 builtin/show-index.c                |  3 +++
 t/t8101-show-index-hash-function.sh | 15 +++++++++++++++
 2 files changed, 18 insertions(+)
 create mode 100755 t/t8101-show-index-hash-function.sh

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/t8101-show-index-hash-function.sh b/t/t8101-show-index-hash-function.sh
new file mode 100755
index 0000000000..2e9308f73c
--- /dev/null
+++ b/t/t8101-show-index-hash-function.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='git show-index'
+
+. ./test-lib.sh
+
+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 &&
+        rm -rf .git &&
+        cat test-*.idx | git show-index
+    )
+'
+
+test_done
-- 
2.45.2.827.g557ae147e6

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

[Abhijeet Sonar]

On 12/07/24 21:05, Junio C Hamano wrote:
> Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
> 
>> 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 funciton is
>> specified, causing a crash.  Fix it by falling back to SHA1 when it is found
>> uninitialized.
>>
>> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
>> ---
>>  builtin/show-index.c | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> Nicely described.
> 
> We'd probably want to protect this with a new test, so that
> regardless of the choice of GIT_TEST_DEFAULT_HASH, the command
> should behave as advertised.

I wrote a test which build an index file using a `hash-object |
pack-objects` chain.  I am not sure if its the best way to do this, I
would appreciate some guidance on this.

Another way I can think of is having an index file sit along with the
tests in the codebase which will be read by `show-index` instead of
generating one on the fly.  Thoughts?

Thanks.

> 
> Having said that, I am not sure if --object-format specified on the
> command line, or picked up from the repository, makes much sense in
> the context of the command, especially for the longer term [*].  The
> command is designed to read from its standard input a byte-stream,
> which is assumed to be an .idx file of _any_ origin, so ideally it
> should be able to tell what hash the incoming data uses and use that
> hash algorithm, without being told from the command line?
> 
> But that longer-term worry has nothing to do with the validity of
> this patch (but the lack of test does).  Thanks.
> 
> [Footnote]
> 
>  * Perhaps the file format does not make it obvious what hash
>    algorithm it uses, so it may be hard to auto-detect without
>    additional code.  But if that is the case, it would be something
>    we may want to eventually fix.
> 

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

[Junio C Hamano]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

On 2024-07-15 at 10:23:43, Abhijeet Sonar wrote:
> diff --git a/t/t8101-show-index-hash-function.sh b/t/t8101-show-index-hash-function.sh
> new file mode 100755
> index 0000000000..2e9308f73c
> --- /dev/null
> +++ b/t/t8101-show-index-hash-function.sh
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +
> +test_description='git show-index'
> +
> +. ./test-lib.sh
> +
> +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 &&
> +        rm -rf .git &&
> +        cat test-*.idx | git show-index
> +    )
> +'

I don't think this change is going to work.  If you run with
`GIT_TEST_DEFAULT_HASH=sha256`, as I do, you see this error:

  fatal: attempt to reinitialize repository with different hash

That's because the repository is already initialized as SHA-256 when you
do `git init`.

The reason that the `--object-format` option was added was to make this
configuration work outside a repository.  It would probably be better to
require the user to specify that option if we're outside of a repository
rather than just try to guess.  We want to have _fewer_ dependencies on
SHA-1 as the implicit algorithm, not more.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Abhijeet Sonar]

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.

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

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

[Abhijeet Sonar]

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

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

[Taylor Blau]

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

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

[Patrick Steinhardt]

On Sun, Oct 27, 2024 at 08:10:16PM -0400, Taylor Blau wrote:
> On Sat, Oct 26, 2024 at 05:39:50PM +0530, Abhijeet Sonar wrote:
> > 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);

Let's add a todo-comment here. The behaviour with this patch is somewhat
broken as you cannot inspect indices that use any other object hash than
SHA256 outside of a repository. This is fine from my point of view and
nothing that you have to fix here, as you simply fix up the broken
behaviour. But in the future, we should either:

  - Add logic to detect the format of the passed-in index and set that
    up as the hash algorithm.

  - If that is impossible, add a command line option to pick the hash
    algo.

> >  	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
> > +'

So how does this behave with SHA256? Does it raise an error? Does it
segfault?

I think it's okay to fail with SHA256 for now, but I'd like the
failure behaviour to be cleanish. So I'd prefer to not skip the test
completely, but adapt our expectations based on the hash algo. Or have
two separate tests, one for each hash, that explicitly init the repo
with `git init --ref-format=$hash`, and then exercise the behaviour for
each of them.

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

Ah, thanks. I've missed this topic somehow.

Patrick

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

[Taylor Blau]

On Mon, Oct 28, 2024 at 06:35:15AM +0100, Patrick Steinhardt wrote:
> > >  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.
>
> Ah, thanks. I've missed this topic somehow.

Not a problem at all. Thanks for a very helpful review.

Abhijeet: I've gone ahead and marked this in my notes as "expecting
another round" to address the feedback from Patrick. I'll keep my eyes
out for the new version of this patch. Thanks!

Thanks,
Taylor

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

[Abhijeet Sonar]

On 28/10/24 05:40, Taylor Blau wrote:
> 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.

On bisecting, the offending commit appears to be c8aed5e8da (repository:
stop setting SHA1 as the default object hash).

Another related commit is ab274909d4 (builtin/diff: explicitly set hash
algo when there is no repo) which did something very similar to my patch.

Thanks

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

[Abhijeet Sonar]

On 28/10/24 11:05, Patrick Steinhardt wrote:
> On Sun, Oct 27, 2024 at 08:10:16PM -0400, Taylor Blau wrote:
>> On Sat, Oct 26, 2024 at 05:39:50PM +0530, Abhijeet Sonar wrote:
>>> 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);
> 
> Let's add a todo-comment here. The behaviour with this patch is somewhat
> broken as you cannot inspect indices that use any other object hash than
> SHA256 outside of a repository. This is fine from my point of view and
> nothing that you have to fix here, as you simply fix up the broken
> behaviour. But in the future, we should either:

I will add those comments, thanks.

> 
>   - Add logic to detect the format of the passed-in index and set that
>     up as the hash algorithm

I am very interested in hacking around and trying to implement this.
I read about the index format here:
https://git-scm.com/docs/index-format and (assuming I am looking at the
right thing) it does not seem like index files contain information about
the hash algorithm used in their headers or anywhere else. Are there
other leads I should follow?

> 
>   - If that is impossible, add a command line option to pick the hash
>     algo.
>
Actually there is already an option (--object-format) that allows
changing the hash algo used, it's just that default format is SHA1 when
one is not specified. (or at least will be, after this patch)

>>>  	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
>>> +'
> 
> So how does this behave with SHA256? Does it raise an error? Does it
> segfault?
> 
> I think it's okay to fail with SHA256 for now, but I'd like the
> failure behaviour to be cleanish. So I'd prefer to not skip the test
> completely, but adapt our expectations based on the hash algo. Or have
> two separate tests, one for each hash, that explicitly init the repo
> with `git init --ref-format=$hash`, and then exercise the behaviour for
> each of the>

Running show-index outside of a repository with a SHA256 based index
file gives:

$ git show-index <foo.idx
fatal: inconsistent 64b offset index

At the very least, it does not crash.

>>>  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.
> 
> Ah, thanks. I've missed this topic somehow.
> 
> Patrick

Thanks

[PATCH v4] show-index: fix uninitialized hash function

[Abhijeet Sonar]

In c8aed5e8da (repository: stop setting SHA1 as the default object
hash), we got rid of the default hash algorithm for the_repository.
Due to this change, it is now the responsibility of the callers to set
thier own default when this is not present.

As stated in the docs, show-index should use SHA1 as the default hash
algorithm when ran outsize of a repository. Make sure this promise is
met by falling back to SHA1 when the_hash_algo is not present (i.e.
when the command is ran outside of a repository). Also add a test that
verifies this behaviour.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
---
 builtin/show-index.c   | 6 ++++++
 rm                     | 3 +++
 t/t5300-pack-object.sh | 4 ++++
 3 files changed, 13 insertions(+)
 create mode 100755 rm

diff --git a/builtin/show-index.c b/builtin/show-index.c
index f164c01bbe..645c2548fb 100644
--- a/builtin/show-index.c
+++ b/builtin/show-index.c
@@ -38,6 +38,12 @@ int cmd_show_index(int argc,
 		repo_set_hash_algo(the_repository, hash_algo);
 	}
 
+	// Fallback to SHA1 if we are running outside of a repository.
+	// TODO: Figure out and implement a way to detect the hash algorithm in use by the
+	//       the index file passed in and use that instead.
+	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/rm b/rm
new file mode 100755
index 0000000000..2237506bf2
--- /dev/null
+++ b/rm
@@ -0,0 +1,3 @@
+#!/bin/sh
+
+echo rm $@
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

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

[Junio C Hamano]

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

> In c8aed5e8da (repository: stop setting SHA1 as the default object
> hash), we got rid of the default hash algorithm for the_repository.
> Due to this change, it is now the responsibility of the callers to set
> thier own default when this is not present.

"their own default".

> As stated in the docs, show-index should use SHA1 as the default hash
> algorithm when ran outsize of a repository. Make sure this promise is

"outside a repository".

> met by falling back to SHA1 when the_hash_algo is not present (i.e.
> when the command is ran outside of a repository). Also add a test that
> verifies this behaviour.
>
> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
> ---
>  builtin/show-index.c   | 6 ++++++
>  rm                     | 3 +++

Huh?

>  t/t5300-pack-object.sh | 4 ++++
>  3 files changed, 13 insertions(+)
>  create mode 100755 rm
>
> diff --git a/builtin/show-index.c b/builtin/show-index.c
> index f164c01bbe..645c2548fb 100644
> --- a/builtin/show-index.c
> +++ b/builtin/show-index.c
> @@ -38,6 +38,12 @@ int cmd_show_index(int argc,
>  		repo_set_hash_algo(the_repository, hash_algo);
>  	}
>  
> +	// Fallback to SHA1 if we are running outside of a repository.
> +	// TODO: Figure out and implement a way to detect the hash algorithm in use by the
> +	//       the index file passed in and use that instead.

	/*
	 * A multi-line comment in our codebase looks
	 * like this; slash-asterisk and asterisk-slash
	 * are placed on their own lines.  We do not do
	 * double-slash comments.
	 */

> +	if (!the_hash_algo)
> +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);

OK.  This is in line with how the command is documented to behave.

Having said that, I am not sure if it was an omission by mistake
when 8e42eb0e (doc: sha256 is no longer experimental, 2023-07-31)
marked SHA-256 as non-experimental, or it was deliberate.  It would
have been an equally plausible, if not more sensible, position to
take to say that, since SHA-1 and SHA-256 are now on equal footing,
we won't "default" to SHA-1 anymore, when 8e42eb0e declared that
SHA-256 is no longer a second-class citizen.

In any case, we can further remedy that, if we really wanted to, by
tweaking the documentation to require the option outside a
repository without any default, for example, and then change this to
die().

Of course, we may want to use the hash that is used in the index
file we are reading, if we can, as your comment said.

These incremental improvements can be left outside the scope of this
change.

> diff --git a/rm b/rm
> new file mode 100755
> index 0000000000..2237506bf2
> --- /dev/null
> +++ b/rm
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +
> +echo rm $@

Please don't.

> 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
> +'

If we are not using a hash that is not SHA-1, we should then be able
to do the same check with

    nongit git show-index --object-format=<hash> <foo.idx

i.e., with an explicit argument.  I do not think we have any hits
in the t/ directory from

    $ git grep -e 'show-index .*--object-format' t/

so such a test might be worth adding, either as a part of this
change or as a separate patch.
   
>  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 &&


Except for these minor nits, everything else looks great.

Thanks.

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

[Abhijeet Sonar]

On 02/11/24 15:59, Junio C Hamano wrote:
> Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
> 
>> In c8aed5e8da (repository: stop setting SHA1 as the default object
>> hash), we got rid of the default hash algorithm for the_repository.
>> Due to this change, it is now the responsibility of the callers to set
>> thier own default when this is not present.
> 
> "their own default".
> 
>> As stated in the docs, show-index should use SHA1 as the default hash
>> algorithm when ran outsize of a repository. Make sure this promise is
> 
> "outside a repository".
> 

I will address those in v5, thanks

>> met by falling back to SHA1 when the_hash_algo is not present (i.e.
>> when the command is ran outside of a repository). Also add a test that
>> verifies this behaviour.
>>
>> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
>> ---
>>  builtin/show-index.c   | 6 ++++++
>>  rm                     | 3 +++
> 
> Huh?
> 
>>  t/t5300-pack-object.sh | 4 ++++
>>  3 files changed, 13 insertions(+)
>>  create mode 100755 rm
>>
>> diff --git a/builtin/show-index.c b/builtin/show-index.c
>> index f164c01bbe..645c2548fb 100644
>> --- a/builtin/show-index.c
>> +++ b/builtin/show-index.c
>> @@ -38,6 +38,12 @@ int cmd_show_index(int argc,
>>  		repo_set_hash_algo(the_repository, hash_algo);
>>  	}
>>  
>> +	// Fallback to SHA1 if we are running outside of a repository.
>> +	// TODO: Figure out and implement a way to detect the hash algorithm in use by the
>> +	//       the index file passed in and use that instead.
> 
> 	/*
> 	 * A multi-line comment in our codebase looks
> 	 * like this; slash-asterisk and asterisk-slash
> 	 * are placed on their own lines.  We do not do
> 	 * double-slash comments.
> 	 */
> 
>> +	if (!the_hash_algo)
>> +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> 
> OK.  This is in line with how the command is documented to behave.
> 
> Having said that, I am not sure if it was an omission by mistake
> when 8e42eb0e (doc: sha256 is no longer experimental, 2023-07-31)
> marked SHA-256 as non-experimental, or it was deliberate.  It would
> have been an equally plausible, if not more sensible, position to
> take to say that, since SHA-1 and SHA-256 are now on equal footing,
> we won't "default" to SHA-1 anymore, when 8e42eb0e declared that
> SHA-256 is no longer a second-class citizen.>
> In any case, we can further remedy that, if we really wanted to, by
> tweaking the documentation to require the option outside a
> repository without any default, for example, and then change this to
> die().
> 
> Of course, we may want to use the hash that is used in the index
> file we are reading, if we can, as your comment said.
> 
> These incremental improvements can be left outside the scope of this
> change.
>

I see. So while this behavior not completely ideal, we are at least able
to resolve a segfault. I take it that it is OK to leave it like this in
this patch and address it separately after.

>> diff --git a/rm b/rm
>> new file mode 100755
>> index 0000000000..2237506bf2
>> --- /dev/null
>> +++ b/rm
>> @@ -0,0 +1,3 @@
>> +#!/bin/sh
>> +
>> +echo rm $@
> 
> Please don't.
> 

Oops, this is embarrassing, that probably slipped in from a different
thing I was experimenting with which is unrelated to this patch. I will
verify that my patches are free of such errors in future before sending
them, apologies.

>> 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
>> +'
> 
> If we are not using a hash that is not SHA-1, we should then be able
> to do the same check with
> 
>     nongit git show-index --object-format=<hash> <foo.idx
> 
> i.e., with an explicit argument.  I do not think we have any hits
> in the t/ directory from
> 
>     $ git grep -e 'show-index .*--object-format' t/
> 

Would that look something like this?

```
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 51fed26cc4..78047604e4 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -527,6 +527,22 @@ test_expect_success SHA1 'show-index works OK
outside a repository' '
        nongit git show-index <foo.idx
 '

+for hash in sha1 sha256
+do
+       test_expect_success 'show-index works OK outside a repository
with hash algo passed in via --object-format' '
+               git init --object-format=$hash $hash-repo &&
+               echo foo >$hash-repo/foo &&
+               git -C $hash-repo add foo &&
+               git -C $hash-repo commit -m "commit foo" &&
+               oid=$(git -C $hash-repo rev-parse HEAD) &&
+               echo $oid | git -C $hash-repo pack-objects $hash &&
+               mv $hash-repo/$hash-*.idx $hash.idx &&
+               nongit git show-index --object-format=$hash <$hash.idx &&
+               wow &&
+               rm -fr $hash/ $hash.idx
+       '
+done
+
 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 &&
```

> so such a test might be worth adding, either as a part of this
> change or as a separate patch.
>    
>>  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 &&
> 
> 
> Except for these minor nits, everything else looks great.
> 
> Thanks.

[PATCH v5 0/2] show-index: fix uninitialized hash function

[Abhijeet Sonar]

In this iteration, I have fixed some typos along with a stylistisc issue
that were noted in v4.

I have also added an additional patch that adds a test for --object-format
option in show-index as it was noted we don't already have any.

Abhijeet Sonar (2):
  show-index: fix uninitialized hash function
  t5300: add test for 'show-index --object-format'

 builtin/show-index.c   |  9 +++++++++
 t/t5300-pack-object.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

Range-diff against v4:
1:  c75175ad9b ! 1:  05ee1e2ea5 show-index: fix uninitialized hash function
    @@ Commit message
         In c8aed5e8da (repository: stop setting SHA1 as the default object
         hash), we got rid of the default hash algorithm for the_repository.
         Due to this change, it is now the responsibility of the callers to set
    -    thier own default when this is not present.
    +    their own default when this is not present.
     
         As stated in the docs, show-index should use SHA1 as the default hash
    -    algorithm when ran outsize of a repository. Make sure this promise is
    +    algorithm when run outside a repository. Make sure this promise is
         met by falling back to SHA1 when the_hash_algo is not present (i.e.
    -    when the command is ran outside of a repository). Also add a test that
    -    verifies this behaviour.
    +    when the command is run outside a repository). Also add a test that
    +    verifies this behavior.
     
         Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
     
    @@ builtin/show-index.c: int cmd_show_index(int argc,
      		repo_set_hash_algo(the_repository, hash_algo);
      	}
      
    -+	// Fallback to SHA1 if we are running outside of a repository.
    -+	// TODO: Figure out and implement a way to detect the hash algorithm in use by the
    -+	//       the index file passed in and use that instead.
    ++	/*
    ++	 * Fallback to SHA1 if we are running outside of a repository.
    ++	 *
    ++	 * TODO: Figure out and implement a way to detect the hash algorithm in use by the
    ++	 *       the index file passed in and use that instead.
    ++	 */
     +	if (!the_hash_algo)
     +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
     +
    @@ builtin/show-index.c: int cmd_show_index(int argc,
      
      	if (fread(top_index, 2 * 4, 1, stdin) != 1)
     
    - ## rm (new) ##
    -@@
    -+#!/bin/sh
    -+
    -+echo rm $@
    -
      ## t/t5300-pack-object.sh ##
     @@ t/t5300-pack-object.sh: test_expect_success 'index-pack --strict <pack> works in non-repo' '
      	test_path_is_file foo.idx
-:  ---------- > 2:  c8a28aae55 t5300: add test for 'show-index --object-format'
-- 
2.47.0.107.g34b6ce9b30

[PATCH v5 1/2] show-index: fix uninitialized hash function

[Abhijeet Sonar]

In c8aed5e8da (repository: stop setting SHA1 as the default object
hash), we got rid of the default hash algorithm for the_repository.
Due to this change, it is now the responsibility of the callers to set
their own default when this is not present.

As stated in the docs, show-index should use SHA1 as the default hash
algorithm when run outside a repository. Make sure this promise is
met by falling back to SHA1 when the_hash_algo is not present (i.e.
when the command is run outside a repository). Also add a test that
verifies this behavior.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
---
 builtin/show-index.c   | 9 +++++++++
 t/t5300-pack-object.sh | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/builtin/show-index.c b/builtin/show-index.c
index f164c01bbe..b5e337869d 100644
--- a/builtin/show-index.c
+++ b/builtin/show-index.c
@@ -38,6 +38,15 @@ int cmd_show_index(int argc,
 		repo_set_hash_algo(the_repository, hash_algo);
 	}
 
+	/*
+	 * Fallback to SHA1 if we are running outside of a repository.
+	 *
+	 * TODO: Figure out and implement a way to detect the hash algorithm in use by the
+	 *       the index file passed in and use that instead.
+	 */
+	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

[PATCH v5 2/2] t5300: add test for 'show-index --object-format'

[Abhijeet Sonar]

In 88a09a557c (builtin/show-index: provide options to determine hash
algo), the flag --object-format was added to show-index builtin as a way
to provide a hash algorithm explicitly. However, we do not have tests in
place for that functionality. Add them.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
---
 t/t5300-pack-object.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 51fed26cc4..301d5f1b61 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -527,6 +527,28 @@ test_expect_success SHA1 'show-index works OK outside a repository' '
 	nongit git show-index <foo.idx
 '
 
+for hash in sha1 sha256
+do
+	test_expect_success 'setup: show-index works OK outside a repository with hash algo passed in via --object-format' '
+		git init explicit-hash-$hash --object-format=$hash &&
+		test_commit -C explicit-hash-$hash one &&
+
+		cat >in <<-EOF &&
+		$(git -C explicit-hash-$hash rev-parse one)
+		EOF
+
+		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in
+	'
+
+	test_expect_success 'show-index works OK outside a repository with hash algo passed in via --object-format' '
+		idx=$(echo explicit-hash-$hash/explicit-hash-$hash*.idx) &&
+		nongit git show-index --object-format=$hash <"$idx" >actual &&
+		test_line_count = 1 actual &&
+
+		rm -rf explicit-hash-$hash
+	'
+done
+
 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

Re: [PATCH v5 2/2] t5300: add test for 'show-index --object-format'

[Junio C Hamano]

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

> In 88a09a557c (builtin/show-index: provide options to determine hash
> algo), the flag --object-format was added to show-index builtin as a way
> to provide a hash algorithm explicitly. However, we do not have tests in
> place for that functionality. Add them.
>
> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
> ---
>  t/t5300-pack-object.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

Nicely described.


> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 51fed26cc4..301d5f1b61 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -527,6 +527,28 @@ test_expect_success SHA1 'show-index works OK outside a repository' '
>  	nongit git show-index <foo.idx
>  '
>  
> +for hash in sha1 sha256
> +do
> +	test_expect_success 'setup: show-index works OK outside a repository with hash algo passed in via --object-format' '
> +		git init explicit-hash-$hash --object-format=$hash &&

"git help cli"; dashed options first and then other arguments.

> +		test_commit -C explicit-hash-$hash one &&
> +
> +		cat >in <<-EOF &&
> +		$(git -C explicit-hash-$hash rev-parse one)
> +		EOF

Hmph, is the above a roundabout way to say

		git -C explicit-hash-$hash rev-parse one >in &&

or am I missing some subtlety?

> +		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in

> +	'
> +
> +	test_expect_success 'show-index works OK outside a repository with hash algo passed in via --object-format' '
> +		idx=$(echo explicit-hash-$hash/explicit-hash-$hash*.idx) &&
> +		nongit git show-index --object-format=$hash <"$idx" >actual &&
> +		test_line_count = 1 actual &&
> +
> +		rm -rf explicit-hash-$hash

When this test fails (e.g., the number of lines in the show-index
output is not 1), explicit-hash-$hash is not removed, because &&-
chain short-circuits.

Perhaps join thw two into one and use test_when_finished, like this?

	test_expect_success 'show-index with explicit --object-format=$hash outside repo' '
		test_when_finished "rm -fr explicit-hash-$hash" &&
		git init --object-format=$hash explicit-hash-$hash &&
		...
                nongit git show-index --object-format=$hash <"$idx" >actual &&
		test_line_count 1 actual
	'

Other than that, very nicely done.

Thanks.

[PATCH v6 0/2] show-index: fix uninitialized hash function

[Abhijeet Sonar]

> Nicely described.

Thanks!

> "git help cli"; dashed options first and then other arguments.

Applied.

> Hmph, is the above a roundabout way to say
>   git -C explicit-hash-$hash rev-parse one >in &&

Applied.

> or am I missing some subtlety?

No, I don't think you are. However, I would like to point out that the code
which I used as the inspiration also does thing the same way:

	test_expect_success 'pack-object <stdin parsing: [|--revs] with --stdin' '
		cat >in <<-EOF &&
		$(git -C pack-object-stdin rev-parse one)
		$(git -C pack-object-stdin rev-parse two)
		EOF


> When this test fails (e.g., the number of lines in the show-index
> output is not 1), explicit-hash-$hash is not removed, because &&-
> chain short-circuits.
> 
> Perhaps join thw two into one and use test_when_finished, like this?
> 
> 	test_expect_success 'show-index with explicit --object-format=$hash outside repo' '
> 		test_when_finished "rm -fr explicit-hash-$hash" &&
> 		git init --object-format=$hash explicit-hash-$hash &&
> 		...
>                 nongit git show-index --object-format=$hash <"$idx" >actual &&
> 		test_line_count 1 actual
> 	'

That makes sense, applied.


Abhijeet Sonar (2):
  show-index: fix uninitialized hash function
  t5300: add test for 'show-index --object-format'

 builtin/show-index.c   |  9 +++++++++
 t/t5300-pack-object.sh | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

Range-diff against v5:
1:  05ee1e2ea5 = 1:  05ee1e2ea5 show-index: fix uninitialized hash function
2:  c8a28aae55 ! 2:  778f3ca18e t5300: add test for 'show-index --object-format'
    @@ t/t5300-pack-object.sh: test_expect_success SHA1 'show-index works OK outside a
      
     +for hash in sha1 sha256
     +do
    -+	test_expect_success 'setup: show-index works OK outside a repository with hash algo passed in via --object-format' '
    -+		git init explicit-hash-$hash --object-format=$hash &&
    -+		test_commit -C explicit-hash-$hash one &&
    -+
    -+		cat >in <<-EOF &&
    -+		$(git -C explicit-hash-$hash rev-parse one)
    -+		EOF
    -+
    -+		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in
    -+	'
    -+
     +	test_expect_success 'show-index works OK outside a repository with hash algo passed in via --object-format' '
    ++		test_when_finished "rm -rf explicit-hash-$hash" &&
    ++		git init --object-format=$hash explicit-hash-$hash &&
    ++		test_commit -C explicit-hash-$hash one &&
    ++		git -C explicit-hash-$hash rev-parse one >in &&
    ++		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in &&
     +		idx=$(echo explicit-hash-$hash/explicit-hash-$hash*.idx) &&
     +		nongit git show-index --object-format=$hash <"$idx" >actual &&
    -+		test_line_count = 1 actual &&
    -+
    -+		rm -rf explicit-hash-$hash
    ++		test_line_count = 1 actual
     +	'
     +done
     +
-- 
2.47.0.107.g34b6ce9b30

[PATCH v6 1/2] show-index: fix uninitialized hash function

[Abhijeet Sonar]

In c8aed5e8da (repository: stop setting SHA1 as the default object
hash), we got rid of the default hash algorithm for the_repository.
Due to this change, it is now the responsibility of the callers to set
their own default when this is not present.

As stated in the docs, show-index should use SHA1 as the default hash
algorithm when run outside a repository. Make sure this promise is
met by falling back to SHA1 when the_hash_algo is not present (i.e.
when the command is run outside a repository). Also add a test that
verifies this behavior.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
---
 builtin/show-index.c   | 9 +++++++++
 t/t5300-pack-object.sh | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/builtin/show-index.c b/builtin/show-index.c
index f164c01bbe..b5e337869d 100644
--- a/builtin/show-index.c
+++ b/builtin/show-index.c
@@ -38,6 +38,15 @@ int cmd_show_index(int argc,
 		repo_set_hash_algo(the_repository, hash_algo);
 	}
 
+	/*
+	 * Fallback to SHA1 if we are running outside of a repository.
+	 *
+	 * TODO: Figure out and implement a way to detect the hash algorithm in use by the
+	 *       the index file passed in and use that instead.
+	 */
+	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

[PATCH v6 2/2] t5300: add test for 'show-index --object-format'

[Abhijeet Sonar]

In 88a09a557c (builtin/show-index: provide options to determine hash
algo), the flag --object-format was added to show-index builtin as a way
to provide a hash algorithm explicitly. However, we do not have tests in
place for that functionality. Add them.

Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
---
 t/t5300-pack-object.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 51fed26cc4..bb6a22b438 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -527,6 +527,20 @@ test_expect_success SHA1 'show-index works OK outside a repository' '
 	nongit git show-index <foo.idx
 '
 
+for hash in sha1 sha256
+do
+	test_expect_success 'show-index works OK outside a repository with hash algo passed in via --object-format' '
+		test_when_finished "rm -rf explicit-hash-$hash" &&
+		git init --object-format=$hash explicit-hash-$hash &&
+		test_commit -C explicit-hash-$hash one &&
+		git -C explicit-hash-$hash rev-parse one >in &&
+		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in &&
+		idx=$(echo explicit-hash-$hash/explicit-hash-$hash*.idx) &&
+		nongit git show-index --object-format=$hash <"$idx" >actual &&
+		test_line_count = 1 actual
+	'
+done
+
 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

Re: [PATCH v6 0/2] show-index: fix uninitialized hash function

[Junio C Hamano]

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

> No, I don't think you are. However, I would like to point out that the code
> which I used as the inspiration also does thing the same way:
>
> 	test_expect_success 'pack-object <stdin parsing: [|--revs] with --stdin' '
> 		cat >in <<-EOF &&
> 		$(git -C pack-object-stdin rev-parse one)
> 		$(git -C pack-object-stdin rev-parse two)
> 		EOF

There is a huge difference between one and two, though ;-)  If you
expect that your new thing may later have to expect more than one
line of output, the way you wrote may be easier to extend, but if
you know it won't gain any more lines to its output, output from a
single command is better written without cat around it.

Re: [PATCH v6 0/2] show-index: fix uninitialized hash function

[Patrick Steinhardt]

On Sat, Nov 09, 2024 at 02:57:37PM +0530, Abhijeet Sonar wrote:
> That makes sense, applied.
> 
> Abhijeet Sonar (2):
>   show-index: fix uninitialized hash function
>   t5300: add test for 'show-index --object-format'
> 
>  builtin/show-index.c   |  9 +++++++++
>  t/t5300-pack-object.sh | 18 ++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> Range-diff against v5:
> 1:  05ee1e2ea5 = 1:  05ee1e2ea5 show-index: fix uninitialized hash function
> 2:  c8a28aae55 ! 2:  778f3ca18e t5300: add test for 'show-index --object-format'
>     @@ t/t5300-pack-object.sh: test_expect_success SHA1 'show-index works OK outside a
>       
>      +for hash in sha1 sha256
>      +do
>     -+	test_expect_success 'setup: show-index works OK outside a repository with hash algo passed in via --object-format' '
>     -+		git init explicit-hash-$hash --object-format=$hash &&
>     -+		test_commit -C explicit-hash-$hash one &&
>     -+
>     -+		cat >in <<-EOF &&
>     -+		$(git -C explicit-hash-$hash rev-parse one)
>     -+		EOF
>     -+
>     -+		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in
>     -+	'
>     -+
>      +	test_expect_success 'show-index works OK outside a repository with hash algo passed in via --object-format' '
>     ++		test_when_finished "rm -rf explicit-hash-$hash" &&
>     ++		git init --object-format=$hash explicit-hash-$hash &&
>     ++		test_commit -C explicit-hash-$hash one &&
>     ++		git -C explicit-hash-$hash rev-parse one >in &&
>     ++		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in &&
>      +		idx=$(echo explicit-hash-$hash/explicit-hash-$hash*.idx) &&
>      +		nongit git show-index --object-format=$hash <"$idx" >actual &&
>     -+		test_line_count = 1 actual &&
>     -+
>     -+		rm -rf explicit-hash-$hash
>     ++		test_line_count = 1 actual
>      +	'
>      +done
>      +

Thanks, this version looks good to me.

Patrick

Re: [PATCH v6 0/2] show-index: fix uninitialized hash function

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Sat, Nov 09, 2024 at 02:57:37PM +0530, Abhijeet Sonar wrote:
>> That makes sense, applied.
>> 
>> Abhijeet Sonar (2):
>>   show-index: fix uninitialized hash function
>>   t5300: add test for 'show-index --object-format'
>> 
>>  builtin/show-index.c   |  9 +++++++++
>>  t/t5300-pack-object.sh | 18 ++++++++++++++++++
>>  2 files changed, 27 insertions(+)
>> 
>> Range-diff against v5:
>> 1:  05ee1e2ea5 = 1:  05ee1e2ea5 show-index: fix uninitialized hash function
>> 2:  c8a28aae55 ! 2:  778f3ca18e t5300: add test for 'show-index --object-format'
>>     @@ t/t5300-pack-object.sh: test_expect_success SHA1 'show-index works OK outside a
>>       
>>      +for hash in sha1 sha256
>>      +do
>>     -+	test_expect_success 'setup: show-index works OK outside a repository with hash algo passed in via --object-format' '
>>     -+		git init explicit-hash-$hash --object-format=$hash &&
>>     -+		test_commit -C explicit-hash-$hash one &&
>>     -+
>>     -+		cat >in <<-EOF &&
>>     -+		$(git -C explicit-hash-$hash rev-parse one)
>>     -+		EOF
>>     -+
>>     -+		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in
>>     -+	'
>>     -+
>>      +	test_expect_success 'show-index works OK outside a repository with hash algo passed in via --object-format' '
>>     ++		test_when_finished "rm -rf explicit-hash-$hash" &&
>>     ++		git init --object-format=$hash explicit-hash-$hash &&
>>     ++		test_commit -C explicit-hash-$hash one &&
>>     ++		git -C explicit-hash-$hash rev-parse one >in &&
>>     ++		git -C explicit-hash-$hash pack-objects explicit-hash-$hash <in &&
>>      +		idx=$(echo explicit-hash-$hash/explicit-hash-$hash*.idx) &&
>>      +		nongit git show-index --object-format=$hash <"$idx" >actual &&
>>     -+		test_line_count = 1 actual &&
>>     -+
>>     -+		rm -rf explicit-hash-$hash
>>     ++		test_line_count = 1 actual
>>      +	'
>>      +done
>>      +
>
> Thanks, this version looks good to me.

Thanks, both.  Let me mark it for 'next', then.