Thread (3 messages) 3 messages, 2 authors, 2016-06-15

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

flat view

From: Carlos Martín Nieto <hidden>
Date: 2016-06-15 22:52:28

On Mon, Nov 07, 2011 at 11:26:42AM -0500, Jeff King wrote:
On Mon, Nov 07, 2011 at 10:43:30AM +0100, Carlos Martín Nieto wrote:
quoted
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
Ah, I see. I got the previous sequence because that's what we have in
an instruction manual and where we saw it.
quoted
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.
That's probably overkill. The issue with reporting 'M' for a deleted
file is that it conflicts with what the status output would say, even
though it's in the same format.
quoted hunk ↗ jump to hunk
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");
While the name fits in with the rest of the variables, it's kind of
the wrong way around, isn't it? It doesn't need an 'rm', it /was/
rm'd. Other than that stupid thing, the patch works great, thanks. Can
we get it into git?
quoted hunk ↗ jump to hunk
 	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;
 		}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help