Re: [PATCH 2/3] ieot: default to not writing IEOT section

[Ben Peart <peartben@gmail.com>]

On 11/13/2018 4:08 PM, Jonathan Nieder wrote:
> Hi again,
> 
> Ben Peart wrote:
>> On 11/13/2018 1:18 PM, Jonathan Nieder wrote:
>>> Ben Peart wrote:
> 
>>>> Why introduce a new setting to disable writing the IEOT extension instead of
>>>> just using the existing index.threads setting?  If index.threads=1 then the
>>>> IEOT extension isn't written which (I believe) will accomplish the same
>>>> goal.
>>>
>>> Do you mean defaulting to index.threads=1?  I don't think that would
>>> be a good default, but if you have a different change in mind then I'd
>>> be happy to hear it.
>>>
>>> Or do you mean that if the user has explicitly specified index.threads=true,
>>> then that should imply index.recordOffsetTable=true so users only have
>>> to set one setting to turn it on?  I can imagine that working well.
>>
>> Reading the index with multiple threads requires the EOIE and IEOT
>> extensions to exist in the index.  If either extension doesn't exist, then
>> the code falls back to the single threaded path.  That means you can't have
>> both 1) no warning for old versions of git and 2) multi-threaded reading for
>> new versions of git.
>>
>> If you set index.threads=1, that will prevent the IEOT extension from being
>> written and there will be no "ignoring IEOT extension" warning in older
>> versions of git.
>>
>> With this patch 'as is' you would have to set both index.threads=true and
>> index.recordOffsetTable=true to get multi-threaded index reads.  If either
>> is set to false, it will silently drop back to single threaded reads.
> 
> Sorry, I'm still not understanding what you're proposing.  What would be
> 
> - the default behavior
> - the mechanism for changing that behavior
> 
> under your proposal?
> 
> I consider index.threads=1 to be a bad default.  I would understand if
> you are saying that that should be the default, and I tried to propose
> a different way to achieve what you're looking for in the quoted reply
> above (but I'm not understanding whether you like that proposal or
> not).
> 

Today, both the write logic (ie should we write out the IEOT extension) 
and the read logic (should I use the IEOT, if available, and do 
multi-threaded reading) are controlled by the single "index.threads" 
setting.  I would like to keep the settings as simple as possible to 
prevent user confusion.

If we have two different settings (index.threads and 
index.recordoffsettable) then the only combination that will result in 
the user actually getting multi-threaded reads is when they are both set 
to true.  Any other combination will silently fail.  I think it would be 
confusing if you set index.threads=true and got no error message but 
didn't get multi-threaded reads either (or vice versa).

If you want to prevent any of the scary "ignoring IEOT extension" from 
ever happening then your only option is to turn off the IEOT writing by 
default.  The downside is that people have to discover and turn it on if 
they want the improvements.  This can be achieved by changing the 
default for index.threads from "true" to "false."

diff --git a/config.c b/config.c
index 2ee29f6f86..86f5c14294 100644
--- a/config.c
+++ b/config.c
@@ -2291,7 +2291,7 @@ int git_config_get_fsmonitor(void)

  int git_config_get_index_threads(void)
  {
-       int is_bool, val = 0;
+       int is_bool, val = 1;

         val = git_env_ulong("GIT_TEST_INDEX_THREADS", 0);
         if (val)


If you want to provide a way for a concerned user to disable the message 
after the first time they have seen it, then they can be instructed to 
run 'git config --global index.threads false'

There is no way to get multi-threaded reads and NOT get the scary 
message with older versions of git.  Multi-threaded reads require the 
IEOT extension to be written into the index and the existence of the 
IEOT extension in the index will always generate the scary warning.

> Jonathan
>