could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Luke Diamand]

I've recently started seeing a lot of this message when doing a rebase:

   warning: could not freshen shared index
'/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'

(There's a repo called dev_full, and I've got a worktree where I'm
working on my 3rd attempt to make it work with gcc8).

That file doesn't actually exist but there are a bunch of
sharedindex.XXX files in there with more convincing looking names.

2.20.1.611.gfbb209baf1

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Christian Couder]

On Wed, Feb 6, 2019 at 11:25 AM Luke Diamand <luke@diamand.org> wrote:
>
> I've recently started seeing a lot of this message when doing a rebase:
>
>    warning: could not freshen shared index
> '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'

Thanks for the report!

Have you tried to run the test suite with GIT_TEST_SPLIT_INDEX set to
"true" on your machine/environment?
Also does the rebase fail or otherwise misbehave?

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Duy Nguyen]

On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote:
> I've recently started seeing a lot of this message when doing a rebase:
> 
>    warning: could not freshen shared index
> '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'

There are only two places in the code that could print this. The one
in read_index_from() can't happen unless is_null_oid() is broken (very
very unlikely).

The other one is in write_locked_index() which could happen in theory
but I don't understand how it got there. If you could build git, could
you try this patch and see if it helps?

-- 8< --
diff --git a/read-cache.c b/read-cache.c
index f68b367613..5ad71478dc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3165,6 +3165,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 		fill_fsmonitor_bitmap(istate);
 
 	if (!si || alternate_index_output ||
+	    (si && is_null_oid(&si->base_oid)) ||
 	    (istate->cache_changed & ~EXTMASK)) {
 		if (si)
 			oidclr(&si->base_oid);
-- 8< --


> (There's a repo called dev_full, and I've got a worktree where I'm
> working on my 3rd attempt to make it work with gcc8).
> 
> That file doesn't actually exist but there are a bunch of
> sharedindex.XXX files in there with more convincing looking names.
> 
> 2.20.1.611.gfbb209baf1
--
Duy

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Luke Diamand]

On Fri, 8 Feb 2019 at 10:02, Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote:
> > I've recently started seeing a lot of this message when doing a rebase:
> >
> >    warning: could not freshen shared index
> > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'
>
> There are only two places in the code that could print this. The one
> in read_index_from() can't happen unless is_null_oid() is broken (very
> very unlikely).
>
> The other one is in write_locked_index() which could happen in theory
> but I don't understand how it got there. If you could build git, could
> you try this patch and see if it helps?

They've gone away!


>
> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index f68b367613..5ad71478dc 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3165,6 +3165,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>                 fill_fsmonitor_bitmap(istate);
>
>         if (!si || alternate_index_output ||
> +           (si && is_null_oid(&si->base_oid)) ||
>             (istate->cache_changed & ~EXTMASK)) {
>                 if (si)
>                         oidclr(&si->base_oid);
> -- 8< --
>

Luke

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote:
>> I've recently started seeing a lot of this message when doing a rebase:
>> 
>>    warning: could not freshen shared index
>> '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'
>
> There are only two places in the code that could print this. The one
> in read_index_from() can't happen unless is_null_oid() is broken (very
> very unlikely).
>
> The other one is in write_locked_index() which could happen in theory
> but I don't understand how it got there. If you could build git, could
> you try this patch and see if it helps?

... meaning, if it hides the symptom, we'd know the codepath that
causes a NULL si->base_oid to appear here is the culprit?  Or do you
mean that it is expected si->base_oid sometimes is NULL and we should
have been pretending as if si were NULL (i.e. split_index is not being
used)?

I take it as the latter (i.e. "helps" to narrow down the bug hunting
field, not "helps" by fixing the bug).

Thanks.

>
> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index f68b367613..5ad71478dc 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3165,6 +3165,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>  		fill_fsmonitor_bitmap(istate);
>  
>  	if (!si || alternate_index_output ||
> +	    (si && is_null_oid(&si->base_oid)) ||
>  	    (istate->cache_changed & ~EXTMASK)) {
>  		if (si)
>  			oidclr(&si->base_oid);
> -- 8< --
>
>
>> (There's a repo called dev_full, and I've got a worktree where I'm
>> working on my 3rd attempt to make it work with gcc8).
>> 
>> That file doesn't actually exist but there are a bunch of
>> sharedindex.XXX files in there with more convincing looking names.
>> 
>> 2.20.1.611.gfbb209baf1
> --
> Duy

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Duy Nguyen]

On Sat, Feb 9, 2019 at 12:24 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote:
> >> I've recently started seeing a lot of this message when doing a rebase:
> >>
> >>    warning: could not freshen shared index
> >> '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'
> >
> > There are only two places in the code that could print this. The one
> > in read_index_from() can't happen unless is_null_oid() is broken (very
> > very unlikely).
> >
> > The other one is in write_locked_index() which could happen in theory
> > but I don't understand how it got there. If you could build git, could
> > you try this patch and see if it helps?
>
> ... meaning, if it hides the symptom, we'd know the codepath that
> causes a NULL si->base_oid to appear here is the culprit?  Or do you
> mean that it is expected si->base_oid sometimes is NULL and we should
> have been pretending as if si were NULL (i.e. split_index is not being
> used)?

I didn't go that far when I suggested my patch. Looking more at the
code, I think we may have written the "link" extension with null oid.
Which could trigger this case and is definitely wrong.

> I take it as the latter (i.e. "helps" to narrow down the bug hunting
> field, not "helps" by fixing the bug).

Yeah definitely not the fix (how can I write "I don't know why" in the
commit message for the fix). At least now I know I'm on the right
track.

Szeder recently fixed some racy bug in split-index.c, which seems fit
in this rebase scenario since we'll be updating the index very often
in a short period of time. Perhaps that uncovers some case that we
don't handle well. I haven't been able to connect the dots though.
-- 
Duy

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Duy Nguyen]

On Fri, Feb 8, 2019 at 11:39 PM Luke Diamand <luke@diamand.org> wrote:
>
> On Fri, 8 Feb 2019 at 10:02, Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote:
> > > I've recently started seeing a lot of this message when doing a rebase:
> > >
> > >    warning: could not freshen shared index
> > > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'
> >
> > There are only two places in the code that could print this. The one
> > in read_index_from() can't happen unless is_null_oid() is broken (very
> > very unlikely).
> >
> > The other one is in write_locked_index() which could happen in theory
> > but I don't understand how it got there. If you could build git, could
> > you try this patch and see if it helps?
>
> They've gone away!

Great! Since you seem able to reproduce this (and can build git!)
could you try bisect to pin point the commit that causes this problem?
That would help a lot. I think you could start maybe from v2.19.0
-- 
Duy

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Luke Diamand]

On Sat, 9 Feb 2019 at 05:01, Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Fri, Feb 8, 2019 at 11:39 PM Luke Diamand <luke@diamand.org> wrote:
> >
> > On Fri, 8 Feb 2019 at 10:02, Duy Nguyen <pclouds@gmail.com> wrote:
> > >
> > > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote:
> > > > I've recently started seeing a lot of this message when doing a rebase:
> > > >
> > > >    warning: could not freshen shared index
> > > > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'
> > >
> > > There are only two places in the code that could print this. The one
> > > in read_index_from() can't happen unless is_null_oid() is broken (very
> > > very unlikely).
> > >
> > > The other one is in write_locked_index() which could happen in theory
> > > but I don't understand how it got there. If you could build git, could
> > > you try this patch and see if it helps?
> >
> > They've gone away!
>
> Great! Since you seem able to reproduce this (and can build git!)
> could you try bisect to pin point the commit that causes this problem?
> That would help a lot. I think you could start maybe from v2.19.0

The first bad commit was d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2.

$ git show d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2
commit d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2 (refs/bisect/bad)
Merge: 6b0f1d9c47 7db118303a
Author: Junio C Hamano <gitster@pobox.com>
Date:   Wed May 23 14:38:22 2018 +0900

    Merge branch 'en/unpack-trees-split-index-fix'

    The split-index feature had a long-standing and dormant bug in
    certain use of the in-core merge machinery, which has been fixed.

    * en/unpack-trees-split-index-fix:
      unpack_trees: fix breakage when o->src_index != o->dst_index

The test I'm doing is just:

1. git checkout some_tag
2. git rebase -i HEAD~5
3. Swap the top and bottom commit
4. repeat

I just chose "5" as my first wild guess, other numbers are also available.

With "5" I get 3 lots of:
   warning: could not freshen shared index
'.git/sharedindex.0000000000000000000000000000000000000000'

As far as I can tell the actual rebasing is fine.

Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000'

[Duy Nguyen]

On Sat, Feb 9, 2019 at 4:58 PM Luke Diamand <luke@diamand.org> wrote:
>
> On Sat, 9 Feb 2019 at 05:01, Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Fri, Feb 8, 2019 at 11:39 PM Luke Diamand <luke@diamand.org> wrote:
> > >
> > > On Fri, 8 Feb 2019 at 10:02, Duy Nguyen <pclouds@gmail.com> wrote:
> > > >
> > > > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote:
> > > > > I've recently started seeing a lot of this message when doing a rebase:
> > > > >
> > > > >    warning: could not freshen shared index
> > > > > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000'
> > > >
> > > > There are only two places in the code that could print this. The one
> > > > in read_index_from() can't happen unless is_null_oid() is broken (very
> > > > very unlikely).
> > > >
> > > > The other one is in write_locked_index() which could happen in theory
> > > > but I don't understand how it got there. If you could build git, could
> > > > you try this patch and see if it helps?
> > >
> > > They've gone away!
> >
> > Great! Since you seem able to reproduce this (and can build git!)
> > could you try bisect to pin point the commit that causes this problem?
> > That would help a lot. I think you could start maybe from v2.19.0
>
> The first bad commit was d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2.

Thanks! I could reproduce it and I think I know what the problem is
now. Patches coming soon.

>
> $ git show d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2
> commit d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2 (refs/bisect/bad)
> Merge: 6b0f1d9c47 7db118303a
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Wed May 23 14:38:22 2018 +0900
>
>     Merge branch 'en/unpack-trees-split-index-fix'
>
>     The split-index feature had a long-standing and dormant bug in
>     certain use of the in-core merge machinery, which has been fixed.
>
>     * en/unpack-trees-split-index-fix:
>       unpack_trees: fix breakage when o->src_index != o->dst_index
>
> The test I'm doing is just:
>
> 1. git checkout some_tag
> 2. git rebase -i HEAD~5
> 3. Swap the top and bottom commit
> 4. repeat
>
> I just chose "5" as my first wild guess, other numbers are also available.
>
> With "5" I get 3 lots of:
>    warning: could not freshen shared index
> '.git/sharedindex.0000000000000000000000000000000000000000'
>
> As far as I can tell the actual rebasing is fine.



-- 
Duy

[PATCH] unpack-trees.c: fix writing "link" index ext with null base oid

[Nguyễn Thái Ngọc Duy]

Since commit 7db118303a (unpack_trees: fix breakage when o->src_index !=
o->dst_index - 2018-04-23) and changes in merge code to use separate
index_state for source and destination, when doing a merge with split
index activated, we may run into this line in unpack_trees():

    o->result.split_index = init_split_index(&o->result);

This is by itself not wrong. But this split index information is not
fully populated (and it's only so when move_cache_to_base_index() is
called, aka force splitting the index, or loading index_state from a
file). Both "base_oid" and "base" in this case remain null.

So when writing the main index down, we link to this index with null
oid (default value after init_split_index()), which also means "no split
index" internally. This triggers an incorrect base index refresh:

    warning: could not freshen shared index '.../sharedindex.0{40}'

This patch makes sure we will not refresh null base_oid (because the
file is never there). It also makes sure not to write "link" extension
with null base_oid in the first place (no point having it at
all). Read code already has protection against null base_oid.

There is also another side fix in remove_split_index() that causes a
crash when doing "git update-index --no-split-index" when base_oid in
the index file is null. In this case we will not load
istate->split_index->base but we dereference it anyway and are rewarded
with a segfault. This should not happen anymore, but it's still wrong to
dereference a potential NULL pointer, especially when we do check for
NULL pointer in the next code.

Reported-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I considered adding a test, but since the problem is a warning, not
 sure how to catch that. And a test would not be able to verify all
 changes in this patch anyway.

 read-cache.c  |  5 +++--
 split-index.c | 34 ++++++++++++++++++----------------
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 0e0c93edc9..d6fb09984f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2894,7 +2894,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			return -1;
 	}
 
-	if (!strip_extensions && istate->split_index) {
+	if (!strip_extensions && istate->split_index &&
+	    !is_null_oid(&istate->split_index->base_oid)) {
 		struct strbuf sb = STRBUF_INIT;
 
 		err = write_link_extension(&sb, istate) < 0 ||
@@ -3189,7 +3190,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	ret = write_split_index(istate, lock, flags);
 
 	/* Freshen the shared index only if the split-index was written */
-	if (!ret && !new_shared_index) {
+	if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) {
 		const char *shared_index = git_path("sharedindex.%s",
 						    oid_to_hex(&si->base_oid));
 		freshen_shared_index(shared_index, 1);
diff --git a/split-index.c b/split-index.c
index 5820412dc5..a9d13611a4 100644
--- a/split-index.c
+++ b/split-index.c
@@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate)
 void remove_split_index(struct index_state *istate)
 {
 	if (istate->split_index) {
-		/*
-		 * When removing the split index, we need to move
-		 * ownership of the mem_pool associated with the
-		 * base index to the main index. There may be cache entries
-		 * allocated from the base's memory pool that are shared with
-		 * the_index.cache[].
-		 */
-		mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
+		if (istate->split_index->base) {
+			/*
+			 * When removing the split index, we need to move
+			 * ownership of the mem_pool associated with the
+			 * base index to the main index. There may be cache entries
+			 * allocated from the base's memory pool that are shared with
+			 * the_index.cache[].
+			 */
+			mem_pool_combine(istate->ce_mem_pool,
+					 istate->split_index->base->ce_mem_pool);
 
-		/*
-		 * The split index no longer owns the mem_pool backing
-		 * its cache array. As we are discarding this index,
-		 * mark the index as having no cache entries, so it
-		 * will not attempt to clean up the cache entries or
-		 * validate them.
-		 */
-		if (istate->split_index->base)
+			/*
+			 * The split index no longer owns the mem_pool backing
+			 * its cache array. As we are discarding this index,
+			 * mark the index as having no cache entries, so it
+			 * will not attempt to clean up the cache entries or
+			 * validate them.
+			 */
 			istate->split_index->base->cache_nr = 0;
+		}
 
 		/*
 		 * We can discard the split index because its
-- 
2.20.1.682.gd5861c6d90

Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid

[Luke Diamand]

On Sat, 9 Feb 2019 at 11:23, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> Since commit 7db118303a (unpack_trees: fix breakage when o->src_index !=
> o->dst_index - 2018-04-23) and changes in merge code to use separate
> index_state for source and destination, when doing a merge with split
> index activated, we may run into this line in unpack_trees():
>
>     o->result.split_index = init_split_index(&o->result);
>
> This is by itself not wrong. But this split index information is not
> fully populated (and it's only so when move_cache_to_base_index() is
> called, aka force splitting the index, or loading index_state from a
> file). Both "base_oid" and "base" in this case remain null.
>
> So when writing the main index down, we link to this index with null
> oid (default value after init_split_index()), which also means "no split
> index" internally. This triggers an incorrect base index refresh:
>
>     warning: could not freshen shared index '.../sharedindex.0{40}'
>
> This patch makes sure we will not refresh null base_oid (because the
> file is never there). It also makes sure not to write "link" extension
> with null base_oid in the first place (no point having it at
> all). Read code already has protection against null base_oid.
>
> There is also another side fix in remove_split_index() that causes a
> crash when doing "git update-index --no-split-index" when base_oid in
> the index file is null. In this case we will not load
> istate->split_index->base but we dereference it anyway and are rewarded
> with a segfault. This should not happen anymore, but it's still wrong to
> dereference a potential NULL pointer, especially when we do check for
> NULL pointer in the next code.
>
> Reported-by: Luke Diamand <luke@diamand.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  I considered adding a test, but since the problem is a warning, not
>  sure how to catch that. And a test would not be able to verify all
>  changes in this patch anyway.
>
>  read-cache.c  |  5 +++--
>  split-index.c | 34 ++++++++++++++++++----------------
>  2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 0e0c93edc9..d6fb09984f 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2894,7 +2894,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>                         return -1;
>         }
>
> -       if (!strip_extensions && istate->split_index) {
> +       if (!strip_extensions && istate->split_index &&
> +           !is_null_oid(&istate->split_index->base_oid)) {
>                 struct strbuf sb = STRBUF_INIT;
>
>                 err = write_link_extension(&sb, istate) < 0 ||
> @@ -3189,7 +3190,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>         ret = write_split_index(istate, lock, flags);
>
>         /* Freshen the shared index only if the split-index was written */
> -       if (!ret && !new_shared_index) {
> +       if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) {
>                 const char *shared_index = git_path("sharedindex.%s",
>                                                     oid_to_hex(&si->base_oid));
>                 freshen_shared_index(shared_index, 1);
> diff --git a/split-index.c b/split-index.c
> index 5820412dc5..a9d13611a4 100644
> --- a/split-index.c
> +++ b/split-index.c
> @@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate)
>  void remove_split_index(struct index_state *istate)
>  {
>         if (istate->split_index) {
> -               /*
> -                * When removing the split index, we need to move
> -                * ownership of the mem_pool associated with the
> -                * base index to the main index. There may be cache entries
> -                * allocated from the base's memory pool that are shared with
> -                * the_index.cache[].
> -                */
> -               mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
> +               if (istate->split_index->base) {
> +                       /*
> +                        * When removing the split index, we need to move
> +                        * ownership of the mem_pool associated with the
> +                        * base index to the main index. There may be cache entries
> +                        * allocated from the base's memory pool that are shared with
> +                        * the_index.cache[].
> +                        */
> +                       mem_pool_combine(istate->ce_mem_pool,
> +                                        istate->split_index->base->ce_mem_pool);
>
> -               /*
> -                * The split index no longer owns the mem_pool backing
> -                * its cache array. As we are discarding this index,
> -                * mark the index as having no cache entries, so it
> -                * will not attempt to clean up the cache entries or
> -                * validate them.
> -                */
> -               if (istate->split_index->base)
> +                       /*
> +                        * The split index no longer owns the mem_pool backing
> +                        * its cache array. As we are discarding this index,
> +                        * mark the index as having no cache entries, so it
> +                        * will not attempt to clean up the cache entries or
> +                        * validate them.
> +                        */
>                         istate->split_index->base->cache_nr = 0;
> +               }
>
>                 /*
>                  * We can discard the split index because its
> --
> 2.20.1.682.gd5861c6d90

Ack, I no longer see those messages.


>

Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid

[Elijah Newren]

On Sat, Feb 9, 2019 at 3:23 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> Since commit 7db118303a (unpack_trees: fix breakage when o->src_index !=
> o->dst_index - 2018-04-23) and changes in merge code to use separate
> index_state for source and destination, when doing a merge with split
> index activated, we may run into this line in unpack_trees():
>
>     o->result.split_index = init_split_index(&o->result);
>
> This is by itself not wrong. But this split index information is not
> fully populated (and it's only so when move_cache_to_base_index() is
> called, aka force splitting the index, or loading index_state from a
> file). Both "base_oid" and "base" in this case remain null.
>
> So when writing the main index down, we link to this index with null
> oid (default value after init_split_index()), which also means "no split
> index" internally. This triggers an incorrect base index refresh:
>
>     warning: could not freshen shared index '.../sharedindex.0{40}'
>
> This patch makes sure we will not refresh null base_oid (because the
> file is never there). It also makes sure not to write "link" extension
> with null base_oid in the first place (no point having it at
> all). Read code already has protection against null base_oid.
>
> There is also another side fix in remove_split_index() that causes a
> crash when doing "git update-index --no-split-index" when base_oid in
> the index file is null. In this case we will not load
> istate->split_index->base but we dereference it anyway and are rewarded
> with a segfault. This should not happen anymore, but it's still wrong to
> dereference a potential NULL pointer, especially when we do check for
> NULL pointer in the next code.
>
> Reported-by: Luke Diamand <luke@diamand.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Thanks for digging this down and fixing it.  When I saw this split
index bug bisect to me that my heart sank a little; there's so much of
that code I don't understand.  Nice to see you've already come along
and fixed it all up.  :-)

> ---
>  I considered adding a test, but since the problem is a warning, not
>  sure how to catch that. And a test would not be able to verify all
>  changes in this patch anyway.
>
>  read-cache.c  |  5 +++--
>  split-index.c | 34 ++++++++++++++++++----------------
>  2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 0e0c93edc9..d6fb09984f 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2894,7 +2894,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>                         return -1;
>         }
>
> -       if (!strip_extensions && istate->split_index) {
> +       if (!strip_extensions && istate->split_index &&
> +           !is_null_oid(&istate->split_index->base_oid)) {
>                 struct strbuf sb = STRBUF_INIT;
>
>                 err = write_link_extension(&sb, istate) < 0 ||
> @@ -3189,7 +3190,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>         ret = write_split_index(istate, lock, flags);
>
>         /* Freshen the shared index only if the split-index was written */
> -       if (!ret && !new_shared_index) {
> +       if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) {
>                 const char *shared_index = git_path("sharedindex.%s",
>                                                     oid_to_hex(&si->base_oid));
>                 freshen_shared_index(shared_index, 1);
> diff --git a/split-index.c b/split-index.c
> index 5820412dc5..a9d13611a4 100644
> --- a/split-index.c
> +++ b/split-index.c
> @@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate)
>  void remove_split_index(struct index_state *istate)
>  {
>         if (istate->split_index) {
> -               /*
> -                * When removing the split index, we need to move
> -                * ownership of the mem_pool associated with the
> -                * base index to the main index. There may be cache entries
> -                * allocated from the base's memory pool that are shared with
> -                * the_index.cache[].
> -                */
> -               mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
> +               if (istate->split_index->base) {
> +                       /*
> +                        * When removing the split index, we need to move
> +                        * ownership of the mem_pool associated with the
> +                        * base index to the main index. There may be cache entries
> +                        * allocated from the base's memory pool that are shared with
> +                        * the_index.cache[].
> +                        */
> +                       mem_pool_combine(istate->ce_mem_pool,
> +                                        istate->split_index->base->ce_mem_pool);
>
> -               /*
> -                * The split index no longer owns the mem_pool backing
> -                * its cache array. As we are discarding this index,
> -                * mark the index as having no cache entries, so it
> -                * will not attempt to clean up the cache entries or
> -                * validate them.
> -                */
> -               if (istate->split_index->base)
> +                       /*
> +                        * The split index no longer owns the mem_pool backing
> +                        * its cache array. As we are discarding this index,
> +                        * mark the index as having no cache entries, so it
> +                        * will not attempt to clean up the cache entries or
> +                        * validate them.
> +                        */
>                         istate->split_index->base->cache_nr = 0;
> +               }
>
>                 /*
>                  * We can discard the split index because its
> --
> 2.20.1.682.gd5861c6d90
>

Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid

[Ævar Arnfjörð Bjarmason]

On Sat, Feb 09 2019, Nguyễn Thái Ngọc Duy wrote:

I ran into this bug in the past but wasn't able to reproduce it long
enough to track it down. Thanks for the fix.

It would be nice to have a test for it if it's easily narrowed down,
tricky edge cases we fix are worth testing for...

>  I considered adding a test, but since the problem is a warning, not
>  sure how to catch that. And a test would not be able to verify all
>  changes in this patch anyway.

You'd write the test like this:

    git [...] 2>stderr &&
    test_i18ngrep "could not.*000000000" stderr

Or something like that. We have a lot of tests like this already, some
can be found with:

    git grep -B1 test_i18ngrep.*stderr

So something being a warning shouldn't stop us from testing for it.

Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> There is also another side fix in remove_split_index() that causes a
>> crash when doing "git update-index --no-split-index" when base_oid in
>> the index file is null. In this case we will not load
>> istate->split_index->base but we dereference it anyway and are rewarded
>> with a segfault. This should not happen anymore, but it's still wrong to
>> dereference a potential NULL pointer, especially when we do check for
>> NULL pointer in the next code.
>>
>> Reported-by: Luke Diamand <luke@diamand.org>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> Thanks for digging this down and fixing it.  When I saw this split
> index bug bisect to me that my heart sank a little; there's so much of
> that code I don't understand.  Nice to see you've already come along
> and fixed it all up.  :-)

Thanks, all.

[PATCH v2] read-cache.c: fix writing "link" index ext with null base oid

[Nguyễn Thái Ngọc Duy]

Since commit 7db118303a (unpack_trees: fix breakage when o->src_index !=
o->dst_index - 2018-04-23) and changes in merge code to use separate
index_state for source and destination, when doing a merge with split
index activated, we may run into this line in unpack_trees():

    o->result.split_index = init_split_index(&o->result);

This is by itself not wrong. But this split index information is not
fully populated (and it's only so when move_cache_to_base_index() is
called, aka force splitting the index, or loading index_state from a
file). Both "base_oid" and "base" in this case remain null.

So when writing the main index down, we link to this index with null
oid (default value after init_split_index()), which also means "no split
index" internally. This triggers an incorrect base index refresh:

    warning: could not freshen shared index '.../sharedindex.0{40}'

This patch makes sure we will not refresh null base_oid (because the
file is never there). It also makes sure not to write "link" extension
with null base_oid in the first place (no point having it at
all). Read code already has protection against null base_oid.

There is also another side fix in remove_split_index() that causes a
crash when doing "git update-index --no-split-index" when base_oid in
the index file is null. In this case we will not load
istate->split_index->base but we dereference it anyway and are rewarded
with a segfault. This should not happen anymore, but it's still wrong to
dereference a potential NULL pointer, especially when we do check for
NULL pointer in the next code.

Reported-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2 added a new test

 read-cache.c           |  5 +++--
 split-index.c          | 34 ++++++++++++++++++----------------
 t/t1700-split-index.sh | 18 ++++++++++++++++++
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 8f644f68b4..d140b44f8f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2520,7 +2520,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		return err;
 
 	/* Write extension data here */
-	if (!strip_extensions && istate->split_index) {
+	if (!strip_extensions && istate->split_index &&
+	    !is_null_oid(&istate->split_index->base_oid)) {
 		struct strbuf sb = STRBUF_INIT;
 
 		err = write_link_extension(&sb, istate) < 0 ||
@@ -2794,7 +2795,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	ret = write_split_index(istate, lock, flags);
 
 	/* Freshen the shared index only if the split-index was written */
-	if (!ret && !new_shared_index) {
+	if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) {
 		const char *shared_index = git_path("sharedindex.%s",
 						    oid_to_hex(&si->base_oid));
 		freshen_shared_index(shared_index, 1);
diff --git a/split-index.c b/split-index.c
index 5820412dc5..a9d13611a4 100644
--- a/split-index.c
+++ b/split-index.c
@@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate)
 void remove_split_index(struct index_state *istate)
 {
 	if (istate->split_index) {
-		/*
-		 * When removing the split index, we need to move
-		 * ownership of the mem_pool associated with the
-		 * base index to the main index. There may be cache entries
-		 * allocated from the base's memory pool that are shared with
-		 * the_index.cache[].
-		 */
-		mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
+		if (istate->split_index->base) {
+			/*
+			 * When removing the split index, we need to move
+			 * ownership of the mem_pool associated with the
+			 * base index to the main index. There may be cache entries
+			 * allocated from the base's memory pool that are shared with
+			 * the_index.cache[].
+			 */
+			mem_pool_combine(istate->ce_mem_pool,
+					 istate->split_index->base->ce_mem_pool);
 
-		/*
-		 * The split index no longer owns the mem_pool backing
-		 * its cache array. As we are discarding this index,
-		 * mark the index as having no cache entries, so it
-		 * will not attempt to clean up the cache entries or
-		 * validate them.
-		 */
-		if (istate->split_index->base)
+			/*
+			 * The split index no longer owns the mem_pool backing
+			 * its cache array. As we are discarding this index,
+			 * mark the index as having no cache entries, so it
+			 * will not attempt to clean up the cache entries or
+			 * validate them.
+			 */
 			istate->split_index->base->cache_nr = 0;
+		}
 
 		/*
 		 * We can discard the split index because its
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index f053bf83eb..ea5181aff9 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -447,4 +447,22 @@ test_expect_success 'writing split index with null sha1 does not write cache tre
 	test_line_count = 0 cache-tree.out
 '
 
+test_expect_success 'do not refresh null base index' '
+	test_create_repo merge &&
+	(
+		cd merge &&
+		test_commit initial &&
+		git checkout -b side-branch &&
+		test_commit extra &&
+		git checkout master &&
+		git update-index --split-index &&
+		test_commit more &&
+		# must not write a new shareindex, or we wont catch the problem
+		git -c splitIndex.maxPercentChange=100 merge --no-edit side-branch 2>err &&
+		# i.e. do not expect warnings like
+		# could not freshen shared index .../shareindex.00000...
+		test_must_be_empty err
+	)
+'
+
 test_done
-- 
2.21.0.rc0.328.g0e39304f8d

Re: [PATCH v2] read-cache.c: fix writing "link" index ext with null base oid

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 13 2019, Nguyễn Thái Ngọc Duy wrote:

> Since commit 7db118303a (unpack_trees: fix breakage when o->src_index !=
> o->dst_index - 2018-04-23) and changes in merge code to use separate
> index_state for source and destination, when doing a merge with split
> index activated, we may run into this line in unpack_trees():
>
>     o->result.split_index = init_split_index(&o->result);
>
> This is by itself not wrong. But this split index information is not
> fully populated (and it's only so when move_cache_to_base_index() is
> called, aka force splitting the index, or loading index_state from a
> file). Both "base_oid" and "base" in this case remain null.
>
> So when writing the main index down, we link to this index with null
> oid (default value after init_split_index()), which also means "no split
> index" internally. This triggers an incorrect base index refresh:
>
>     warning: could not freshen shared index '.../sharedindex.0{40}'
>
> This patch makes sure we will not refresh null base_oid (because the
> file is never there). It also makes sure not to write "link" extension
> with null base_oid in the first place (no point having it at
> all). Read code already has protection against null base_oid.
>
> There is also another side fix in remove_split_index() that causes a
> crash when doing "git update-index --no-split-index" when base_oid in
> the index file is null. In this case we will not load
> istate->split_index->base but we dereference it anyway and are rewarded
> with a segfault. This should not happen anymore, but it's still wrong to
> dereference a potential NULL pointer, especially when we do check for
> NULL pointer in the next code.
>
> Reported-by: Luke Diamand <luke@diamand.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2 added a new test

This round looks good to me. Passes all tests with/without
GIT_TEST_SPLIT_INDEX=true

>  read-cache.c           |  5 +++--
>  split-index.c          | 34 ++++++++++++++++++----------------
>  t/t1700-split-index.sh | 18 ++++++++++++++++++
>  3 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 8f644f68b4..d140b44f8f 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2520,7 +2520,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>  		return err;
>
>  	/* Write extension data here */
> -	if (!strip_extensions && istate->split_index) {
> +	if (!strip_extensions && istate->split_index &&
> +	    !is_null_oid(&istate->split_index->base_oid)) {
>  		struct strbuf sb = STRBUF_INIT;
>
>  		err = write_link_extension(&sb, istate) < 0 ||

However, it looks like you based this on a pre-2.20.0 version of
git. This conflicts with read-cache.c earlier than 3b1d9e045e ("eoie:
add End of Index Entry (EOIE) extension", 2018-10-10).

I fixed that manually, and pushed it out to github.com/avar/git.git
duy-read-cache-null-split-index-fix, that's what I've tested on top of
current "master".