Message from git reset: confusing?

Message from git reset: confusing?

[Matthieu Moy]

Hi,

I was wondering what was the motivation for the output of "git merge":

$ git reset file
file: locally modified
$ 

I find this message misleading, since it gives me the feeling that
"git reset" errored out because of the file being locally modified (I
also got the remark from a git beginner to whom I was showing the
command: "hey, why didn't it work??").

And indeed, I do not understand the motivation for showing this
message. When I stage content (git add), Git tells me nothing, so why
should it do so when I unstage content? If I want to know the state of
the index after running reset, I'll run "git status" myself ...

I'd suggest adding a -v|--verbose flag to git reset, and default to
being quiet (the already existing -q flag would still be used, to
disable progress indicator). What do other people think?

-- 
Matthieu

Re: Message from git reset: confusing?

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> I was wondering what was the motivation for the output of "git merge":

You meant "git reset".

> $ git reset file
> file: locally modified
> $ 

First a tangent.

Removing this output _when no <path> is given_ would greatly reduce the
usability of the command.

I often find myself working on something that consists of more than one
steps, and initially I decide that these would form, say, two commits A,
and B.  I first start adding the changes that are relevant to A, and my
index gradually gets closer to what A should finally look like.

But then I realize that logically commit B should come before commit A,
and it is time to "git reset" without any <path>s.  The output would let
me review the changes (this includes changes pertaining to both A and B)
to help me recall which files would contain changes that are relevant to
B.

I agree that "git reset a-single-exact-filename" could be much more
silent.  I would even say we do not even need -v in such a case.

But the thing is, that is a very narrow special case.  The parameter the
command takes is not _a file_, but is a set of pathspecs, and I would
imagine that when you are in a situation similar to what I just described
in a larger project, you would appreciate the same reminder of modified
paths when you run the command like this:

    $ git reset include/ arch/x86/

I wouldn't oppose to a patch that squelches the output when all pathspecs
given from the command line _exactly_ name existing paths, but I tend to
think that it would be usability regression if you do not show any output
in a case like the last example.

Re: Message from git reset: confusing?

[Avery Pennarun]

On Wed, Aug 5, 2009 at 5:21 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> I was wondering what was the motivation for the output of "git merge":
>
> You meant "git reset".
>
>> $ git reset file
>> file: locally modified
>> $
>
> First a tangent.
>
> Removing this output _when no <path> is given_ would greatly reduce the
> usability of the command.

Yes.  I think the problem is that the current output looks more like
an error message than a status report.

I would find it very pleasant if the output looked more like the
output of "git checkout" (no parameters) in the no-files-specified
case.

Even if people don't know what "M" means on day 1 (although hopefully
they don't need "git reset" on day 1), at least it doesn't look like
an error message.

Have fun,

Avery

Re: Message from git reset: confusing?

[John Tapsell]

> Even if people don't know what "M" means on day 1 (although hopefully
> they don't need "git reset" on day 1), at least it doesn't look like
> an error message.

Actually for newbies, git reset is pretty much the first thing they
learn to try to undo the mess that they created.

Almost the very first thing a newbie to git does is something like:

$ git checkout experimental_branch
<modify file>
$ git commit
..
(what do you mean I've committed to a detached head?  why is
everything going wrong?  what's happening!!)

$ git reset

Re: Message from git reset: confusing?

[Sverre Rabbelier]

Heya,

On Wed, Aug 5, 2009 at 10:42, Avery Pennarun<apenwarr@gmail.com> wrote:
> Yes.  I think the problem is that the current output looks more like
> an error message than a status report.

Definitely.

> I would find it very pleasant if the output looked more like the
> output of "git checkout" (no parameters) in the no-files-specified
> case.

Perhaps instead we could get away with simply adding a header like
'git status' does? And perhaps change the wording some.

-- 
Cheers,

Sverre Rabbelier

Re: Message from git reset: confusing?

[Matthieu Moy]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Wed, Aug 5, 2009 at 10:42, Avery Pennarun<apenwarr@gmail.com> wrote:
>> Yes.  I think the problem is that the current output looks more like
>> an error message than a status report.
>
> Definitely.

This is my biggest issue, indeed. Actually, several things bother me
(by decreasing annoyance):

1) It looks like an error message, and user can think git reset
failed.

2) It's inconsistant with the usual status display. I'd prefer an
output like "git diff --name-only" or "git status".

3) It's verbose. Junio's reply addresses this point. I'm not really
convinced that verbosity by default is a good thing, but I don't think
I can convince Junio either ;-), and I don't care that much.

So, let's address 1) and 2) only.

>> I would find it very pleasant if the output looked more like the
>> output of "git checkout" (no parameters) in the no-files-specified
>> case.
>
> Perhaps instead we could get away with simply adding a header like
> 'git status' does? And perhaps change the wording some.

Just this patch would already solve 1) above mostly. At first, I
wanted the message to say "reset successfull", but it's harder than it
seems, since the output is printf-ed as the work is done, so one knows
that reset is successful only after displaying the whole thing:

diff --git a/builtin-reset.c b/builtin-reset.c
index 5fa1789..6b16a00 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -108,6 +108,7 @@ static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)
        if (read_cache() < 0)
                return error("Could not read index");
 
+       printf("Unstaged changes after reset:\n");
        result = refresh_cache(flags) ? 1 : 0;
        if (write_cache(fd, active_cache, active_nr) ||
                        commit_locked_index(index_lock))

For 2), something along the lines of:

diff --git a/read-cache.c b/read-cache.c
index 4e3e272..3a99a2b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1075,10 +1075,13 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
        int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
        int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
        unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
-       const char *needs_update_message;
+       const char *needs_update_fmt;
+       const char *needs_merge_fmt;
 
-       needs_update_message = ((flags & REFRESH_SAY_CHANGED)
-                               ? "locally modified" : "needs update");
+       needs_update_fmt = ((flags & REFRESH_SAY_CHANGED)
+                               ? "M\t%s\n" : "%s: needs update\n");
+       needs_merge_fmt = ((flags & REFRESH_SAY_CHANGED)
+                               ? "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;
@@ -1094,7 +1097,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
                        i--;
                        if (allow_unmerged)
                                continue;
-                       printf("%s: needs merge\n", ce->name);
+                       printf(needs_merge_fmt, ce->name);
                        has_errors = 1;
                        continue;
                }
@@ -1117,7 +1120,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
                        }
                        if (quiet)
                                continue;
-                       printf("%s: %s\n", ce->name, needs_update_message);
+                       printf(needs_update_fmt, ce->name);
                        has_errors = 1;
                        continue;
                }

would do. See d14e7407b3 ("needs update" considered harmful, Sun Jul
20 00:21:38 2008) for the previous improvement on the subject. The
problem with this second patch is that it says "M" where "diff
--name-status" would say "D" for example, which is a bit strange. If
the idea of the patch is accepted, REFRESH_SAY_CHANGED should also be
renamed to reflect its new nature, to stg like
REFRESH_PORCELAIN_OUTPUT.

Any opinion? Am I going in the right direction?

-- 
Matthieu

Re: Message from git reset: confusing?

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> This is my biggest issue, indeed. Actually, several things bother me
> (by decreasing annoyance):
>
> 1) It looks like an error message, and user can think git reset
> failed.
>
> 2) It's inconsistant with the usual status display. I'd prefer an
> output like "git diff --name-only" or "git status".
> ...
> So, let's address 1) and 2) only.
>
> Any opinion? Am I going in the right direction?

I think the approach is sane.  I didn't check if the unconditional 

	printf("Unstaged changes after reset:\n");

needs to be protected, or at that point we already know we will talk about
one or more paths (either needs update or needs merge).

[PATCH 1/2] Rename REFRESH_SAY_CHANGED to REFRESH_IN_PORCELAIN.

[Matthieu Moy]

The change in the output is going to become more general than just saying
"changed", so let's make the variable name more general too.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin-add.c   |    2 +-
 builtin-reset.c |    4 ++--
 cache.h         |    2 +-
 read-cache.c    |    2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 581a2a1..a325bc9 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -105,7 +105,7 @@ static void refresh(int verbose, const char **pathspec)
 	for (specs = 0; pathspec[specs];  specs++)
 		/* nothing */;
 	seen = xcalloc(specs, 1);
-	refresh_index(&the_index, verbose ? REFRESH_SAY_CHANGED : REFRESH_QUIET,
+	refresh_index(&the_index, verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET,
 		      pathspec, seen);
 	for (i = 0; i < specs; i++) {
 		if (!seen[i])
diff --git a/builtin-reset.c b/builtin-reset.c
index 5fa1789..ddf68d5 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -261,7 +261,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			die("Cannot do %s reset with paths.",
 					reset_type_names[reset_type]);
 		return read_from_tree(prefix, argv + i, sha1,
-				quiet ? REFRESH_QUIET : REFRESH_SAY_CHANGED);
+				quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
 	}
 	if (reset_type == NONE)
 		reset_type = MIXED; /* by default */
@@ -302,7 +302,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		break;
 	case MIXED: /* Report what has not been updated. */
 		update_index_refresh(0, NULL,
-				quiet ? REFRESH_QUIET : REFRESH_SAY_CHANGED);
+				quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
 		break;
 	}
 
diff --git a/cache.h b/cache.h
index e6c7f33..a2f2923 100644
--- a/cache.h
+++ b/cache.h
@@ -473,7 +473,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
 #define REFRESH_QUIET		0x0004	/* be quiet about it */
 #define REFRESH_IGNORE_MISSING	0x0008	/* ignore non-existent */
 #define REFRESH_IGNORE_SUBMODULES	0x0010	/* ignore submodules */
-#define REFRESH_SAY_CHANGED	0x0020	/* say "changed" not "needs update" */
+#define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 extern int refresh_index(struct index_state *, unsigned int flags, const char **pathspec, char *seen);
 
 struct lock_file {
diff --git a/read-cache.c b/read-cache.c
index 4e3e272..f1aff81 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1077,7 +1077,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
 	const char *needs_update_message;
 
-	needs_update_message = ((flags & REFRESH_SAY_CHANGED)
+	needs_update_message = ((flags & REFRESH_IN_PORCELAIN)
 				? "locally modified" : "needs update");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
-- 
1.6.4.62.g39c83.dirty

[PATCH 2/2 (v2)] reset: make the output more user-friendly.

[Matthieu Moy]

git reset without argument displays a summary of the remaining
unstaged changes. The problem with these is that they look like an
error message, and the format is inconsistant with the format used in
other places like "git diff --name-status".

This patch mimics the output of "git diff --name-status", and adds a
header to make it clear the output is informative, and not an error.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

As noted by Junio, my previous version did actually display the header
unconditionnaly. This version displays it as the first change is found.

 read-cache.c     |   22 +++++++++++++++++-----
 t/t7102-reset.sh |    3 ++-
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f1aff81..4a4f4a5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1065,6 +1065,15 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	return updated;
 }
 
+static void show_file(const char * fmt, const char * name, int in_porcelain, int * first)
+{
+	if (in_porcelain && *first) {
+		printf("Unstaged changes after reset:\n");
+		*first=0;
+	}
+	printf(fmt, name);
+}
+
 int refresh_index(struct index_state *istate, unsigned int flags, const char **pathspec, char *seen)
 {
 	int i;
@@ -1074,11 +1083,14 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	int quiet = (flags & REFRESH_QUIET) != 0;
 	int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
 	int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
+	int first = 1;
+	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
-	const char *needs_update_message;
+	const char *needs_update_fmt;
+	const char *needs_merge_fmt;
 
-	needs_update_message = ((flags & REFRESH_IN_PORCELAIN)
-				? "locally modified" : "needs update");
+	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");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
 		int cache_errno = 0;
@@ -1094,7 +1106,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			i--;
 			if (allow_unmerged)
 				continue;
-			printf("%s: needs merge\n", ce->name);
+			show_file(needs_merge_fmt, ce->name, in_porcelain, &first);
 			has_errors = 1;
 			continue;
 		}
@@ -1117,7 +1129,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			printf("%s: %s\n", ce->name, needs_update_message);
+			show_file(needs_update_fmt, ce->name, in_porcelain, &first);
 			has_errors = 1;
 			continue;
 		}
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index e637c7d..e85ff02 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -419,7 +419,8 @@ test_expect_success 'resetting an unmodified path is a no-op' '
 '
 
 cat > expect << EOF
-file2: locally modified
+Unstaged changes after reset:
+M	file2
 EOF
 
 test_expect_success '--mixed refreshes the index' '
-- 
1.6.4.62.g39c83.dirty

Re: [PATCH 2/2 (v2)] reset: make the output more user-friendly.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

>  cat > expect << EOF
> -file2: locally modified
> +Unstaged changes after reset:
> +M	file2

It simply feels backwards when plumbing output says something in human
language (e.g. "needs update") while Porcelain output spits out a cryptic
M or U.  If the goal is human-readability and user-friendliness, shouldn't
we rather say:

	Path with local modifications:
        	file2

or something?

Re: [PATCH 2/2 (v2)] reset: make the output more user-friendly.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>>  cat > expect << EOF
>> -file2: locally modified
>> +Unstaged changes after reset:
>> +M	file2
>
> It simply feels backwards when plumbing output says something in human
> language (e.g. "needs update") while Porcelain output spits out a cryptic
> M or U.  If the goal is human-readability and user-friendliness,

The goal here is just consistency.

And I do consider 'git diff --name-status' as porcelain.

> shouldn't we rather say:
>
> 	Path with local modifications:
>         	file2
>
> or something?

Why not, but if we do, we should also remove this "M" from other
places. It was already there in one error message given by 'git
rebase' in a non-clean tree (and you just accepted a patch giving the
same output for another one).

-- 
Matthieu

Re: [PATCH 2/2 (v2)] reset: make the output more user-friendly.

[Matthieu Moy]

[ back from holiday ]

Any opinion/update on this? I don't think I got any reply ...

Thanks,

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>
>>>  cat > expect << EOF
>>> -file2: locally modified
>>> +Unstaged changes after reset:
>>> +M	file2
>>
>> It simply feels backwards when plumbing output says something in human
>> language (e.g. "needs update") while Porcelain output spits out a cryptic
>> M or U.  If the goal is human-readability and user-friendliness,
>
> The goal here is just consistency.
>
> And I do consider 'git diff --name-status' as porcelain.
>
>> shouldn't we rather say:
>>
>> 	Path with local modifications:
>>         	file2
>>
>> or something?
>
> Why not, but if we do, we should also remove this "M" from other
> places. It was already there in one error message given by 'git
> rebase' in a non-clean tree (and you just accepted a patch giving the
> same output for another one).

-- 
Matthieu

Re: [PATCH 2/2 (v2)] reset: make the output more user-friendly.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>>
>>>>  cat > expect << EOF
>>>> -file2: locally modified
>>>> +Unstaged changes after reset:
>>>> +M	file2
>>>
>>> It simply feels backwards when plumbing output says something in human
>>> language (e.g. "needs update") while Porcelain output spits out a cryptic
>>> M or U.  If the goal is human-readability and user-friendliness,
>>
>> The goal here is just consistency.
>>
>> And I do consider 'git diff --name-status' as porcelain.

Ok.

[PATCH 1/2] Rename REFRESH_SAY_CHANGED to REFRESH_IN_PORCELAIN.

[Matthieu Moy]

The change in the output is going to become more general than just saying
"changed", so let's make the variable name more general too.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin-add.c   |    2 +-
 builtin-reset.c |    4 ++--
 cache.h         |    2 +-
 read-cache.c    |    2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 581a2a1..a325bc9 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -105,7 +105,7 @@ static void refresh(int verbose, const char **pathspec)
 	for (specs = 0; pathspec[specs];  specs++)
 		/* nothing */;
 	seen = xcalloc(specs, 1);
-	refresh_index(&the_index, verbose ? REFRESH_SAY_CHANGED : REFRESH_QUIET,
+	refresh_index(&the_index, verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET,
 		      pathspec, seen);
 	for (i = 0; i < specs; i++) {
 		if (!seen[i])
diff --git a/builtin-reset.c b/builtin-reset.c
index 5fa1789..ddf68d5 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -261,7 +261,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			die("Cannot do %s reset with paths.",
 					reset_type_names[reset_type]);
 		return read_from_tree(prefix, argv + i, sha1,
-				quiet ? REFRESH_QUIET : REFRESH_SAY_CHANGED);
+				quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
 	}
 	if (reset_type == NONE)
 		reset_type = MIXED; /* by default */
@@ -302,7 +302,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		break;
 	case MIXED: /* Report what has not been updated. */
 		update_index_refresh(0, NULL,
-				quiet ? REFRESH_QUIET : REFRESH_SAY_CHANGED);
+				quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
 		break;
 	}
 
diff --git a/cache.h b/cache.h
index 9222774..ae0e83e 100644
--- a/cache.h
+++ b/cache.h
@@ -476,7 +476,7 @@ extern int check_path(const char *path, int len, struct stat *st);
 #define REFRESH_QUIET		0x0004	/* be quiet about it */
 #define REFRESH_IGNORE_MISSING	0x0008	/* ignore non-existent */
 #define REFRESH_IGNORE_SUBMODULES	0x0010	/* ignore submodules */
-#define REFRESH_SAY_CHANGED	0x0020	/* say "changed" not "needs update" */
+#define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 extern int refresh_index(struct index_state *, unsigned int flags, const char **pathspec, char *seen);
 
 struct lock_file {
diff --git a/read-cache.c b/read-cache.c
index 4e3e272..f1aff81 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1077,7 +1077,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
 	const char *needs_update_message;
 
-	needs_update_message = ((flags & REFRESH_SAY_CHANGED)
+	needs_update_message = ((flags & REFRESH_IN_PORCELAIN)
 				? "locally modified" : "needs update");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
-- 
1.6.4.187.gd399.dirty

[PATCH 2/2 (v3)] reset: make the output more user-friendly.

[Matthieu Moy]

git reset without argument displays a summary of the remaining
unstaged changes. The problem with these is that they look like an
error message, and the format is inconsistant with the format used in
other places like "git diff --name-status".

This patch mimics the output of "git diff --name-status", and adds a
header to make it clear the output is informative, and not an error.

It also changes the output of "git add --refresh --verbose" in the same
way.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

In previous version, I hadn't noticed that the same message was
displayed for 'git reset' and for 'git add --refresh --verbose'. This
new version takes both cases into account, and avoids saying "after
reset" when the user called "add".

 builtin-add.c    |    2 +-
 builtin-reset.c  |    3 ++-
 cache.h          |    4 ++--
 read-cache.c     |   26 ++++++++++++++++++++------
 t/t7102-reset.sh |    3 ++-
 5 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index a325bc9..006fd08 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -106,7 +106,7 @@ static void refresh(int verbose, const char **pathspec)
 		/* nothing */;
 	seen = xcalloc(specs, 1);
 	refresh_index(&the_index, verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET,
-		      pathspec, seen);
+		      pathspec, seen, "Unstaged changes after refreshing the index:");
 	for (i = 0; i < specs; i++) {
 		if (!seen[i])
 			die("pathspec '%s' did not match any files", pathspec[i]);
diff --git a/builtin-reset.c b/builtin-reset.c
index ddf68d5..0fc0b07 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -108,7 +108,8 @@ static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)
 	if (read_cache() < 0)
 		return error("Could not read index");
 
-	result = refresh_cache(flags) ? 1 : 0;
+	result = refresh_index(&the_index, (flags), NULL, NULL,
+			       "Unstaged changes after reset:") ? 1 : 0;
 	if (write_cache(fd, active_cache, active_nr) ||
 			commit_locked_index(index_lock))
 		return error ("Could not refresh index");
diff --git a/cache.h b/cache.h
index ae0e83e..fda9816 100644
--- a/cache.h
+++ b/cache.h
@@ -330,7 +330,7 @@ static inline void remove_name_hash(struct cache_entry *ce)
 #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))
 #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))
 #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))
-#define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL)
+#define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
 #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
 #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
 #define cache_name_exists(name, namelen, igncase) index_name_exists(&the_index, (name), (namelen), (igncase))
@@ -477,7 +477,7 @@ extern int check_path(const char *path, int len, struct stat *st);
 #define REFRESH_IGNORE_MISSING	0x0008	/* ignore non-existent */
 #define REFRESH_IGNORE_SUBMODULES	0x0010	/* ignore submodules */
 #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
-extern int refresh_index(struct index_state *, unsigned int flags, const char **pathspec, char *seen);
+extern int refresh_index(struct index_state *, unsigned int flags, const char **pathspec, char *seen, char *header_msg);
 
 struct lock_file {
 	struct lock_file *next;
diff --git a/read-cache.c b/read-cache.c
index f1aff81..1bbaf1c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1065,7 +1065,18 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	return updated;
 }
 
-int refresh_index(struct index_state *istate, unsigned int flags, const char **pathspec, char *seen)
+static void show_file(const char * fmt, const char * name, int in_porcelain,
+		      int * first, char *header_msg)
+{
+	if (in_porcelain && *first && header_msg) {
+		printf("%s\n", header_msg);
+		*first=0;
+	}
+	printf(fmt, name);
+}
+
+int refresh_index(struct index_state *istate, unsigned int flags, const char **pathspec,
+		  char *seen, char *header_msg)
 {
 	int i;
 	int has_errors = 0;
@@ -1074,11 +1085,14 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 	int quiet = (flags & REFRESH_QUIET) != 0;
 	int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
 	int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
+	int first = 1;
+	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
 	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
-	const char *needs_update_message;
+	const char *needs_update_fmt;
+	const char *needs_merge_fmt;
 
-	needs_update_message = ((flags & REFRESH_IN_PORCELAIN)
-				? "locally modified" : "needs update");
+	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");
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce, *new;
 		int cache_errno = 0;
@@ -1094,7 +1108,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			i--;
 			if (allow_unmerged)
 				continue;
-			printf("%s: needs merge\n", ce->name);
+			show_file(needs_merge_fmt, ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
@@ -1117,7 +1131,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			printf("%s: %s\n", ce->name, needs_update_message);
+			show_file(needs_update_fmt, ce->name, in_porcelain, &first, header_msg);
 			has_errors = 1;
 			continue;
 		}
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index e637c7d..e85ff02 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -419,7 +419,8 @@ test_expect_success 'resetting an unmodified path is a no-op' '
 '
 
 cat > expect << EOF
-file2: locally modified
+Unstaged changes after reset:
+M	file2
 EOF
 
 test_expect_success '--mixed refreshes the index' '
-- 
1.6.4.187.gd399.dirty

Re: [PATCH 2/2 (v3)] reset: make the output more user-friendly.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> git reset without argument displays a summary of the remaining
> unstaged changes. The problem with these is that they look like an
> error message, and the format is inconsistant with the format used in
> other places like "git diff --name-status".
>
> This patch mimics the output of "git diff --name-status", and adds a
> header to make it clear the output is informative, and not an error.
>
> It also changes the output of "git add --refresh --verbose" in the same
> way.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>

Thanks.  Will queue.

However, I'd change the justification.

    git reset without argument displays a summary of the local modification,
    like this:
    
        $ git reset
        Makefile: locally modified
    
    Some people have problems with this; they look like an error message.
    
    This patch makes its output mimic how "git checkout $another_branch"
    reports the paths with local modifications.  "git add --refresh --verbose"
    is changed in the same way.
    
    It also adds a header to make it clear that the output is informative,
    and not an error.
    
    Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>

The output from reset in question is merely an informative side effect, as
opposed to what you actively ask "git diff" to give as its primary output.
As such, your "consistency" argument is pretty weak.  There is no reason
to expect that the informative message to resemble one particular format
(namely, --name-status) and not another (e.g. --stat or --name-only), as
you are not explicitly specifying what format to use; nor we would want to
make it customizable--after all it is just a friendly reminder.

Informative output from "git checkout $branch" when there are local
changes is a much better precedent to refer to.

After applying your patch and having compared these two sets of output:

        (1) without changes

        $ git reset --hard
        $ git checkout mm/reset-report
        Already on 'mm/reset-report'
        $ git reset
        $ git add --refresh -v Makefile

        (2) with changes

        $ echo >>Makefile
        $ git add Makefile
        $ git checkout mm/reset-report
        M       Makefile
        Already on 'mm/reset-report'
        $ git reset
        Unstaged changes after reset:
        M       Makefile
        $ git add --refresh -v Makefile
        Unstaged changes after refreshing the index:
        M       Makefile

I am somewhat inclined to suggest that we should drop the new "Unstaged
changes after ..." message, though.

By the way, "Already on .../Switched to ..." noise from "git checkout" is
also very annoying.  It is useless to report that the command did exactly
what the user told it to do.  Even more annoyingly, "git checkout -q" to
squelch this useless noise also squelches the "here are the paths you have
local changes" reminder, which is much more useful.

But that is a separate topic.

Re: [PATCH 2/2 (v3)] reset: make the output more user-friendly.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Thanks.  Will queue.

Thanks,

> However, I'd change the justification.

Fine with me.

> The output from reset in question is merely an informative side effect, as
> opposed to what you actively ask "git diff" to give as its primary output.
> As such, your "consistency" argument is pretty weak.  There is no reason
> to expect that the informative message to resemble one particular format
> (namely, --name-status) and not another (e.g. --stat or --name-only),

I agree that chosing --name-status over, like, --stat is rather
arbitrary. But Git has IMHO far too many languages for talking about
changes (--stat, --name-status, 'git status' itself, the 'git ls-files
-t' that I just discovered, and this 'bla: locally modified').
Reducing the number of formats by one is a good thing to me.

> Informative output from "git checkout $branch" when there are local
> changes is a much better precedent to refer to.

Yes.

> I am somewhat inclined to suggest that we should drop the new "Unstaged
> changes after ..." message, though.

I've thought about this too. The new format already looks much less
like an error message, which was really the problem I was solving. But
one advantage of the message contains two relevant informations:
"unstaged" and "after".

Intuitively, I would have thought that "git reset" was reporting what
it was doing, as it was doing it. So to me (before experimenting a bit
more and looking at the source code),

M	foo.txt
M	bar.txt

would mean "I've just reseted foo.txt and bar.txt, which were locally
modified", while actually "git reset" can very well show this message
after reseting only foo.txt, just informing the user that bar.txt is
also modified. So, at least to me, the semantics was very unclear, and
while I would have understood immediately with the one-liner message.

In short: no strong objection to remove this message, but to me it is
usefull.

-- 
Matthieu

Re: [PATCH 2/2 (v3)] reset: make the output more user-friendly.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Intuitively, I would have thought that "git reset" was reporting what
> it was doing, as it was doing it. So to me (before experimenting a bit
> more and looking at the source code),
>
> M	foo.txt
> M	bar.txt
>
> would mean "I've just reseted foo.txt and bar.txt, which were locally
> modified", while actually "git reset" can very well show this message
> after reseting only foo.txt, just informing the user that bar.txt is
> also modified. So, at least to me, the semantics was very unclear, and
> while I would have understood immediately with the one-liner message.
>
> In short: no strong objection to remove this message, but to me it is
> usefull.

Thanks for sharing the reasoning.

Two conflicting/competing thoughts come to mind:

 1. Perhaps we should add a similar "explanation" for the list of paths
    with changes upon switching branches with "git checkout" for
    consistency.

 2. Such an "explanation" of what the output means would help the first
    time people, but would everybody stay "first time" forever?  Would the
    explanation become just another wasted line in valuable screen real
    estate after people gain experience?

I am leaning towards #1 right now.

Re: [PATCH 2/2 (v3)] reset: make the output more user-friendly.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Two conflicting/competing thoughts come to mind:
>
>  1. Perhaps we should add a similar "explanation" for the list of paths
>     with changes upon switching branches with "git checkout" for
>     consistency.

Actually, I had never paid much attention to this message for
checkout. Just checked, and I got it wrong too ;-). I thought checkout
was showing me the files it was modifying, that wasn't it.

That said, I'm not a heavy user of local branches, so I'm a bad judge
on what should be the behavior.

>  2. Such an "explanation" of what the output means would help the first
>     time people, but would everybody stay "first time" forever?  Would the
>     explanation become just another wasted line in valuable screen real
>     estate after people gain experience?

Yes, and this is a much more general issue than just checkout/reset.
For example, the output of 'git status' is very nice to newbies:

  # On branch master
  # Changed but not updated:
  #   (use "git add <file>..." to update what will be committed)
  #   (use "git checkout -- <file>..." to discard changes in working directory)
  #
  #       modified:   git.c
  #
  no changes added to commit (use "git add" and/or "git commit -a")

But out of these 8 lines, only two contain real informations, and the
(use "git bla") are just noise to expert users.

I've been thinking of a configuration option, like "core.expertuser"
or "ui.expertuser" that would let users disable these informative
messages on demand. I'm not sure how good the idea is.

--
Matthieu

Re: [PATCH 2/2 (v3)] reset: make the output more user-friendly.

[Reece Dunn]

2009/8/23 Matthieu Moy <Matthieu.Moy@imag.fr>:
> For example, the output of 'git status' is very nice to newbies:
>
>  # On branch master
>  # Changed but not updated:
>  #   (use "git add <file>..." to update what will be committed)

Shouldn't this be something like: (use "git add <file>..." to add new
and modified files to be committed) -- I am saying this as "update"
can also refer to removing files, or discarding changes.

>  #   (use "git checkout -- <file>..." to discard changes in working directory)
>  #
>  #       modified:   git.c
>  #
>  no changes added to commit (use "git add" and/or "git commit -a")
>
> But out of these 8 lines, only two contain real informations, and the
> (use "git bla") are just noise to expert users.

Yes and no. Using git for quite a while now, the day-to-day operations
are second nature, but other slightly obscure commands (how exactly do
I remove a staged file?) are useful to have.

> I've been thinking of a configuration option, like "core.expertuser"
> or "ui.expertuser" that would let users disable these informative
> messages on demand. I'm not sure how good the idea is.

The "core.expertuser" option does not really say what this is doing
(should the expertuser option list the sha1's for the commits, trees
and objects it is adding?).

I would call it something like "core.interactive-help",
"core.inplace-help" or "core.inline-help", as that is what the (use
...) lines are.

- Reece