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

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

[Torsten Krah]

Hi,

as written here https://git-scm.com/community I should sent reports to
this list - if I am mistaken please tell me where to direct this
question / problem I've encountered.

I've got a problem with a git branch and as this is the second time now
(I never hit that problem before I've started using the "new" git
restore --staged command - but since I am using it its the second time
now - maybe by accident, maybe related - I don't know).

Maybe someone can help me with this matter.

I am using 2.24.1 git and my starting point was a local branch and I
wanted to removed some files from the last commit I made.
So I did this:

1. git reset --soft HEAD~1

Now I had all my files from the last commit in the staging area and to
remove some of them I did:

2. git restore --staged $my-files

This worked too - now those files were moved to the working tree in git
status and were untracked and the rest looked still fine in the staging
area.

3. git commit

Now I made my commit - had a look on that with git show and it looks
fine.

But now I've hit the problem - looking with "git status" I was left now
with 6 files in the staging area all marked as deleted - still they are
in the working tree.
But I did never deleted them and the most annoying part now - I can't
fix that.

I tried:

1. git restore --staged

Does not change anything, still deleted in the staging area.

2. git stash

Does create a stash entry (did not look at the content though yet) but
staging area still unchanged.

3. git commit

Does create a commit but the whole commit is empty - nothing from the
tracked staging area is there and "git status" is unchanged - still 6
tracked deleted files.

I don't know what went wrong and I am unable to fix it - help
appreciated.
I can't share the whole git repository but I can try whatever git
command or other tool output may help here to shed some light on this -
as written I don't have a clear reproducer, I just have my branch with
a borked staging area.

thx and kind regards

Torsten

PS: Of cause I can delete the whole repo and clone it again or remove
the branch and start from a fresh new one ;) but I want to analyse
this.

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

[Torsten Krah]

Am Dienstag, den 07.01.2020, 13:55 +0100 schrieb Torsten Krah:
> 3. git commit
> 
> Now I made my commit - had a look on that with git show and it looks
> fine.

I had a second look on this step and the result it wrong.

Although restore --staged moved my unwanted files away from the staging
area and "git status" told me that they are not "in" the commit the
commit itself did still include them.

So they are listed as unversioned now but are in the commit although
git status told me otherwise - weird.

kind regards

Torsten

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

[Torsten Krah]

Am Dienstag, den 07.01.2020, 14:43 +0100 schrieb Torsten Krah:
> Although restore --staged moved my unwanted files away from the
> staging
> area and "git status" told me that they are not "in" the commit the
> commit itself did still include them.

I can reproduce that (locally) at least:

What does *not* work for me:

   git clone XX main
   cd main
   git fetch XX && git checkout FETCH_HEAD
   git checkout -b TEST
   git reset --soft HEAD~1
   git restore --staged $FILES

git status now lists $FILES as unstaged and they are not included in
the staging area.

   git commit

-> now $FILES are included in the commit (I would expect them not to be
included - right?) and git status does list those still in the working
area.

What does work:

   git clone XX main
   cd main
   git fetch XX && git checkout FETCH_HEAD
   git checkout -b TEST
   git reset --soft HEAD~1
   git reset HEAD $FILES

git status now lists $FILES as unstaged and they are not included in
the staging area.

   git commit

produces a commit where $FILES are not included and they are still in
the working area, unstaged - like expected.



git status tells me this in the staging area part:

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)

I did that and its not working (for me) - looks at least like a bug or
I am doing something wrong and I am just too dumb at the moment to see
my failure.

Cheers

Torsten

PS: $FILES are files which are all "new" and first time added in the
commit I want to modify with restore.

PPS: The second problem with those staged deleted, unstageable,
uncomittable files still persists in my copy of those branch (I can't
reproduce that - still I have the repository in that state).

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

[Jeff King]

On Tue, Jan 07, 2020 at 04:28:24PM +0100, Torsten Krah wrote:

> I can reproduce that (locally) at least:
> 
> What does *not* work for me:
> 
>    git clone XX main
>    cd main
>    git fetch XX && git checkout FETCH_HEAD
>    git checkout -b TEST
>    git reset --soft HEAD~1
>    git restore --staged $FILES
> 
> git status now lists $FILES as unstaged and they are not included in
> the staging area.
> 
>    git commit
> 
> -> now $FILES are included in the commit (I would expect them not to be
> included - right?) and git status does list those still in the working
> area.

That step seems wrong, and I can't reproduce it here. If "git status"
lists the files as unstaged, then "git commit" should not be committing
them. Can you show us a more complete example that we can run ourselves
(i.e., that does not rely on whatever is in "main", and what is in
$FILES)? Barring that, can you show us the output of the commands, as
well as "git show FETCH_HEAD FETCH_HEAD~1"?

-Peff

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

[Torsten Krah]

Am Mittwoch, den 08.01.2020, 04:11 -0500 schrieb Jeff King:
> That step seems wrong, and I can't reproduce it here. If "git status"
> lists the files as unstaged, then "git commit" should not be
> committing
> them. Can you show us a more complete example that we can run
> ourselves
> (i.e., that does not rely on whatever is in "main", and what is in
> $FILES)? Barring that, can you show us the output of the commands, as
> well as "git show FETCH_HEAD FETCH_HEAD~1"?
> 
> -Peff

Hi Jeff, I have a poc you can try:

cd /tmp
mkdir testrepo
cd testrepo
touch TEST1 TEST2
git add -A
git commit -m First
touch TEST3 TEST4
git add -A
git commit -m Second
git reset --soft HEAD~1

git status

   Auf Branch master
   Zum Commit vorgemerkte Änderungen:
     (benutzen Sie "git restore --staged <Datei>..." zum Entfernen aus
   der Staging-Area)
   	neue Datei:     TEST3
   	neue Datei:     TEST4

git restore --staged TEST3

   [10:57:26][tkrah@torstenknbl:/tmp/testrepo]  (master) $ LC_ALL=C git
   status
   On branch master
   Changes to be committed:
     (use "git restore --staged <file>..." to unstage)
   	new file:   TEST4

   Untracked files:
     (use "git add <file>..." to include in what will be committed)
   	TEST3

git commit -m Second

   [master 5b62331] Second
    2 files changed, 0 insertions(+), 0
   deletions(-)
    create mode 100644 TEST3
    create mode 100644 TEST4

And now TEST3 is in the commit and what is even more "interesting" is
the next one:

   [10:59:16][tkrah@torstenknbl:/tmp/testrepo]  (master) $ LC_ALL=C git
status
   On branch master
   Changes to be committed:
     (use "git restore --staged <file>..." to unstage)
   	deleted:    TEST3

   Untracked files:
     (use "git add <file>..." to include in what will be committed)
   	TEST3

TEST3 is unstaged and deleted now.

This seems wrong - or did I something wrong?

Cheers

Torsten


-- 
Mit freundlichen Grüßen / Best regards

Torsten Krah

mgm technology partners GmbH
Neumarkt 2
04109 Leipzig

Tel. +49 (341) 339 893-539
E-Mail Torsten.Krah@mgm-tp.com

Innovation Implemented.

Geschäftsführer / CEO: Hamarz Mehmanesh
Sitz der Gesellschaft / Registered office: München
Handelsregister/ Commercial register: AG München HRB 161298
USt-IdNr. / VAT ID: DE815309575

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

[Torsten Krah]

Am Mittwoch, den 08.01.2020, 11:02 +0100 schrieb Torsten Krah:
> cd testrepo

git init

of cause after this one ;)

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

[Jeff King]

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

> Hi Jeff, I have a poc you can try:

Thanks, I can see the problem now.

> cd /tmp
> mkdir testrepo
> cd testrepo
> touch TEST1 TEST2
> git add -A
> git commit -m First
> touch TEST3 TEST4
> git add -A
> git commit -m Second
> git reset --soft HEAD~1

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

> git status
> 
>    Auf Branch master
>    Zum Commit vorgemerkte Änderungen:
>      (benutzen Sie "git restore --staged <Datei>..." zum Entfernen aus
>    der Staging-Area)
>    	neue Datei:     TEST3
>    	neue Datei:     TEST4

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

> git restore --staged TEST3
> 
>    [10:57:26][tkrah@torstenknbl:/tmp/testrepo]  (master) $ LC_ALL=C git
>    status
>    On branch master
>    Changes to be committed:
>      (use "git restore --staged <file>..." to unstage)
>    	new file:   TEST4
> 
>    Untracked files:
>      (use "git add <file>..." to include in what will be committed)
>    	TEST3

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

> git commit -m Second
> 
>    [master 5b62331] Second
>     2 files changed, 0 insertions(+), 0
>    deletions(-)
>     create mode 100644 TEST3
>     create mode 100644 TEST4

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:

> And now TEST3 is in the commit and what is even more "interesting" is
> the next one:
> 
>    [10:59:16][tkrah@torstenknbl:/tmp/testrepo]  (master) $ LC_ALL=C git
> status
>    On branch master
>    Changes to be committed:
>      (use "git restore --staged <file>..." to unstage)
>    	deleted:    TEST3
> 
>    Untracked files:
>      (use "git add <file>..." to include in what will be committed)
>    	TEST3
> 
> TEST3 is unstaged and deleted now.

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/

[PATCH] restore: invalidate cache-tree when removing entries with --staged

[Jeff King]

On Wed, Jan 08, 2020 at 05:40:08AM -0500, Jeff King wrote:

> 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

Here's a fix for the first one. I'm adding Junio to the cc as an expert
in index and cache-tree issues. I'm pretty sure this is the correct fix,
but I have some lingering questions below.

I'm not planning on working on the second one immediately. Between this
and Emily's patch from yesterday, I have a feeling that the index code
could use an audit to be a bit more careful about handling bogus on-disk
data.

-- >8 --
Subject: restore: invalidate cache-tree when removing entries with --staged

When "git restore --staged <path>" removes a path that's in the index,
it marks the entry with CE_REMOVE, but we don't do anything to
invalidate the cache-tree. In the non-staged case, we end up in
checkout_worktree(), which calls remove_marked_cache_entries(). That
actually drops the entries from the index, as well as invalidating the
cache-tree and untracked-cache.

But with --staged, we never call checkout_worktree(), and the CE_REMOVE
entries remain. Interestingly, they are dropped when we write out the
index, but that means the resulting index is inconsistent: its
cache-tree will not match the actual entries, and running "git commit"
immediately after will create the wrong tree.

We can solve this by calling remove_marked_cache_entries() ourselves
before writing out the index. Note that we can't just hoist it out of
checkout_worktree(); that function needs to iterate over the CE_REMOVE
entries (to drop their matching worktree files) before removing them.

One curiosity about the test: without this patch, it actually triggers a
BUG() when running git-restore:

  BUG: cache-tree.c:810: new1 with flags 0x4420000 should not be in cache-tree

But in the original problem report, which used a similar recipe,
git-restore actually creates the bogus index (and the commit is created
with the wrong tree). I'm not sure why the test here behaves differently
than my out-of-suite reproduction, but what's here should catch either
symptom (and the fix corrects both cases).

Reported-by: Torsten Krah <krah.tm@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
So I'm mildly puzzled about the BUG thing above. But also: it seems
really weird that do_write_index() will drop CE_REMOVE entries as it
writes, but not invalidate the cache-tree (or the untracked-cache, which
AFAICT is probably similarly affected). Should it be doing that
invalidation? Or should it be a BUG() to write out an index without
having done remove_marked_cache_entries()?

 builtin/checkout.c |  2 ++
 t/t2070-restore.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b52c490c8f..18ef5fb975 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -524,6 +524,8 @@ static int checkout_paths(const struct checkout_opts *opts,
 	/* Now we are committed to check them out */
 	if (opts->checkout_worktree)
 		errs |= checkout_worktree(opts);
+	else
+		remove_marked_cache_entries(&the_index, 1);
 
 	/*
 	 * Allow updating the index when checking out from the index.
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 21c3f84459..06a5976143 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -96,6 +96,7 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
 '
 
 test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '
+	test_when_finished git reset --hard &&
 	echo "nonempty" >nonempty &&
 	>empty &&
 	git add nonempty empty &&
@@ -106,4 +107,21 @@ test_expect_success 'restore --staged adds deleted intent-to-add file back to in
 	git diff --cached --exit-code
 '
 
+test_expect_success 'restore --staged invalidates cache tree for deletions' '
+	test_when_finished git reset --hard &&
+	>new1 &&
+	>new2 &&
+	git add new1 new2 &&
+
+	# It is important to commit and then reset here, so that the index
+	# contains a valid cache-tree for the "both" tree.
+	git commit -m both &&
+	git reset --soft HEAD^ &&
+
+	git restore --staged new1 &&
+	git commit -m "just new2" &&
+	git rev-parse HEAD:new2 &&
+	test_must_fail git rev-parse HEAD:new1
+'
+
 test_done
-- 
2.25.0.rc1.622.g8cfac75bdd

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

[Torsten Krah]

Am Mittwoch, den 08.01.2020, 05:40 -0500 schrieb Jeff King:
> 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
-- 

Hi Peff,

thanks for confirming those bugs and taking a look at my report itself
even before I've put the small reproducer recipe.
I don't need to take any other actions or create tickets somewhere else
now to get those fixed, you're driving this from here, right?

Another question - how can I fix my broken original branch where this
bug did hit me which is in that "bogus" state now to get back to the
"original" commit made so I can remove my unwanted files with "git
reset HEAD <files>" instead of git-restore in the meantime (tried your
rm .git/index variant but after that I had files which were already in
the branch shown as unversioned after that, so that did not work very
well for me)?

kind regards

Torsten

Re: [PATCH] restore: invalidate cache-tree when removing entries with --staged

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b52c490c8f..18ef5fb975 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -524,6 +524,8 @@ static int checkout_paths(const struct checkout_opts *opts,
>  	/* Now we are committed to check them out */
>  	if (opts->checkout_worktree)
>  		errs |= checkout_worktree(opts);
> +	else
> +		remove_marked_cache_entries(&the_index, 1);

Ah, I was wondering why we were seeing breakages on cache-tree,
which is fairly old and stable part of the system---even though it
had caused us quite a lot of pain while it was growing---all of a
sudden.  No wonder---this codepath is a fairly new one, introduced
when "restore" was added X-<.

And the fix looks right, of course.  Thanks for extracting a
reproducible recipe out of the original reporter and quickly
diagnosing it.  Very much appreciated.

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

[Jeff King]

On Wed, Jan 08, 2020 at 01:42:14PM +0100, Torsten Krah wrote:

> thanks for confirming those bugs and taking a look at my report itself
> even before I've put the small reproducer recipe.
> I don't need to take any other actions or create tickets somewhere else
> now to get those fixed, you're driving this from here, right?

Correct.

> Another question - how can I fix my broken original branch where this
> bug did hit me which is in that "bogus" state now to get back to the
> "original" commit made so I can remove my unwanted files with "git
> reset HEAD <files>" instead of git-restore in the meantime (tried your
> rm .git/index variant but after that I had files which were already in
> the branch shown as unversioned after that, so that did not work very
> well for me)?

You should be able to do:

  git checkout your-branch
  git reset --hard <original>

to go back to the state you were at (this is assuming you haven't built
more commits on top, of course), and then repeat your steps. That should
also clear out any breakage in the index, since "reset --hard" will
invalidate the cache-tree as necessary.

If you need to find the commit id of that original commit, try looking
in the reflog for your branch: git log -g your-branch

-Peff

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

[Torsten Krah]

Am Donnerstag, den 09.01.2020, 04:39 -0500 schrieb Jeff King:
> You should be able to do:
> 
>   git checkout your-branch
>   git reset --hard <original>
> 
> to go back to the state you were at (this is assuming you haven't
> built
> more commits on top, of course), and then repeat your steps. That
> should
> also clear out any breakage in the index, since "reset --hard" will
> invalidate the cache-tree as necessary.
> 
> If you need to find the commit id of that original commit, try
> looking
> in the reflog for your branch: git log -g your-branch

Hi Peff,

thanks that did work - but I had to "rm .git/index" before the hard
reset to get those "deleted" ones removed from the staging area, after
that I was able to use "git reset -- $files" and that was working like
expected.
Without deleting that index file I could run the hard reset but running
"git status" afterwards insisted every time on this:

On branch feature-2182
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	deleted:    docker-compose.gerrit.yml
	deleted:    docker-compose.reverseproxy.yml
	deleted:    docker-compose.yml
	deleted:    docker-seccomp.json
	deleted:    gradle.properties
	deleted:    settings.gradle

Those only vanished after deleting the index file.

The "settings.gradle" is part of my changeset and listed in the
"commit".
But those other 5 files listed I had never touched in my commit - I
don't know why they are in the staging area here - maybe some other
edge case of the git restore bug - but just a "git hard --reset $hash"
did not clear those.

Torsten
-- 

Re: [PATCH] restore: invalidate cache-tree when removing entries with --staged

[Dennis Kaarsemaker]

On Wed, 2020-01-08 at 06:43 -0500, Jeff King wrote:
> On Wed, Jan 08, 2020 at 05:40:08AM -0500, Jeff King wrote:
> 
> > 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
> 
> Here's a fix for the first one. I'm adding Junio to the cc as an expert
> in index and cache-tree issues. I'm pretty sure this is the correct fix,
> but I have some lingering questions below.
> 
> I'm not planning on working on the second one immediately. Between this
> and Emily's patch from yesterday, I have a feeling that the index code
> could use an audit to be a bit more careful about handling bogus on-disk
> data.

We just ran into something similar where git would create really bogus
commits when mixing squash merges and restore. As it's a private repo,
I don't have an exact recipe for reproducing it, but it roughly goes
like:

git checkout master
git merge --squash branch-for-which-we-want-to-redo-commits
git restore --staged .
git add file1 file2 file3
git commit -m "commit"

This commit would remove a file4 that wasn't even touched in the branch
(further commits would do even more broken things, eventually leading
to broken commit objects with duplicate file contents). Changing `git
restore --staged .` to `git reset HEAD` made this behaviour go away. A
quick search in the list brought this patch and I'm happy to say it
fixes our issue as well.

Thanks Peff!

D.