Bug: Failed checkout causes index to be in incorrect state

Bug: Failed checkout causes index to be in incorrect state

[some-java-user-99206970363698485155]

Hello,

this originally occurred with Git for Windows (see https://github.com/git-for-windows/git/issues/3411)
but it appears the underlying issue is within Git itself, see Johannes Schindelin's comment
(https://github.com/git-for-windows/git/issues/3411#issuecomment-914188040).

## Description
When Git fails to check out files (during `git clone`) the index seems to be in an incomplete state afterwards.
The files which could not be checked out are erroneously staged as "deleted", and files which were checked out
successfully are marked as untracked. This renders the suggested `git restore --source=HEAD :/` command to retry
the checkout ineffective.

## Reproduction steps (Windows)
Preconditions:
- Long Windows file paths are not enabled
  (https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=cmd#enable-long-paths-in-windows-10-version-1607-and-later)
- `git config core.longpaths` is not set

1. `git clone https://github.com/dependabot/dependabot-core --depth=1 file-name-test`
   (should fail due to too long file names)
2. `cd file-name-test`
3. `git status`
   (should claim that files for which checkout failed are staged as "deleted")
4. `git config core.longpaths true`
5. Run the command suggested by the error message from step 1: `git restore --source=HEAD :/`
6. `git status`
   (you should see that `git restore` had no effect)

## System information
Microsoft Windows [Version 10.0.19043.1237]
git version 2.33.0.windows.2

All reproduction steps up to, including, step 3 can also be reproduced with WSL 2.
You might need to use a different Git repository with longer files names though, such as
https://github.com/Marcono1234/git-file-length-test
WSL 2: 5.4.72-microsoft-standard-WSL2
git version 2.25.1

Re: Bug: Failed checkout causes index to be in incorrect state

[Jeff King]

On Sat, Oct 02, 2021 at 10:41:32PM +0200, some-java-user-99206970363698485155@vodafonemail.de wrote:

> ## Description
> When Git fails to check out files (during `git clone`) the index seems to be in an incomplete state afterwards.
> The files which could not be checked out are erroneously staged as "deleted", and files which were checked out
> successfully are marked as untracked. This renders the suggested `git restore --source=HEAD :/` command to retry
> the checkout ineffective.

Hmm. I guess I'm not too surprised that we might leave things in an
inconsistent state, but I'm having trouble getting the same outcome on
Linux.

If I clone a repo with a very long file name, I do indeed get a failure.
But there is no index written at all, nor any files checked out. So all
file are marked as to-be-deleted in the index (compared to the HEAD
commit), but nothing is untracked, nor are there differences between the
index and working tree.

My reproduction is:

	git init repo
	cd repo

	echo before >0
	echo after >2
	git add .
	git update-index --index-info <<-EOF
	100644 $(git rev-parse :0)	1-$(perl -e 'print "a" x 5000')
	EOF
	git commit -qm whatever

	git clone --no-local . foo.git
	cd foo.git
	git status | cut -c -60

where the final status yields (note the intentional truncation on the
filename):

  On branch main
  Your branch is up to date with 'origin/main'.
  
  Changes to be committed:
  	deleted:    0
  	deleted:    1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
  	deleted:    2

One interesting thing is that my error during clone is:

  error: cannot stat '1-aaaaaaaaaaa...

which happens before we even start putting things into the filesystem.
Whereas in the sample output you linked, Dscho had:

  error: unable to create file...

followed by more "Unpacking objects".

So there seems to be some difference in how the checkout occurs. The
backtrace for my error is:

  #0  error_errno (fmt=0x5555558bcf02 "cannot stat '%s'") at usage.c:220
  #1  0x0000555555804950 in verify_absent_1 (ce=0x7ffff7390098, error_type=ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, 
      o=0x7fffffffdba0) at unpack-trees.c:2249
  #2  0x0000555555804a56 in verify_absent (ce=0x7ffff7390098, error_type=ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, 
      o=0x7fffffffdba0) at unpack-trees.c:2268
  #3  0x0000555555804af8 in merged_entry (ce=0x5555559a8ae0, old=0x0, o=0x7fffffffdba0) at unpack-trees.c:2301
  #4  0x0000555555805a50 in oneway_merge (src=0x7fffffffd450, o=0x7fffffffdba0) at unpack-trees.c:2701
  #5  0x0000555555800896 in call_unpack_fn (src=0x7fffffffd450, o=0x7fffffffdba0) at unpack-trees.c:582
  #6  0x0000555555801f04 in unpack_single_entry (n=1, mask=1, dirmask=0, src=0x7fffffffd450, names=0x7fffffffd790, 
      info=0x7fffffffd9e0) at unpack-trees.c:1132
  #7  0x00005555558028c0 in unpack_callback (n=1, mask=1, dirmask=0, names=0x7fffffffd790, info=0x7fffffffd9e0)
      at unpack-trees.c:1404
  #8  0x00005555557fd0d5 in traverse_trees (istate=0x55555596f0e0 <the_index>, n=1, t=0x7fffffffdb50, 
      info=0x7fffffffd9e0) at tree-walk.c:532
  #9  0x0000555555803654 in unpack_trees (len=1, t=0x7fffffffdb50, o=0x7fffffffdba0) at unpack-trees.c:1784
  #10 0x0000555555599a7e in checkout (submodule_progress=1) at builtin/clone.c:699
  #11 0x000055555559b56a in cmd_clone (argc=2, argv=0x7fffffffe4f0, prefix=0x0) at builtin/clone.c:1299

I wonder why we get further than that on Windows. If I hack
verify_absent_1() to treat this as success, I do get "unable to create"
message later, along with some checked-out files. My status output is:

  On branch main
  Your branch is up to date with 'origin/main'.
  
  Changes to be committed:
  	deleted:    0
  	deleted:    1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
  	deleted:    2
  
  Untracked files:
  	0
  	2

We did not create the index at all, so comparing to HEAD, _every_ file
appears to be deleted. And likewise comparing to the working tree,
anything we did create is untracked.

I think it would probably be nicer if we wrote out the index in this
case, if there were no problems creating the index itself, and just in
the actual file-checkout stage. unpack_trees() tries to make this
distinction, but we don't pay any attention. So perhaps something like:

diff --git a/builtin/clone.c b/builtin/clone.c
index ff1d3d447a..84dca96dad 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -696,14 +696,27 @@ static int checkout(int submodule_progress)
 	tree = parse_tree_indirect(&oid);
 	parse_tree(tree);
 	init_tree_desc(&t, tree->buffer, tree->size);
-	if (unpack_trees(1, &t, &opts) < 0)
-		die(_("unable to checkout working tree"));
+	switch (unpack_trees(1, &t, &opts)) {
+	case 0: break;
+	case -2:
+		/*
+		 * failed to checkout; install index so that tools like
+		 * git-status give sensible results, but die below.
+		 */
+		err = -1;
+		break;
+	default:
+		die(_("unable to create index"));
+	}
 
 	free(head);
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
 
+	if (err)
+		die(_("unable to checkout working tree"));
+
 	err |= run_hook_le(NULL, "post-checkout", oid_to_hex(null_oid()),
 			   oid_to_hex(&oid), "1", NULL);
 

would help. But the outcome from git-status is weird. Because the long
name chokes stat(), it doesn't report it as a change, even though the
file is missing! This seems like a bug on the reading side.

I'm also still puzzled at the fact that on Linux we barf in
verify_absent(), but somehow don't on Windows.

-Peff