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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Attachment:
signature.asc
Description: Digital signature