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

["Carlos Martín Nieto" <cmn@elego.de>]

[-- Attachment #1: Type: text/plain, Size: 5184 bytes --]

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

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

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]