Sverre Rabbelier <srabbelier@xxxxxxxxx> writes:
> Heya,
>
> On Wed, Aug 5, 2009 at 10:42, Avery Pennarun<apenwarr@xxxxxxxxx> 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
--
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