Re: Message from git reset: confusing?
From: Matthieu Moy <hidden>
Date: 2016-06-15 22:47:11
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: Message from git reset: confusing? · Junio C Hamano <hidden>
- 2016-06-15 · Message from git reset: confusing? · Matthieu Moy <hidden>
- 2016-06-15 · Re: Message from git reset: confusing? · Junio C Hamano <hidden>
- 2016-06-15 · Re: Message from git reset: confusing? · Avery Pennarun <hidden>
- 2016-06-15 · Re: Message from git reset: confusing? · John Tapsell <hidden>
Sverre Rabbelier [off-list ref] writes:
Heya, On Wed, Aug 5, 2009 at 10:42, Avery Pennarun[off-list ref] wrote:quoted
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.
quoted
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