Re: Message from git reset: confusing?

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: Message from git reset: confusing?

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:11

Matthieu Moy [off-list ref] 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 2/2 (v2)] reset: make the output more user-friendly.

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:47:12

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 <redacted>
---

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

[PATCH 1/2] Rename REFRESH_SAY_CHANGED to REFRESH_IN_PORCELAIN.

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:47:12

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 <redacted>
---
 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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help