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

[Abhijeet Sonar <abhijeet.nkt@gmail.com>]

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.