Re: Message from git reset: confusing?

[Matthieu Moy <Matthieu.Moy@imag.fr>]

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