{"thread":{"id":"12104","subject":"git-status producing incorrect results","startedAt":"2008-02-14T16:45:05Z","lastAt":"2008-02-14T17:18:23Z","messageCount":7,"participants":["Jeff King","Johannes Schindelin","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"68744","messageId":"20080214164505.GA21932@coredump.intra.peff.net","threadId":"12104","inReplyTo":null,"subject":"git-status producing incorrect results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-14T16:45:05Z","receivedAt":"2008-02-14T16:45:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There seems to be a bug in \"git-status\" in next (but not in master). I\nbisected it to:\n\n commit d1f2d7e8ca65504722108e2db710788f66c34c6c\n Author: Linus Torvalds \n Date: Sat Jan 19 17:27:12 2008 -0800\n\n Make run_diff_index() use unpack_trees(), not read_tree()\n\nBasically, doing a partial commit when a new file has been added to the\nindex but isn't part of the partial commit will cause that new file to\nbe listed as part of the index. You can reproduce it with:\n\n mkdir trash && cd trash && git init &&\n touch file && git add file && git commit -m one &&\n touch added && git add added &&\n echo modified >file && git status file\n\nEven more exciting, the later commit cf558704 causes an infinite loop\n(but still has the bad behavior), which is then fixed in 9cb76b8c (which\nstill has the bad behavior, in addition to printing the \"bug in\nwt_status_print_untracked\" message to indicate that we found something\nfunny in the index).\n\nI _think_ of all of this is caused by the fact that builtin-commit looks\nin the regular index, but then gives us an alternate index. This didn't\nmatter before because we were discarding the index so many time anyway.\n\nThe patch below fixes it by discarding and re-reading the index if we\nare doing a partial commit, but I suspect it may just be papering over\nthe problem again. We probably need to have two separate index_states,\nand pass in the correct one to wt-status (rather than giving it the\nfilename and having it read into the_index).\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex c63ff82..005362e 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -313,6 +313,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \tif (write_cache(fd, active_cache, active_nr) ||\n \t close_lock_file(&false_lock))\n \t\tdie(\"unable to write temporary index file\");\n+\n+\tdiscard_cache();\n+\tread_cache_from(false_lock.filename);\n+\n \treturn false_lock.filename;\n }\n \n-Peff\n"},{"id":"68746","messageId":"alpine.LSU.1.00.0802141650080.30505@racer.site","threadId":"12104","inReplyTo":"20080214164505.GA21932@coredump.intra.peff.net","subject":"Re: git-status producing incorrect results","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-14T16:51:04Z","receivedAt":"2008-02-14T16:51:04Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 14 Feb 2008, Jeff King wrote:\n\n> There seems to be a bug in \"git-status\" in next (but not in master). I\n> bisected it to:\n> \n> commit d1f2d7e8ca65504722108e2db710788f66c34c6c\n> Author: Linus Torvalds \n> Date: Sat Jan 19 17:27:12 2008 -0800\n> \n> Make run_diff_index() use unpack_trees(), not read_tree()\n> \n> Basically, doing a partial commit when a new file has been added to the\n> index but isn't part of the partial commit will cause that new file to\n> be listed as part of the index.\n\nI experienced the same bug, but when I looked in the tests, I had the \nimpression that it tested for that very bug, and succeeded. And I did not \nhave time to look into it further.\n\nSpeaking of tests... How about it?\n\nCiao,\nDscho\n"},{"id":"68747","messageId":"20080214165432.GA28059@coredump.intra.peff.net","threadId":"12104","inReplyTo":"alpine.LSU.1.00.0802141650080.30505@racer.site","subject":"Re: git-status producing incorrect results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-14T16:54:32Z","receivedAt":"2008-02-14T16:54:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2008 at 04:51:04PM +0000, Johannes Schindelin wrote:\n\n> I experienced the same bug, but when I looked in the tests, I had the \n> impression that it tested for that very bug, and succeeded. And I did not \n> have time to look into it further.\n\nWhich test did you think was checking for it?\n\n> Speaking of tests... How about it?\n\nI included a \"from scratch\" test case in my last message which I think\nhelps in understanding what's happening. But if we just want to test\nthis condition (it fails because there is a new file that is added but\nnot committed):\n\ndiff --git a/t/t7502-status.sh b/t/t7502-status.sh\nindex b64ce30..11e5655 100755\n--- a/t/t7502-status.sh\n+++ b/t/t7502-status.sh\n@@ -128,4 +128,8 @@ test_expect_success 'status without relative paths' '\n \n '\n \n+test_expect_success 'status of partial commit excluding new file in index' '\n+\tgit status modified\n+'\n+\n test_done\n"},{"id":"68748","messageId":"alpine.LSU.1.00.0802141700450.30505@racer.site","threadId":"12104","inReplyTo":"20080214165432.GA28059@coredump.intra.peff.net","subject":"Re: git-status producing incorrect results","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-14T17:02:29Z","receivedAt":"2008-02-14T17:02:29Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 14 Feb 2008, Jeff King wrote:\n\n> On Thu, Feb 14, 2008 at 04:51:04PM +0000, Johannes Schindelin wrote:\n> \n> > I experienced the same bug, but when I looked in the tests, I had the \n> > impression that it tested for that very bug, and succeeded. And I did \n> > not have time to look into it further.\n> \n> Which test did you think was checking for it?\n\nWell, I was looking at t7501-commit.sh, since I assumed that it not only \n_showed_ this status, but also _committed_ it. I did not even bother to \ntry, but aborted the commit by deleting the whole commit message.\n\n> diff --git a/t/t7502-status.sh b/t/t7502-status.sh\n> index b64ce30..11e5655 100755\n> --- a/t/t7502-status.sh\n> +++ b/t/t7502-status.sh\n> @@ -128,4 +128,8 @@ test_expect_success 'status without relative paths' '\n> \n> '\n> \n> +test_expect_success 'status of partial commit excluding new file in index' '\n> +\tgit status modified\n> +'\n> +\n\nThat would need a redirection to \"> output\", and a known-good \"expect\", to \nmake sure that it indeed works as expected.\n\nCiao,\nDscho\n"},{"id":"68749","messageId":"20080214170250.GB28059@coredump.intra.peff.net","threadId":"12104","inReplyTo":"20080214165432.GA28059@coredump.intra.peff.net","subject":"Re: git-status producing incorrect results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-14T17:02:50Z","receivedAt":"2008-02-14T17:02:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2008 at 11:54:32AM -0500, Jeff King wrote:\n\n> diff --git a/t/t7502-status.sh b/t/t7502-status.sh\n> index b64ce30..11e5655 100755\n> --- a/t/t7502-status.sh\n> +++ b/t/t7502-status.sh\n> @@ -128,4 +128,8 @@ test_expect_success 'status without relative paths' '\n> \n> '\n> \n> +test_expect_success 'status of partial commit excluding new file in index' '\n> +\tgit status modified\n> +'\n> +\n> test_done\n\nActually, this only finds the problem in the current 'next', where\ngit-status barfs with an error, and not in the first commit that\nexhibits the problem, where only the results are incorrect.\n\nI tried making a \"git commit\" test, but it seems that the commit\nactually happens correctly (although if you do not specify -m, it does a\nrun_status, and ends up barfing). So the problem really is that the\nindex is screwed up after we decide what to commit but before we do the\nstatus.\n\n-Peff\n"},{"id":"68750","messageId":"alpine.LFD.1.00.0802140902470.6110@woody.linux-foundation.org","threadId":"12104","inReplyTo":"20080214164505.GA21932@coredump.intra.peff.net","subject":"Re: git-status producing incorrect results","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-02-14T17:04:59Z","receivedAt":"2008-02-14T17:04:59Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 14 Feb 2008, Jeff King wrote:\n> \n> The patch below fixes it by discarding and re-reading the index if we\n> are doing a partial commit\n\nACK. The partial commit case really should discard the index.\n\n> but I suspect it may just be papering over\n> the problem again. We probably need to have two separate index_states,\n> and pass in the correct one to wt-status (rather than giving it the\n> filename and having it read into the_index).\n\nACK again, that's almost certainly the right long-term fix, but it \nessentially requires a \"copy_index()\" thing that creates a new index from \nan existing copy (so that we don't need to re-read it) etc infrastructure \nchanges, so I think your patch is the correct one for now.\n\n\t\tLinus\n"},{"id":"68751","messageId":"20080214171823.GA28762@coredump.intra.peff.net","threadId":"12104","inReplyTo":"alpine.LFD.1.00.0802140902470.6110@woody.linux-foundation.org","subject":"Re: git-status producing incorrect results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-14T17:18:23Z","receivedAt":"2008-02-14T17:18:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2008 at 09:04:59AM -0800, Linus Torvalds wrote:\n\n> ACK again, that's almost certainly the right long-term fix, but it \n> essentially requires a \"copy_index()\" thing that creates a new index from \n> an existing copy (so that we don't need to re-read it) etc infrastructure \n> changes, so I think your patch is the correct one for now.\n\nHere is the cleaned-up mini-fix for now, then.\n\n-- >8 --\ncommit: discard index after setting up partial commit\n\nThere may still be some entries from the original index that\nshould be discarded before we show the status. In\nparticular, if a file was added in the index but not\nincluded in the partial commit, it would still show up in\nthe status listing as staged for commit.\n\nUltimately the correct fix is to keep the two states in\nseparate index_state variables. Then we can avoid having\nto reload the cache from the temporary file altogether, and\njust point wt_status_print at the correct index.\n\nSigned-off-by: Jeff King \n---\n builtin-commit.c | 4 ++++\n t/t7502-status.sh | 21 +++++++++++++++++++++\n 2 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 0442c8e..6612b4f 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -317,6 +317,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \tif (write_cache(fd, active_cache, active_nr) ||\n \t close_lock_file(&false_lock))\n \t\tdie(\"unable to write temporary index file\");\n+\n+\tdiscard_cache();\n+\tread_cache_from(false_lock.filename);\n+\n \treturn false_lock.filename;\n }\n \ndiff --git a/t/t7502-status.sh b/t/t7502-status.sh\nindex b64ce30..e006074 100755\n--- a/t/t7502-status.sh\n+++ b/t/t7502-status.sh\n@@ -128,4 +128,25 @@ test_expect_success 'status without relative paths' '\n \n '\n \n+cat <expect\n+# On branch master\n+# Changes to be committed:\n+# (use \"git reset HEAD ...\" to unstage)\n+#\n+#\tmodified: dir1/modified\n+#\n+# Untracked files:\n+# (use \"git add ...\" to include in what will be committed)\n+#\n+#\tdir1/untracked\n+#\tdir2/\n+#\texpect\n+#\toutput\n+#\tuntracked\n+EOF\n+test_expect_success 'status of partial commit excluding new file in index' '\n+\tgit status dir1/modified >output &&\n+\tdiff -u expect output\n+'\n+\n test_done\n-- \n1.5.4.1.1324.gfec5c\n"}]}