On 02/11/24 15:59, Junio C Hamano wrote:
> Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
