threads / discuss / 12104

git-status producing incorrect results

Subject: git-status producing incorrect results

## tl;dr

7 messages between Feb 14, 2008 and Feb 14, 2008.

replies: 6people: 3as markdown or json

Jeff King· Feb 14, 2008, 16:45 UTC · lore

There seems to be a bug in "git-status" in next (but not in master). I bisected it to:

  commit d1f2d7e8ca65504722108e2db710788f66c34c6c
  Author: Linus Torvalds <torvalds@linux-foundation.org>
  Date:   Sat Jan 19 17:27:12 2008 -0800
  Make run_diff_index() use unpack_trees(), not read_tree()

Basically, doing a partial commit when a new file has been added to the index but isn't part of the partial commit will cause that new file to be listed as part of the index. You can reproduce it with:

  mkdir trash && cd trash && git init &&
  touch file && git add file && git commit -m one &&
  touch added && git add added &&
  echo modified >file && git status file

Even more exciting, the later commit cf558704 causes an infinite loop (but still has the bad behavior), which is then fixed in 9cb76b8c (which still has the bad behavior, in addition to printing the "bug in wt_status_print_untracked" message to indicate that we found something funny in the index).

I _think_ of all of this is caused by the fact that builtin-commit looks in the regular index, but then gives us an alternate index. This didn't matter before because we were discarding the index so many time anyway.

The patch below fixes it by discarding and re-reading the index if we are doing a partial commit, but I suspect it may just be papering over the problem again. We probably need to have two separate index_states, and pass in the correct one to wt-status (rather than giving it the filename and having it read into the_index).

diff --git a/builtin-commit.c b/builtin-commit.c
index c63ff82..005362e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -313,6 +313,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	if (write_cache(fd, active_cache, active_nr) ||
 	    close_lock_file(&false_lock))
 		die("unable to write temporary index file");
+
+	discard_cache();
+	read_cache_from(false_lock.filename);
+
 	return false_lock.filename;
 }
 
-Peff
Johannes Schindelin· Feb 14, 2008, 16:51 UTC · re: Jeff King · lore

Re: git-status producing incorrect results

Hi,
On Thu, 14 Feb 2008, Jeff King wrote:
Show 12 quoted lines
> There seems to be a bug in "git-status" in next (but not in master). I
> bisected it to:
> 
>   commit d1f2d7e8ca65504722108e2db710788f66c34c6c
>   Author: Linus Torvalds <torvalds@linux-foundation.org>
>   Date:   Sat Jan 19 17:27:12 2008 -0800
> 
>   Make run_diff_index() use unpack_trees(), not read_tree()
> 
> Basically, doing a partial commit when a new file has been added to the
> index but isn't part of the partial commit will cause that new file to
> be listed as part of the index.

I experienced the same bug, but when I looked in the tests, I had the impression that it tested for that very bug, and succeeded. And I did not have time to look into it further.

Speaking of tests... How about it?

Ciao, Dscho

Jeff King· Feb 14, 2008, 16:54 UTC · re: Johannes Schindelin · lore

Re: git-status producing incorrect results

On Thu, Feb 14, 2008 at 04:51:04PM +0000, Johannes Schindelin wrote:
> I experienced the same bug, but when I looked in the tests, I had the 
> impression that it tested for that very bug, and succeeded.  And I did not 
> have time to look into it further.
Which test did you think was checking for it?
> Speaking of tests... How about it?

I included a "from scratch" test case in my last message which I think helps in understanding what's happening. But if we just want to test this condition (it fails because there is a new file that is added but not committed):

diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index b64ce30..11e5655 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -128,4 +128,8 @@ test_expect_success 'status without relative paths' '
 
 '
 
+test_expect_success 'status of partial commit excluding new file in index' '
+	git status modified
+'
+
 test_done
Johannes Schindelin· Feb 14, 2008, 17:02 UTC · re: Jeff King · lore

Re: git-status producing incorrect results

Hi,
On Thu, 14 Feb 2008, Jeff King wrote:
Show 7 quoted lines
> On Thu, Feb 14, 2008 at 04:51:04PM +0000, Johannes Schindelin wrote:
> 
> > I experienced the same bug, but when I looked in the tests, I had the 
> > impression that it tested for that very bug, and succeeded.  And I did 
> > not have time to look into it further.
> 
> Which test did you think was checking for it?

Well, I was looking at t7501-commit.sh, since I assumed that it not only _showed_ this status, but also _committed_ it. I did not even bother to try, but aborted the commit by deleting the whole commit message.

Show 12 quoted lines
> diff --git a/t/t7502-status.sh b/t/t7502-status.sh
> index b64ce30..11e5655 100755
> --- a/t/t7502-status.sh
> +++ b/t/t7502-status.sh
> @@ -128,4 +128,8 @@ test_expect_success 'status without relative paths' '
>  
>  '
>  
> +test_expect_success 'status of partial commit excluding new file in index' '
> +	git status modified
> +'
> +

That would need a redirection to "> output", and a known-good "expect", to make sure that it indeed works as expected.

Ciao, Dscho

Jeff King· Feb 14, 2008, 17:02 UTC · re: Jeff King · lore

Re: git-status producing incorrect results

On Thu, Feb 14, 2008 at 11:54:32AM -0500, Jeff King wrote:
Show 13 quoted lines
> diff --git a/t/t7502-status.sh b/t/t7502-status.sh
> index b64ce30..11e5655 100755
> --- a/t/t7502-status.sh
> +++ b/t/t7502-status.sh
> @@ -128,4 +128,8 @@ test_expect_success 'status without relative paths' '
>  
>  '
>  
> +test_expect_success 'status of partial commit excluding new file in index' '
> +	git status modified
> +'
> +
>  test_done

Actually, this only finds the problem in the current 'next', where git-status barfs with an error, and not in the first commit that exhibits the problem, where only the results are incorrect.

I tried making a "git commit" test, but it seems that the commit actually happens correctly (although if you do not specify -m, it does a run_status, and ends up barfing). So the problem really is that the index is screwed up after we decide what to commit but before we do the status.

-Peff
Linus Torvalds· Feb 14, 2008, 17:04 UTC · re: Jeff King · lore

Re: git-status producing incorrect results

On Thu, 14 Feb 2008, Jeff King wrote:
> 
> The patch below fixes it by discarding and re-reading the index if we
> are doing a partial commit
ACK. The partial commit case really should discard the index.
>                            but I suspect it may just be papering over
> the problem again. We probably need to have two separate index_states,
> and pass in the correct one to wt-status (rather than giving it the
> filename and having it read into the_index).

ACK again, that's almost certainly the right long-term fix, but it essentially requires a "copy_index()" thing that creates a new index from an existing copy (so that we don't need to re-read it) etc infrastructure changes, so I think your patch is the correct one for now.

		Linus
Jeff King· Feb 14, 2008, 17:18 UTC · re: Linus Torvalds · lore

Re: git-status producing incorrect results

On Thu, Feb 14, 2008 at 09:04:59AM -0800, Linus Torvalds wrote:
> ACK again, that's almost certainly the right long-term fix, but it 
> essentially requires a "copy_index()" thing that creates a new index from 
> an existing copy (so that we don't need to re-read it) etc infrastructure 
> changes, so I think your patch is the correct one for now.
Here is the cleaned-up mini-fix for now, then.

-- >8 -- commit: discard index after setting up partial commit

There may still be some entries from the original index that should be discarded before we show the status. In particular, if a file was added in the index but not included in the partial commit, it would still show up in the status listing as staged for commit.

Ultimately the correct fix is to keep the two states in separate index_state variables. Then we can avoid having to reload the cache from the temporary file altogether, and just point wt_status_print at the correct index.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-commit.c  |    4 ++++
 t/t7502-status.sh |   21 +++++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index 0442c8e..6612b4f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -317,6 +317,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	if (write_cache(fd, active_cache, active_nr) ||
 	    close_lock_file(&false_lock))
 		die("unable to write temporary index file");
+
+	discard_cache();
+	read_cache_from(false_lock.filename);
+
 	return false_lock.filename;
 }
 
diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index b64ce30..e006074 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -128,4 +128,25 @@ test_expect_success 'status without relative paths' '
 
 '
 
+cat <<EOF >expect
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/
+#	expect
+#	output
+#	untracked
+EOF
+test_expect_success 'status of partial commit excluding new file in index' '
+	git status dir1/modified >output &&
+	diff -u expect output
+'
+
 test_done

-- 
1.5.4.1.1324.gfec5c

← back to recent threads