Re: Broken branch after git commit - tracked files in staging area can't be removed with restore --staged, or commit or stash

On Wed, Jan 08, 2020 at 11:02:41AM +0100, Torsten Krah wrote:

Thanks, I can see the problem now.

OK, so we'd expect to still see TEST3 and TEST4 in the index. And we do:

And then if we try to unstage one of them, we should see that:

So far so good. But I think there is a lurking problem in the index, because...

This is very wrong (and is a bug, not a problem with what you're doing). We wrote a commit with TEST3 in it, even though it wasn't in the index. Why would we do that? Almost certainly it's because of a cache-tree extension in the index. Presumably "git restore --staged" is not properly invalidating the cache-tree entry. And that would explain what you see next:

That happens because we wrote out a commit with TEST3, but it's still not in the index! So it appears as if a deletion was staged (remember that what is "staged" is really just the diff between the index and HEAD).

We can repeat the same process, substituting "git reset -- TEST3" for git-restore, and it works fine. So the problem is in git-restore itself.

Here's another observation. If we do:

  git restore --staged TEST3 TEST4

then running "git commit" won't do anything, since there's nothing staged (and this is why I had trouble reproducing the problem earlier). If we add a new file, like:

  echo whatever >new
  git add new

then the problem won't exhibit, because that will also invalidate the cache-tree (because it has to account for "new"). But we could put those files in their own subdirectory and do:

  $ git restore --staged subdir/TEST3
  $ git status
  On branch master
  Changes to be committed:
  	new file:   subdir/TEST4
  
  Untracked files:
  	subdir/TEST3

and if I commit that, I see the bug again:

  $ git commit -m second
  [master 5c3b8c1] second
   2 files changed, 0 insertions(+), 0 deletions(-)
   create mode 100644 subdir/TEST3
   create mode 100644 subdir/TEST4

Now here's even more fun. I wanted to do another test, so I tried to reset back to the original "second" commit, but it segfaults! Yikes.

  $ git reset --hard HEAD@{2}
  Segmentation fault

Running it under valgrind shows we're indeed seeing a bogus cache-tree:

  ==2214443== Invalid read of size 4
  ==2214443==    at 0x362DFF: traverse_by_cache_tree (unpack-trees.c:734)
  ==2214443==    by 0x3630A3: traverse_trees_recursive (unpack-trees.c:799)
  ==2214443==    by 0x364209: unpack_callback (unpack-trees.c:1258)
  ==2214443==    by 0x35F925: traverse_trees (tree-walk.c:497)
  ==2214443==    by 0x364DDD: unpack_trees (unpack-trees.c:1589)
  ==2214443==    by 0x1BE6CF: reset_index (reset.c:95)
  ==2214443==    by 0x1BF894: cmd_reset (reset.c:421)
  ==2214443==    by 0x124868: run_builtin (git.c:444)
  ==2214443==    by 0x124BAE: handle_builtin (git.c:674)
  ==2214443==    by 0x124E24: run_argv (git.c:741)
  ==2214443==    by 0x1252AB: cmd_main (git.c:872)
  ==2214443==    by 0x1E811A: main (common-main.c:52)
  ==2214443==  Address 0x40 is not stack'd, malloc'd or (recently) free'd
  ==2214443== 
  ==2214443== Process terminating with default action of signal 11 (SIGSEGV)

This isn't the same spot that Emily fixed earlier today in [1], but it seems like a very similar problem. Weird.

So instead I blew away the index and then did hard reset:

  $ rm .git/index
  $ git reset --hard
  HEAD is now at 5c3b8c1 second

And now we can see what happens if both files are unstaged, and then we add a new file, and then commit:

  $ git reset --soft HEAD^
  $ git restore --staged subdir/TEST3 subdir/TEST4
  $ echo whatever >new
  $ git add new
  $ git commit -m again
  [master 119d9e4] again
   1 file changed, 1 insertion(+)
   create mode 100644 new

So that _doesn't_ exhibit the problem. I guess because adding the new file causes git-add to reexamine the whole cache-tree and clean it up.

So there seem to be at least two bugs:

 - git-restore doesn't properly invalidate the cache-tree

 - the index-reading code is not careful enough about bogus cache-trees,
   and may segfault

-Peff

[1] https://lore.kernel.org/git/20200108023127.219429-1-emilyshaffer@google.com/