From: Lessley Dennington <lessleydennington@gmail.com>
Date: Thu, 04 Nov 2021 23:55:06 GMT
Subject: Re: [PATCH v3 1/2] diff: enable and test the sparse index
Message-ID: <1a269d05-49f6-6c49-860a-044070f14ffc@gmail.com>
In-Reply-To: <xmqqcznh8913.fsf@gitster.g>

On 11/3/21 10:05 AM, Junio C Hamano wrote:
> "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> 2000.34: git diff --staged (full-v3)      0.08    0.08 +0.0%
>> 2000.35: git diff --staged (full-v4)      0.08    0.08 +0.0%
>> 2000.36: git diff --staged (sparse-v3)    0.17    0.04 -76.5%
>> 2000.37: git diff --staged (sparse-v4)    0.16    0.04 -75.0%
> 
> Please do not add more use of the synonym to the test suite, other
> than the one that makes sure the synonym works the same way as the
> real option, which is "--cached".
>

Thank you, changed for v4.

>> diff --git a/builtin/diff.c b/builtin/diff.c
>> index dd8ce688ba7..cbf7b51c7c0 100644
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
>> @@ -437,6 +437,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>>   
>>   	prefix = setup_git_directory_gently(&nongit);
>>   
>> +	prepare_repo_settings(the_repository);
>> +	the_repository->settings.command_requires_full_index = 0;
>> +
> 
> Doesn't the code need to be protected with
> 
> 	if (!nongit) {
> 		prepare_repo_settings(the_repository);
> 		the_repository->settings.command_requires_full_index = 0;
> 	}
> 
> at the very least?  It may be that the code is getting lucky because
> the_repository may be initialized with a random value (after all,
> when we are not in a repository, there is nowhere to read the
> on-disk settings from) and we may even be able to set a bit in the
> settings structure without crashing, but conceptually, doing the
> above when we _know_ we are not in any repository is simply wrong.
> 
> I wonder if prepare_repo_settings() needs be more strict.  For
> example, shouldn't it check if we have a repository to begin with
> and BUG() if it was called when there is not a repository?  After
> all, it tries to read from the repository configuration file, so any
> necessary set-up to discover where the gitdir is must have been done
> already before it can be called.
> 
> With such a safety feature to catch a programmer errors, perhaps the
> above could have been caught before the patch hit the list.
> 
> Thoughts?  Am I missing some chicken-and-egg situation where
> prepare_repo_settings() must be callable before we know where the
> repository is, or something, which justifies why the function is so
> loose in its sanity checks in the current form?
> 
> 

This seems like a good idea. I've added both the nongit check and the 
prepare_repo_settings() updates you've suggested for v4, pending review 
by my team.

Best,
Lessley