Re: reset reports file as modified when it's in fact deleted

From: Jeff King <peff@peff.net>
Date: Nov 7, 2011, 16:26 UTC
Message-ID: <20111107162642.GA27055@sigill.intra.peff.net>
In-Reply-To: <20111107094330.GB10936@beez.lab.cmartin.tk>

On Mon, Nov 07, 2011 at 10:43:30AM +0100, Carlos Martín Nieto wrote:

Show 21 quoted lines

> When I delete a file (git rm) and then reset so it exists in the index
> again, the message tells me 'M file.txt' even though the file doesn't
> exist in the worktree anymore. Running git status afterwards does give
> the correct output. So, here's the minimal steps to reproduce:
> 
>     $ git init
>     Initialized empty Git repository in /home/carlos/test/reset-err/.git/
>     $ touch file.txt
>     $ git add file.txt
>     $ git ci file.txt -m initial
>     [master (root-commit) a536393] initial
>      0 files changed, 0 insertions(+), 0 deletions(-)
>      create mode 100644 file.txt
>     $ git rm file.txt
>     rm 'file.txt'
>     $ git reset -- file.txt
>     Unstaged changes after reset:
>     M		 file.txt
>     $ git status -b -s
>     ## master
>      D file.txt

You can simplify this even further by not touching the index at all:

  git init -q &&
  >file && git add file && git commit -q -m initial &&
  rm file &&
  git reset

produces:

  Unstaged changes after reset:
  M       file

Show 8 quoted lines

> I'd expect the output after "Unstaged changes after reset" to tell me
> file.txt has been deleted instead of modified. This happens with
> 1.7.8-rc0, 1.7.7 and 1.7.4.1 and I expect with many more that I don't
> have here.
> 
> I thought the index diff code might have been checking the index at the
> wrong time, but I can run 'git reset HEAD -- file.txt' as many times
> as I want, and it will still say 'M', so now I'm not sure.

The index diff code isn't running at all. Those messages are produced by refresh_index, which outputs only two flags: modified or unmerged. I think the reason for this is somewhat historical. You would run "update-index --refresh", and it would helpfully say "by the way, when refreshing this entry, I noticed that it is in need of being updated in the index". The text was "file.txt: needs update".

Later, many porcelain commands started to refresh the index themselves, and the "needs update" message was very confusing. So it was switched to the more familiar "M file.txt" (though you can still see the original plumbing message if you run update-index yourself).

I think it is a little more friendly to distinguish deletion from just modification. And there's shouldn't be any compatibility questions, as these are explicitly porcelain output (plumbing will still say "needs update").

There are a few other cases users might expect to see. We'll never show copies or renames, of course, because we aren't actually doing a diff with copy detection. We won't see an "A"dded file, because such a file would be in the working tree but not the index, meaning it is not tracked.

We could see a "T"ypechange, but the distinction between that and a modified file is lost by the time we get to refresh_index. We could pass it up, but I wonder if it's really worth it.

The patch to do "D"eleted is pretty simple:

diff --git a/read-cache.c b/read-cache.c
index dea7cd8..cc1ebdf 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1103,9 +1103,11 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
 	const char *needs_update_fmt;
+	const char *needs_rm_fmt;
 	const char *needs_merge_fmt;
 
 	needs_update_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+	needs_rm_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
 	needs_merge_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
@@ -1145,7 +1147,10 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			show_file(needs_update_fmt, ce->name, in_porcelain, &first, header_msg);
+			if (cache_errno == ENOENT)
+				show_file(needs_rm_fmt, ce->name, in_porcelain, &first, header_msg);
+			else
+				show_file(needs_update_fmt, ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}