git checkout under 1.7.6 does not properly list untracked files and aborts

This may be an msysGit 1.7.6 issue, as that is what I am using. It also occurs in msysGit 1.7.5, but I am almost certain it did not happen in msysGit 1.7.2.

Given an untracked file in the working directory that has been added to an alternate branch, when switching to that alternate branch, 'git checkout' exits with an error code but does not print anything to the console. It should print an untracked file error.

I have been trying to track this down in code. The point where the error messages are printed, display_error_msgs, is never hit.

----- Original Message -----
From: Joshua Jensen
Date: 9/9/2011 2:04 PM

Quoted 5 lines

This may be an msysGit 1.7.6 issue, as that is what I am using. It also occurs in msysGit 1.7.5, but I am almost certain it did not happen in msysGit 1.7.2.

Given an untracked file in the working directory that has been added to an alternate branch, when switching to that alternate branch, 'git checkout' exits with an error code but does not print anything to the console. It should print an untracked file error.

I have been trying to track this down in code. The point where the error messages are printed, display_error_msgs, is never hit.

Okay, so I've tracked this down, but I am unsure what the correct fix is.

In unpack-trees.c's unpack_trees() function, there are some lines that read:

    if (ce->ce_flags & CE_ADDED &&
        verify_absent(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))
            return -1;

If the 'return -1' is changed to 'goto return_failed', then a proper error message appears:

error: The following untracked working tree files would be
overwritten by checkout:
one/file/listed/here.txt
Please move or remove them before you can switch branches. Aborting

The thing is, there are multiple files that would be overwritten by checkout, and I believe an older version of Git showed me the entire list before aborting.

----- Original Message -----
From: Joshua Jensen
Date: 9/9/2011 2:04 PM

Quoted 11 lines

This may be an msysGit 1.7.6 issue, as that is what I am using. It also occurs in msysGit 1.7.5, but I am almost certain it did not happen in msysGit 1.7.2.

Given an untracked file in the working directory that has been added to an alternate branch, when switching to that alternate branch, 'git checkout' exits with an error code but does not print anything to the console. It should print an untracked file error.

I have been trying to track this down in code. The point where the error messages are printed, display_error_msgs, is never hit.

Okay, so I've tracked this down, but I am unsure what the correct fix is.

In unpack-trees.c's unpack_trees() function, there are some lines that read:

     if (ce->ce_flags & CE_ADDED &&
         verify_absent(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))
             return -1;

If the 'return -1' is changed to 'goto return_failed', then a proper error message appears:

error: The following untracked working tree files would be
overwritten by checkout:
one/file/listed/here.txt
Please move or remove them before you can switch branches. Aborting

The thing is, there are multiple files that would be overwritten by checkout, and I believe an older version of Git showed me the entire list before aborting.

What would be the proper fix here? What am I doing wrong?

Can you provide a simple test case, such as this one:

---%<---
#!/bin/sh

rm -Rf utest || exit 1
mkdir utest || exit 1
cd utest || exit 1
git init
echo tracked>a
git add a
git commit -m a a
git branch side
echo tracked>b
git add b
git commit -m b
cat b
git checkout side
cat b
echo untracked>b
cat b
git checkout master
cat b
---%<---

Does this test reproduce your problem on msysgit?

----- Original Message -----
From: Michael J Gruber
Date: 9/20/2011 9:10 AM

Quoted 25 lines

Can you provide a simple test case, such as this one:

---%<---
#!/bin/sh

rm -Rf utest || exit 1
mkdir utest || exit 1
cd utest || exit 1
git init
echo tracked>a
git add a
git commit -m a a
git branch side
echo tracked>b
git add b
git commit -m b
cat b
git checkout side
cat b
echo untracked>b
cat b
git checkout master
cat b
---%<---

Quoted 1 line

Does this test reproduce your problem on msysgit?

As listed, your script produces the same results on msysGit.

It appears the issue is related to sparse checkouts:

---%<---
#!/bin/sh

rm -Rf utest || exit 1
mkdir utest || exit 1
cd utest || exit 1
git init
git config core.sparseCheckout true
echo *>.git/info/sparse-checkout
echo tracked>a
git add a
git commit -m a a
git branch side
echo tracked>b
git add b
git commit -m b
cat b
git checkout side
cat b
echo untracked>b
cat b
git checkout master
cat b
---%<---

-Josh

Forgot to update a test or two that this breaks?

Quoted 1 line · Junio C Hamano

Forgot to update a test or two that this breaks?

In the meantime I've squashed this in.

 t/t7607-merge-overwrite.sh     |    1 +
 t/t7609-merge-co-error-msgs.sh |    5 +++++
 2 files changed, 6 insertions(+), 0 deletions(-)

t/t7607-merge-overwrite.sh +1 -0

index 72a8731..aa74184 100755
@@ -107,6 +107,7 @@ error: The following untracked working tree files would be overwritten by merge:
 	sub
 	sub2
 Please move or remove them before you can merge.
+Aborting
 EOF
 
 test_expect_success 'will not overwrite untracked file in leading path' '

t/t7609-merge-co-error-msgs.sh +5 -0

index c994836..0e4a682 100755
@@ -32,6 +32,7 @@ error: The following untracked working tree files would be overwritten by merge:
 	three
 	two
 Please move or remove them before you can merge.
+Aborting
 EOF
 
 test_expect_success 'untracked files overwritten by merge (fast and non-fast forward)' '

@@ -56,6 +57,7 @@ Please, commit your changes or stash them before you can merge.
 error: The following untracked working tree files would be overwritten by merge:
 	five
 Please move or remove them before you can merge.
+Aborting
 EOF
 
 test_expect_success 'untracked files or local changes ovewritten by merge' '

@@ -71,6 +73,7 @@ error: Your local changes to the following files would be overwritten by checkou
 	rep/one
 	rep/two
 Please, commit your changes or stash them before you can switch branches.
+Aborting
 EOF
 
 test_expect_success 'cannot switch branches because of local changes' '

@@ -92,6 +95,7 @@ error: Your local changes to the following files would be overwritten by checkou
 	rep/one
 	rep/two
 Please, commit your changes or stash them before you can switch branches.
+Aborting
 EOF
 
 test_expect_success 'not uptodate file porcelain checkout error' '

@@ -105,6 +109,7 @@ error: Updating the following directories would lose untracked files in it:
 	rep
 	rep2
 
+Aborting
 EOF
 
 test_expect_success 'not_uptodate_dir porcelain checkout error' '

Interesting. So, it is related to sparse, it seems, and there is also a mistake in the documentation (which contradicts itself, btw). With your script, since all files are requested to be checked out ('*'), none should have skip-worktree set, and the result should be the same as with my version. But I get this:

Initialized empty Git repository in /tmp/t/utest/.git/ [master (root-commit) 5929c8b] a

 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 a
[master 60ad69e] b
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 b

tracked
Switched to branch 'side'
cat: b: No such file or directory
untracked
untracked

That is probably the same output as for you. The final "git checkout master" errored out without doing anything and without message. I'll send a few cleanups now, and do some bisecting and thinking later; though there are people whose sparse-knowledge is way less sparse than mine ;)

So, I bisected it. The first bad commit is

9037026 (unpack-trees: fix sparse checkout's "unable to match directories", 2010-11-27)

although the real culprit may be its predecessor

2431afb (unpack-trees: move all skip-worktree checks back to unpack_trees(), 2010-11-27)

which does not compile:

CC unpack-trees.o
unpack-trees.c: In function 'mark_new_skip_worktree':
unpack-trees.c:852:75: error: 'o' undeclared (first use in this function) unpack-trees.c:852:75: note: each undeclared identifier is reported only once for each function it appears in
make: *** [unpack-trees.o] Error 1

Duy, sorry for prodding you again.

Quoted 10 lines

So, I bisected it. The first bad commit is

9037026 (unpack-trees: fix sparse checkout's "unable to match directories", 2010-11-27)

although the real culprit may be its predecessor

2431afb (unpack-trees: move all skip-worktree checks back to unpack_trees(), 2010-11-27)

Quoted 8 lines

which does not compile:

CC unpack-trees.o
unpack-trees.c: In function 'mark_new_skip_worktree':
unpack-trees.c:852:75: error: 'o' undeclared (first use in this function) unpack-trees.c:852:75: note: each undeclared identifier is reported only once for each function it appears in
make: *** [unpack-trees.o] Error 1

This may help

--8<--

unpack-trees.c +3 -4

index a6518db..a239af7 100644
@@ -245,13 +245,13 @@ static int check_updates(struct unpack_trees_options *o)
 static int verify_uptodate_sparse(struct cache_entry *ce, struct unpack_trees_options *o);
 static int verify_absent_sparse(struct cache_entry *ce, enum unpack_trees_error_types, struct unpack_trees_options *o);
 
-static int will_have_skip_worktree(const struct cache_entry *ce, struct unpack_trees_options *o)
+static int will_have_skip_worktree(const struct cache_entry *ce, struct exclude_list *el)
 {
 	const char *basename;
 
 	basename = strrchr(ce->name, '/');
 	basename = basename ? basename+1 : ce->name;
-	return excluded_from_list(ce->name, ce_namelen(ce), basename, NULL, o->el) <= 0;
+	return excluded_from_list(ce->name, ce_namelen(ce), basename, NULL, el) <= 0;
 }
 
 static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_options *o)

@@ -849,7 +849,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,
 		if (select_flag && !(ce->ce_flags & select_flag))
 			continue;
 
-		if (!ce_stage(ce) && will_have_skip_worktree(ce, o))
+		if (!ce_stage(ce) && will_have_skip_worktree(ce, el))
 			ce->ce_flags |= skip_wt_flag;
 		else
 			ce->ce_flags &= ~skip_wt_flag;
--8<--

Quoted 1 line

Duy, sorry for prodding you again.

No problem (and sorry for breaking the build). I'll also have a look at this problem.

Thanks. I can confirm that with the above patch, the code compiles and fails my test. So it's the earlier of the two commits which introduces this.

I was afraid some logic had gone horribly wrong. Turns out I did not catch up with unpack_trees() coding style and drop the error messages so "git checkout" in this case becomes "git checkout -q".

The description of .git/info/sparse-checkout and skip-worktree is exactly the opposite of what is true, which is:

If a file matches a pattern in sparse-checkout, then (it is to be checked out and therefore) skip-worktree is unset for that file; otherwise, it is set (so that it is not checked out).

Currently, the opposite is documented, and (consistently) read-tree's behavior with respect to bit flips is descibed incorrectly.

Fix it.

In hindsight, it would have been much better to have a "sparse-ignore" or "sparse-skip" file so that an empty file would mean a full checkout, and the file logic would be analogous to that of .gitignore, excludes and skip-worktree.

Quoted 11 lines · Michael J Gruber

The description of .git/info/sparse-checkout and skip-worktree is exactly the opposite of what is true, which is:

If a file matches a pattern in sparse-checkout, then (it is to be checked out and therefore) skip-worktree is unset for that file; otherwise, it is set (so that it is not checked out).

Currently, the opposite is documented, and (consistently) read-tree's behavior with respect to bit flips is descibed incorrectly.

Fix it.

Ack.

Quoted 4 lines

In hindsight, it would have been much better to have a "sparse-ignore" or "sparse-skip" file so that an empty file would mean a full checkout, and the file logic would be analogous to that of .gitignore, excludes and skip-worktree.

.gitignore works towards excluding files. No rule means no excluding. sparse-checkout file works towards including files, no rule means no inclusion.

Quoted 5 lines

Quoted 4 lines

In hindsight, it would have been much better to have a "sparse-ignore" or "sparse-skip" file so that an empty file would mean a full checkout, and the file logic would be analogous to that of .gitignore, excludes and skip-worktree.

Quoted 3 lines

.gitignore works towards excluding files. No rule means no excluding. sparse-checkout file works towards including files, no rule means no inclusion.

Sure, but with a "sparse-skip" rather than "sparse-checkout", we would not even need an additional config variable, and the skip-worktree centered explanations would follow the same logic (no need for the additional negation) as the ignore files and the new sparse-skip file.

These are a few cleanups I noticed while checking Joshua's sparse checkout problem when swithcing branches with a dirty sparse tree.
They are independent of a possible fix.

Michael J Gruber (3):
unpack-trees: print "Aborting" to stderr git-read-tree.txt: language and typography fixes git-read-tree.txt: correct sparse-checkout and skip-worktree
description

 Documentation/git-read-tree.txt |   48 +++++++++++++++++++-------------------
 unpack-trees.c                  |    2 +-
 2 files changed, 25 insertions(+), 25 deletions(-)

Uhm, I forgot about the dont-cc-jc-in-rc policy, sorry. This is not a late-minute regression fix and can wait.