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

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

On 28/10/24 11:05, Patrick Steinhardt wrote:
> On Sun, Oct 27, 2024 at 08:10:16PM -0400, Taylor Blau wrote:
>> On Sat, Oct 26, 2024 at 05:39:50PM +0530, Abhijeet Sonar wrote:
>>> diff --git a/builtin/show-index.c b/builtin/show-index.c
>>> index f164c01bbe..978ae70470 100644
>>> --- a/builtin/show-index.c
>>> +++ b/builtin/show-index.c
>>> @@ -38,6 +38,9 @@ int cmd_show_index(int argc,
>>>  		repo_set_hash_algo(the_repository, hash_algo);
>>>  	}
>>>
>>> +	if (!the_hash_algo)
>>> +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> 
> Let's add a todo-comment here. The behaviour with this patch is somewhat
> broken as you cannot inspect indices that use any other object hash than
> SHA256 outside of a repository. This is fine from my point of view and
> nothing that you have to fix here, as you simply fix up the broken
> behaviour. But in the future, we should either:

I will add those comments, thanks.

> 
>   - Add logic to detect the format of the passed-in index and set that
>     up as the hash algorithm

I am very interested in hacking around and trying to implement this.
I read about the index format here:
https://git-scm.com/docs/index-format and (assuming I am looking at the
right thing) it does not seem like index files contain information about
the hash algorithm used in their headers or anywhere else. Are there
other leads I should follow?

> 
>   - If that is impossible, add a command line option to pick the hash
>     algo.
>
Actually there is already an option (--object-format) that allows
changing the hash algo used, it's just that default format is SHA1 when
one is not specified. (or at least will be, after this patch)

>>>  	hashsz = the_hash_algo->rawsz;
>>>
>>>  	if (fread(top_index, 2 * 4, 1, stdin) != 1)
>>> 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
>>> +'
> 
> So how does this behave with SHA256? Does it raise an error? Does it
> segfault?
> 
> I think it's okay to fail with SHA256 for now, but I'd like the
> failure behaviour to be cleanish. So I'd prefer to not skip the test
> completely, but adapt our expectations based on the hash algo. Or have
> two separate tests, one for each hash, that explicitly init the repo
> with `git init --ref-format=$hash`, and then exercise the behaviour for
> each of the>

Running show-index outside of a repository with a SHA256 based index
file gives:

$ git show-index <foo.idx
fatal: inconsistent 64b offset index

At the very least, it does not crash.

>>>  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 &&
>>> --
>>> 2.47.0.107.g34b6ce9b30
>>
>> These all look reasonable and as-expected to me. Patrick (CC'd) has been
>> reviewing similar changes elsewhere, so I'd like him to chime in as well
>> on whether or not this looks good to go.
> 
> Ah, thanks. I've missed this topic somehow.
> 
> Patrick

Thanks