| Message ID | 20181120061221.GC144753@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid confusing messages from new index extensions | expand |
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; > > /* >
> > +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; > > +}
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; >>> +}
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
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.
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; /*
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(-)