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

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

[Carlos Martín Nieto]

Hello,

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

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.

   cmn

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

[Jeff King]

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

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

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

[Carlos Martín Nieto]

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

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

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> On Mon, Nov 07, 2011 at 11:26:42AM -0500, Jeff King wrote:
> ...
>> 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.

The variable names were chosen to mean "In a situation where the plumbing
traditionally would have said X, use this format to describe it". This is
the first topic to separate a single situation (from the plumbing's point
of view) into two and say different things at Porcelain, and the variable
naming no longer works.

An obvious solution would be to rename all of them to be based on "what
happened to the path". E.g. "modified_fmt" would be set to either "M" or
"needs update", and "removed_fmt" would be set to either "D" or "needs
update", etc.

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

[Jeff King]

On Fri, Nov 11, 2011 at 08:43:34AM -0800, Junio C Hamano wrote:

> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > On Mon, Nov 07, 2011 at 11:26:42AM -0500, Jeff King wrote:
> > ...
> >> 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.
> 
> The variable names were chosen to mean "In a situation where the plumbing
> traditionally would have said X, use this format to describe it". This is
> the first topic to separate a single situation (from the plumbing's point
> of view) into two and say different things at Porcelain, and the variable
> naming no longer works.

I agree the naming is awkward (but then, I think the "needs update"
message is awkward ;) ). But my interpretation was: if you want the
index to be in sync with the working tree, you must do this. Hence:

  $EDITOR file             ;# triggers needs_update
  git update-index file    ;# and do update in index
  rm file                  ;# triggers needs rm
  git rm --cached file     ;# and do rm in index

So that was my attempt to follow the same scheme, and I think it does
work. But I agree it's awkward, and since we're not changing the
plumbing message (nor do I think we should; most users won't see it, and
we would only be breaking scripts that do), it's not a big deal just to
change the variable names.

> An obvious solution would be to rename all of them to be based on "what
> happened to the path". E.g. "modified_fmt" would be set to either "M" or
> "needs update", and "removed_fmt" would be set to either "D" or "needs
> update", etc.

I'm happy to make that change. What do you think of the feature overall?
And should typechanges also be handled here (it's no extra work for git
to detect them; we just have to pass the TYPE_CHANGED flag back up the
stack)?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Nov 11, 2011 at 08:43:34AM -0800, Junio C Hamano wrote:
>
> I agree the naming is awkward (but then, I think the "needs update"
> message is awkward ;) ). But my interpretation was: if you want the
> index to be in sync with the working tree, you must do this. Hence:
>
>   $EDITOR file             ;# triggers needs_update
>   git update-index file    ;# and do update in index
>   rm file                  ;# triggers needs rm
>   git rm --cached file     ;# and do rm in index

The word "update" at the plumbing level simply means "update the path in
the index". At the Porcelain level you have to use "git add" or "git rm"
depending on different kind of "needs update", which may be argued an
inconsistency, but at the plumbing level everything is done with the same
update-index command, and "needs update" is the word that matches your
interpretation.

I however think the topic was about updating the Porclain-ish output that
is similar to "status -s" output. And at that level, paths marked with M/T
and D are dealt differently by end users ("add" vs "rm").

And the ? : expressions the patch is touching is about mapping the state
of the path to your "you must do this" interpretation. What is being
mapped is the state, so it would be more natural to have the variable
"modified_fmt" hold the result of the mapping "you must do...", instead of
calling the variable in terms of "you must do this if you were working at
the plumbing level", which is where the original needs_update_fmt naming
comes from.

>> An obvious solution would be to rename all of them to be based on "what
>> happened to the path". E.g. "modified_fmt" would be set to either "M" or
>> "needs update", and "removed_fmt" would be set to either "D" or "needs
>> update", etc.
>
> I'm happy to make that change. What do you think of the feature overall?

The "feature" is more or less "Meh" to me; I do not mind differentiating M
and D there because the necessary information is already there in the
codepath, but if we were to really do that, then the variables must be
renamed. We shouldn't name them after "you must do this at the plumbing
level" like we have done so far, and instead use "the path is in this
state". This is even more so if we were to further split a single "state"
that plumbing layer considers the same and gives the same "needs X" to
different states that Porcelains would give different messages in the
future.

> And should typechanges also be handled here (it's no extra work for git
> to detect them; we just have to pass the TYPE_CHANGED flag back up the
> stack)?

As long as "pass ... back up the stack" does not result in too much
ugliness in the code, I am OK with it.

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

[Jeff King]

On Fri, Nov 11, 2011 at 04:11:29PM -0800, Junio C Hamano wrote:

> > I'm happy to make that change. What do you think of the feature overall?
> 
> The "feature" is more or less "Meh" to me; I do not mind differentiating M
> and D there because the necessary information is already there in the
> codepath, but if we were to really do that, then the variables must be
> renamed. We shouldn't name them after "you must do this at the plumbing
> level" like we have done so far, and instead use "the path is in this
> state". This is even more so if we were to further split a single "state"
> that plumbing layer considers the same and gives the same "needs X" to
> different states that Porcelains would give different messages in the
> future.

I admit I don't care all that much either, but it's easy to do, and I
think it is less surprising to users. Patches are below.

> > And should typechanges also be handled here (it's no extra work for git
> > to detect them; we just have to pass the TYPE_CHANGED flag back up the
> > stack)?
> 
> As long as "pass ... back up the stack" does not result in too much
> ugliness in the code, I am OK with it.

It ended up pretty simple, but I split it out from the deletion case so
you can see it more clearly (and can drop the latter half of the series
if we don't want it).

  [1/4]: refresh_index: rename format variables
  [2/4]: refresh_index: mark deletions in porcelain output
  [3/4]: read-cache: let refresh_cache_ent pass up changed flags
  [4/4]: refresh_index: notice typechanges in output

(If I were sure we wanted the typechange output, I think a more
reasonable ordering would be 1, then 3, then squash 2 and 4 into a
single patch. But see my comment in 4/4).

-Peff

[PATCH 1/4] refresh_index: rename format variables

[Jeff King]

When refreshing the index, for modified (or unmerged) files
we will print "needs update" (or "needs merge") for plumbing,
or a "diff --name-status"-ish line for porcelain.

The variables holding which type of message to show are
named after the plumbing messages. However, as we begin to
differentiate more cases at the porcelain level (with the
plumbing message stayin the same), that naming scheme will
become awkward.

Instead, name the variables after which case we found
(modified or unmerged), not what we will output.

Signed-off-by: Jeff King <peff@peff.net>
---
 read-cache.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 5790a91..eb3aae3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1102,11 +1102,11 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	int first = 1;
 	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
-	const char *needs_update_fmt;
-	const char *needs_merge_fmt;
+	const char *modified_fmt;
+	const char *unmerged_fmt;
 
-	needs_update_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
-	needs_merge_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
+	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+	unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
 		int cache_errno = 0;
@@ -1122,7 +1122,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			i--;
 			if (allow_unmerged)
 				continue;
-			show_file(needs_merge_fmt, ce->name, in_porcelain, &first, header_msg);
+			show_file(unmerged_fmt, ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
@@ -1145,7 +1145,7 @@ 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);
+			show_file(modified_fmt, ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
-- 
1.7.7.3.8.g38efa

[PATCH 2/4] refresh_index: mark deletions in porcelain output

[Jeff King]

If you have a deleted file and a porcelain refreshes the
cache, we print:

  Unstaged changes after reset:
  M	file

This is technically correct, in that the file is modified,
but it's friendlier to the user if we further differentiate
the case of a deleted file (especially because this output
looks a lot like "diff --name-status", which would also make
the distinction).

The plumbing version stays as "needs update" for historical
compatibility.

Signed-off-by: Jeff King <peff@peff.net>
---
 read-cache.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index eb3aae3..046cf7e 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 *modified_fmt;
+	const char *deleted_fmt;
 	const char *unmerged_fmt;
 
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
 	unmerged_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,9 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			show_file(modified_fmt, ce->name, in_porcelain, &first, header_msg);
+			show_file((cache_errno == ENOENT ? deleted_fmt :
+				   modified_fmt),
+				  ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
-- 
1.7.7.3.8.g38efa

[PATCH 3/4] read-cache: let refresh_cache_ent pass up changed flags

[Jeff King]

This will enable refresh_cache to differentiate more cases
of modification (such as typechange) when telling the user
what isn't fresh.

Signed-off-by: Jeff King <peff@peff.net>
---
There's only a single layer of call-stack to modify, and there's only
one other caller, so it turned out to be a pretty minor change.

 read-cache.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 046cf7e..83fb19c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1001,7 +1001,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
  */
 static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 					     struct cache_entry *ce,
-					     unsigned int options, int *err)
+					     unsigned int options, int *err,
+					     int *changed_ret)
 {
 	struct stat st;
 	struct cache_entry *updated;
@@ -1033,6 +1034,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
 	}
 
 	changed = ie_match_stat(istate, ce, &st, options);
+	if (changed_ret)
+		*changed_ret = changed;
 	if (!changed) {
 		/*
 		 * The path is unchanged.  If we were told to ignore
@@ -1132,7 +1135,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 		if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
 			continue;
 
-		new = refresh_cache_ent(istate, ce, options, &cache_errno);
+		new = refresh_cache_ent(istate, ce, options, &cache_errno, NULL);
 		if (new == ce)
 			continue;
 		if (!new) {
@@ -1161,7 +1164,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 
 static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really)
 {
-	return refresh_cache_ent(&the_index, ce, really, NULL);
+	return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
 }
 
 static int verify_hdr(struct cache_header *hdr, unsigned long size)
-- 
1.7.7.3.8.g38efa

[PATCH 4/4] refresh_index: notice typechanges in output

[Jeff King]

If a file changes type and a porcelain updates the index, we
will print "M file". Instead, let's be more specific and
print "T file", which matches actual diff and status output.

The plumbing version remains "needs update" for historical
compatibility.

Signed-off-by: Jeff King <peff@peff.net>
---
The "changed" flag comes from refresh_cache_ent, which in turn gets it
from ie_modified_stat. The one hesitation I have is that intent-to-add
entries get the TYPE_CHANGED flag set, which means they will get a "T"
output. Whereas I actually think "M" is a little more sensible.

For "git reset", I'm not sure if it matters, since resetting the index
will always drop such an entry anyway. But you can see it with:

  git add -N file
  git add --refresh -v other

 read-cache.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 83fb19c..0e17add 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1107,14 +1107,17 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
 	const char *modified_fmt;
 	const char *deleted_fmt;
+	const char *typechange_fmt;
 	const char *unmerged_fmt;
 
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
 	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
+	typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
 	unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
 		int cache_errno = 0;
+		int changed = 0;
 
 		ce = istate->cache[i];
 		if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
@@ -1135,7 +1138,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 		if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
 			continue;
 
-		new = refresh_cache_ent(istate, ce, options, &cache_errno, NULL);
+		new = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);
 		if (new == ce)
 			continue;
 		if (!new) {
@@ -1151,6 +1154,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			if (quiet)
 				continue;
 			show_file((cache_errno == ENOENT ? deleted_fmt :
+				   changed & TYPE_CHANGED ? typechange_fmt :
 				   modified_fmt),
 				  ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
-- 
1.7.7.3.8.g38efa

Re: [PATCH 4/4] refresh_index: notice typechanges in output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If a file changes type and a porcelain updates the index, we
> will print "M file". Instead, let's be more specific and
> print "T file", which matches actual diff and status output.
>
> The plumbing version remains "needs update" for historical
> compatibility.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The "changed" flag comes from refresh_cache_ent, which in turn gets it
> from ie_modified_stat. The one hesitation I have is that intent-to-add
> entries get the TYPE_CHANGED flag set, which means they will get a "T"
> output. Whereas I actually think "M" is a little more sensible.

I agree that we should not say that an intent-to-add entry has changed
type relative to whatever, as by definition there is nothing to compare
against. "A" that stands for "A"dd is a lot more sensible here, I would
think.

 read-cache.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 0e17add..27e9fc6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1108,11 +1108,13 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	const char *modified_fmt;
 	const char *deleted_fmt;
 	const char *typechange_fmt;
+	const char *added_fmt;
 	const char *unmerged_fmt;
 
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
 	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
 	typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
+	added_fmt = (in_porcelain ? "A\t%s\n" : "%s needs update\n");
 	unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
@@ -1142,6 +1144,8 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 		if (new == ce)
 			continue;
 		if (!new) {
+			const char *fmt;
+
 			if (not_new && cache_errno == ENOENT)
 				continue;
 			if (really && cache_errno == EINVAL) {
@@ -1153,9 +1157,16 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			show_file((cache_errno == ENOENT ? deleted_fmt :
-				   changed & TYPE_CHANGED ? typechange_fmt :
-				   modified_fmt),
+
+			if (cache_errno == ENOENT)
+				fmt = deleted_fmt;
+			else if (ce->ce_flags & CE_INTENT_TO_ADD)
+				fmt = added_fmt; /* must be before other checks */
+			else if (changed & TYPE_CHANGED)
+				fmt = typechange_fmt;
+			else
+				fmt = modified_fmt;
+			show_file(fmt,
 				  ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;

Re: [PATCH 4/4] refresh_index: notice typechanges in output

[Jeff King]

On Mon, Nov 14, 2011 at 04:08:32PM -0800, Junio C Hamano wrote:

> I agree that we should not say that an intent-to-add entry has changed
> type relative to whatever, as by definition there is nothing to compare
> against. "A" that stands for "A"dd is a lot more sensible here, I would
> think.

Yeah, that makes sense to me.

> +			if (cache_errno == ENOENT)
> +				fmt = deleted_fmt;
> +			else if (ce->ce_flags & CE_INTENT_TO_ADD)
> +				fmt = added_fmt; /* must be before other checks */

Thanks, I was trying to figure out how to tell an intent-to-add file
from the 'changed' flag, but obviously looking for the bit in the cache
entry is the right thing.

Do you want to add your patch on top, or do you want me to re-roll with
this squashed in? I can also hold the re-roll until post-release if you
want.

-Peff

Re: [PATCH 4/4] refresh_index: notice typechanges in output

[Jeff King]

On Mon, Nov 14, 2011 at 09:05:06PM -0500, Jeff King wrote:

> Do you want to add your patch on top, or do you want me to re-roll with
> this squashed in? I can also hold the re-roll until post-release if you
> want.

You mentioned squashing in the "what's cooking" message. Rather than
squashing just the typechange bits, how about this re-roll, which I
think is a little easier to follow:

  [1/3]: read-cache: let refresh_cache_ent pass up changed flags
  [2/3]: refresh_index: rename format variables
  [3/3]: refresh_index: make porcelain output more specific

-Peff

[PATCHv2 1/3] read-cache: let refresh_cache_ent pass up changed flags

[Jeff King]

This will enable refresh_cache to differentiate more cases
of modification (such as typechange) when telling the user
what isn't fresh.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as 3/4 from my v1 series.

 read-cache.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 5790a91..8e69ea3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1001,7 +1001,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
  */
 static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 					     struct cache_entry *ce,
-					     unsigned int options, int *err)
+					     unsigned int options, int *err,
+					     int *changed_ret)
 {
 	struct stat st;
 	struct cache_entry *updated;
@@ -1033,6 +1034,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
 	}
 
 	changed = ie_match_stat(istate, ce, &st, options);
+	if (changed_ret)
+		*changed_ret = changed;
 	if (!changed) {
 		/*
 		 * The path is unchanged.  If we were told to ignore
@@ -1130,7 +1133,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 		if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
 			continue;
 
-		new = refresh_cache_ent(istate, ce, options, &cache_errno);
+		new = refresh_cache_ent(istate, ce, options, &cache_errno, NULL);
 		if (new == ce)
 			continue;
 		if (!new) {
@@ -1157,7 +1160,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 
 static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really)
 {
-	return refresh_cache_ent(&the_index, ce, really, NULL);
+	return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
 }
 
 static int verify_hdr(struct cache_header *hdr, unsigned long size)
-- 
1.7.7.3.8.g38efa

[PATCHv2 2/3] refresh_index: rename format variables

[Jeff King]

When refreshing the index, for modified (or unmerged) files
we will print "needs update" (or "needs merge") for plumbing,
or a "diff --name-status"-ish line for porcelain.

The variables holding which type of message to show are
named after the plumbing messages. However, as we begin to
differentiate more cases at the porcelain level (with the
plumbing message stayin the same), that naming scheme will
become awkward.

Instead, name the variables after which case we found
(modified or unmerged), not what we will output.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as 1/4 from my v1 series.

 read-cache.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 8e69ea3..f19b2f1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1105,11 +1105,11 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	int first = 1;
 	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
-	const char *needs_update_fmt;
-	const char *needs_merge_fmt;
+	const char *modified_fmt;
+	const char *unmerged_fmt;
 
-	needs_update_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
-	needs_merge_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
+	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+	unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
 		int cache_errno = 0;
@@ -1125,7 +1125,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			i--;
 			if (allow_unmerged)
 				continue;
-			show_file(needs_merge_fmt, ce->name, in_porcelain, &first, header_msg);
+			show_file(unmerged_fmt, ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
@@ -1148,7 +1148,7 @@ 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);
+			show_file(modified_fmt, ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
-- 
1.7.7.3.8.g38efa

[PATCH 3/3] refresh_index: make porcelain output more specific

[Jeff King]

If you have a deleted file and a porcelain refreshes the
cache, we print:

  Unstaged changes after reset:
  M	file

This is technically correct, in that the file is modified,
but it's friendlier to the user if we further differentiate
the case of a deleted file (especially because this output
looks a lot like "diff --name-status", which would also make
the distinction).

Similarly, we can distinguish typechanges ("T") and
intent-to-add files ("A"), both of which appear as just "M"
in the current output.

The plumbing output for all cases remains "needs update" for
historical compatibility.

Signed-off-by: Jeff King <peff@peff.net>
---
This is my deletion and typechange patches squashed together with your
intent-to-add, and using an if/else series instead of the ternary
operator, as you did.

 read-cache.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f19b2f1..27e9fc6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1106,13 +1106,20 @@ 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 *modified_fmt;
+	const char *deleted_fmt;
+	const char *typechange_fmt;
+	const char *added_fmt;
 	const char *unmerged_fmt;
 
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
+	typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
+	added_fmt = (in_porcelain ? "A\t%s\n" : "%s needs update\n");
 	unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
 		int cache_errno = 0;
+		int changed = 0;
 
 		ce = istate->cache[i];
 		if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
@@ -1133,10 +1140,12 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 		if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
 			continue;
 
-		new = refresh_cache_ent(istate, ce, options, &cache_errno, NULL);
+		new = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);
 		if (new == ce)
 			continue;
 		if (!new) {
+			const char *fmt;
+
 			if (not_new && cache_errno == ENOENT)
 				continue;
 			if (really && cache_errno == EINVAL) {
@@ -1148,7 +1157,17 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			show_file(modified_fmt, ce->name, in_porcelain, &first, header_msg);
+
+			if (cache_errno == ENOENT)
+				fmt = deleted_fmt;
+			else if (ce->ce_flags & CE_INTENT_TO_ADD)
+				fmt = added_fmt; /* must be before other checks */
+			else if (changed & TYPE_CHANGED)
+				fmt = typechange_fmt;
+			else
+				fmt = modified_fmt;
+			show_file(fmt,
+				  ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
-- 
1.7.7.3.8.g38efa

Re: [PATCH 4/4] refresh_index: notice typechanges in output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Nov 14, 2011 at 09:05:06PM -0500, Jeff King wrote:
>
>> Do you want to add your patch on top, or do you want me to re-roll with
>> this squashed in? I can also hold the re-roll until post-release if you
>> want.
>
> You mentioned squashing in the "what's cooking" message. Rather than
> squashing just the typechange bits, how about this re-roll, which I
> think is a little easier to follow:
>
>   [1/3]: read-cache: let refresh_cache_ent pass up changed flags
>   [2/3]: refresh_index: rename format variables
>   [3/3]: refresh_index: make porcelain output more specific

Looks sensible; thanks. Will replace.