git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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.




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