[2/5] ieot: default to not writing IEOT section
diff mbox series

Message ID 20181120061221.GC144753@google.com
State New
Headers show
Series
  • Avoid confusing messages from new index extensions
Related show

Commit Message

Jonathan Nieder Nov. 20, 2018, 6:12 a.m. UTC
As with EOIE, popular versions of Git do not support the new IEOT
extension yet.  When accessing a Git repository written by a more
modern version of Git, they correctly ignore the unrecognized section,
but in the process they loudly warn

	ignoring IEOT extension

resulting in confusion for users.  Introduce the index extension more
gently by not writing it yet in this first version with support for
it.  Soon, once sufficiently many users are running a modern version
of Git, we can flip the default so users benefit from this index
extension by default.

Introduce a '[index] recordOffsetTable' configuration variable to
control whether the new index extension is written.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
As with patch 1/5, no change from v1 other than rebasing.

 Documentation/config/index.txt |  7 +++++++
 read-cache.c                   | 11 ++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Ben Peart Nov. 20, 2018, 1:07 p.m. UTC | #1
On 11/20/2018 1:12 AM, Jonathan Nieder wrote:
> As with EOIE, popular versions of Git do not support the new IEOT
> extension yet.  When accessing a Git repository written by a more
> modern version of Git, they correctly ignore the unrecognized section,
> but in the process they loudly warn
> 
> 	ignoring IEOT extension
> 
> resulting in confusion for users.  Introduce the index extension more
> gently by not writing it yet in this first version with support for
> it.  Soon, once sufficiently many users are running a modern version
> of Git, we can flip the default so users benefit from this index
> extension by default.
> 
> Introduce a '[index] recordOffsetTable' configuration variable to
> control whether the new index extension is written.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> As with patch 1/5, no change from v1 other than rebasing.
> 
>   Documentation/config/index.txt |  7 +++++++
>   read-cache.c                   | 11 ++++++++++-
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config/index.txt b/Documentation/config/index.txt
> index 8e138aba7a..de44183235 100644
> --- a/Documentation/config/index.txt
> +++ b/Documentation/config/index.txt
> @@ -5,6 +5,13 @@ index.recordEndOfIndexEntries::
>   	reading the index using Git versions before 2.20. Defaults to
>   	'false'.
>   
> +index.recordOffsetTable::
> +	Specifies whether the index file should include an "Index Entry
> +	Offset Table" section. This reduces index load time on
> +	multiprocessor machines but produces a message "ignoring IEOT
> +	extension" when reading the index using Git versions before 2.20.
> +	Defaults to 'false'.
> +
>   index.threads::
>   	Specifies the number of threads to spawn when loading the index.
>   	This is meant to reduce index load time on multiprocessor machines.
> diff --git a/read-cache.c b/read-cache.c
> index 1e9c772603..f3d5638d9e 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2698,6 +2698,15 @@ static int record_eoie(void)
>   	return 0;
>   }
>   
> +static int record_ieot(void)
> +{
> +	int val;
> +

Initialize stack val to zero to ensure proper default.

> +	if (!git_config_get_bool("index.recordoffsettable", &val))
> +		return val;
> +	return 0;
> +}
> +
>   /*
>    * On success, `tempfile` is closed. If it is the temporary file
>    * of a `struct lock_file`, we will therefore effectively perform
> @@ -2761,7 +2770,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   	else
>   		nr_threads = 1;
>   
> -	if (nr_threads != 1) {
> +	if (nr_threads != 1 && record_ieot()) {
>   		int ieot_blocks, cpus;
>   
>   		/*
>
Stefan Beller Nov. 26, 2018, 7:59 p.m. UTC | #2
> > +static int record_ieot(void)
> > +{
> > +     int val;
> > +
>
> Initialize stack val to zero to ensure proper default.

I don't think that is needed here, as we only use `val` when
we first write to it via git_config_get_bool.

Did you spot this via code review and thought of
defensive programming or is there a tool that
has a false positive here?

>
> > +     if (!git_config_get_bool("index.recordoffsettable", &val))
> > +             return val;
> > +     return 0;
> > +}
Ben Peart Nov. 26, 2018, 9:47 p.m. UTC | #3
On 11/26/2018 2:59 PM, Stefan Beller wrote:
>>> +static int record_ieot(void)
>>> +{
>>> +     int val;
>>> +
>>
>> Initialize stack val to zero to ensure proper default.
> 
> I don't think that is needed here, as we only use `val` when
> we first write to it via git_config_get_bool.
> 
> Did you spot this via code review and thought of
> defensive programming or is there a tool that
> has a false positive here?
> 

Code review and defensive programming.  I had to review the code in 
git_config_get_bool() to see if it always initialized the val even if it 
didn't find the requested config variable (esp since we don't pass in a 
default value for this function like we do others).

>>
>>> +     if (!git_config_get_bool("index.recordoffsettable", &val))
>>> +             return val;
>>> +     return 0;
>>> +}
Stefan Beller Nov. 26, 2018, 10:02 p.m. UTC | #4
On Mon, Nov 26, 2018 at 1:48 PM Ben Peart <peartben@gmail.com> wrote:
>
>
>
> On 11/26/2018 2:59 PM, Stefan Beller wrote:
> >>> +static int record_ieot(void)
> >>> +{
> >>> +     int val;
> >>> +
> >>
> >> Initialize stack val to zero to ensure proper default.
> >
> > I don't think that is needed here, as we only use `val` when
> > we first write to it via git_config_get_bool.
> >
> > Did you spot this via code review and thought of
> > defensive programming or is there a tool that
> > has a false positive here?
> >
>
> Code review and defensive programming.  I had to review the code in
> git_config_get_bool() to see if it always initialized the val even if it
> didn't find the requested config variable (esp since we don't pass in a
> default value for this function like we do others).
>

Ah, sorry to have sent out this email, which I found as one of the
earliest discussions in my mailbox. The later patches/discussions
became a lot more heated from my cursory skimming and sorted
out this as well.

It is interesting to notice that, as I also had to lookup how the config
machinery works (once? a couple times?) but now it is so hardcoded
in my brain to assume that if functions like git_config_* take the
branch, we can access the value that the config function was supposed
to read into.

Sorry for the noise,
Stefan
Junio C Hamano Nov. 27, 2018, 12:50 a.m. UTC | #5
Stefan Beller <sbeller@google.com> writes:

>> > +static int record_ieot(void)
>> > +{
>> > +     int val;
>> > +
>>
>> Initialize stack val to zero to ensure proper default.
>
> I don't think that is needed here, as we only use `val` when
> we first write to it via git_config_get_bool.

Yup.

Patch
diff mbox series

diff --git a/Documentation/config/index.txt b/Documentation/config/index.txt
index 8e138aba7a..de44183235 100644
--- a/Documentation/config/index.txt
+++ b/Documentation/config/index.txt
@@ -5,6 +5,13 @@  index.recordEndOfIndexEntries::
 	reading the index using Git versions before 2.20. Defaults to
 	'false'.
 
+index.recordOffsetTable::
+	Specifies whether the index file should include an "Index Entry
+	Offset Table" section. This reduces index load time on
+	multiprocessor machines but produces a message "ignoring IEOT
+	extension" when reading the index using Git versions before 2.20.
+	Defaults to 'false'.
+
 index.threads::
 	Specifies the number of threads to spawn when loading the index.
 	This is meant to reduce index load time on multiprocessor machines.
diff --git a/read-cache.c b/read-cache.c
index 1e9c772603..f3d5638d9e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2698,6 +2698,15 @@  static int record_eoie(void)
 	return 0;
 }
 
+static int record_ieot(void)
+{
+	int val;
+
+	if (!git_config_get_bool("index.recordoffsettable", &val))
+		return val;
+	return 0;
+}
+
 /*
  * On success, `tempfile` is closed. If it is the temporary file
  * of a `struct lock_file`, we will therefore effectively perform
@@ -2761,7 +2770,7 @@  static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	else
 		nr_threads = 1;
 
-	if (nr_threads != 1) {
+	if (nr_threads != 1 && record_ieot()) {
 		int ieot_blocks, cpus;
 
 		/*