git-status producing incorrect results

git-status producing incorrect results

[Jeff King]

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

Re: git-status producing incorrect results

[Johannes Schindelin]

Hi,

On Thu, 14 Feb 2008, Jeff King wrote:

> 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

Re: git-status producing incorrect results

[Jeff King]

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

Re: git-status producing incorrect results

[Johannes Schindelin]

Hi,

On Thu, 14 Feb 2008, Jeff King wrote:

> 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.

> 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

Re: git-status producing incorrect results

[Jeff King]

On Thu, Feb 14, 2008 at 11:54:32AM -0500, Jeff King wrote:

> 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

Re: git-status producing incorrect results

[Linus Torvalds]

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

Re: git-status producing incorrect results

[Jeff King]

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