From: Abhijeet Sonar <abhijeet.nkt@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: me@ttaylorr.com, git@vger.kernel.org, ps@pks.im,
sandals@crustytoothpaste.net
Subject: Re: [PATCH v4] show-index: fix uninitialized hash function
Date: Sat, 2 Nov 2024 21:56:20 +0530 [thread overview]
Message-ID: <74c0eddf-8bf9-4fb7-a0cd-edea8acaa938@gmail.com> (raw)
In-Reply-To: <xmqq1pzuylm6.fsf@gitster.g>
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.
next prev parent reply other threads:[~2024-11-02 16:26 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-12 14:23 [PATCH] show-index: fix uninitialized hash function Abhijeet Sonar
2024-07-12 15:35 ` Junio C Hamano
2024-07-15 10:23 ` [PATCH v2] " Abhijeet Sonar
2024-07-15 16:22 ` Re* " Junio C Hamano
2024-10-26 12:09 ` [PATCH v3] " Abhijeet Sonar
2024-10-28 0:10 ` Taylor Blau
2024-10-28 5:35 ` Patrick Steinhardt
2024-10-28 17:42 ` Taylor Blau
2024-11-01 17:28 ` [PATCH v4] " Abhijeet Sonar
2024-11-02 10:29 ` Junio C Hamano
2024-11-02 16:26 ` Abhijeet Sonar [this message]
2024-11-04 19:29 ` [PATCH v5 0/2] " Abhijeet Sonar
2024-11-04 19:29 ` [PATCH v5 1/2] " Abhijeet Sonar
2024-11-04 19:29 ` [PATCH v5 2/2] t5300: add test for 'show-index --object-format' Abhijeet Sonar
2024-11-05 1:19 ` Junio C Hamano
2024-11-09 9:27 ` [PATCH v6 0/2] show-index: fix uninitialized hash function Abhijeet Sonar
2024-11-09 9:27 ` [PATCH v6 1/2] " Abhijeet Sonar
2024-11-09 9:27 ` [PATCH v6 2/2] t5300: add test for 'show-index --object-format' Abhijeet Sonar
2024-11-11 3:16 ` [PATCH v6 0/2] show-index: fix uninitialized hash function Junio C Hamano
2024-12-16 8:11 ` Patrick Steinhardt
2024-12-16 16:21 ` Junio C Hamano
2024-10-29 11:54 ` [PATCH v3] " Abhijeet Sonar
2024-10-29 10:30 ` Abhijeet Sonar
2024-10-26 12:17 ` Re* [PATCH v2] " Abhijeet Sonar
2024-07-15 22:07 ` brian m. carlson
2024-07-15 10:31 ` [PATCH] " Abhijeet Sonar
2024-07-12 16:53 ` Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=74c0eddf-8bf9-4fb7-a0cd-edea8acaa938@gmail.com \
--to=abhijeet.nkt@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=ps@pks.im \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).