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

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