[PATCH 0/5] Revamping "git status"

[PATCH 0/5] Revamping "git status"

[Junio C Hamano]

The first two patches in this series are a small bugfix and an internal
clean-up for the underlying "diff-index" engine used by "git status" when
it inspects how the index differs from the HEAD commit.

The third one separates the logic to inspect the changes in the index and
the work tree from the code to show the result in "git status" command, so
that collected data can be presented in different formats more easily.
It is roughly based on an old patch of mine from October last year, but it
collects a bit more detail on conflicted paths.

The fourth one changes the output from the "git status" command when
unmerged paths are present.  We now have a separate "Unmerged paths"
section at the beginning and describe the nature of the conflict in more
detail.

The last one is a bit iffy, but shows how the new infrastructure can be
used to present the "git status" output in a more succinct and machine
readble format.

Junio C Hamano (5):
  diff-index: report unmerged new entries
  diff-index: keep the original index intact
  wt-status.c: rework the way changes to the index and work tree are
    summarized
  status: show worktree status of conflicted paths separately
  shortstatus: a new command

 Makefile            |    1 +
 builtin-commit.c    |  107 +++++++++++++++++-
 builtin.h           |    1 +
 diff-lib.c          |   22 +----
 git.c               |    1 +
 t/t7060-wtstatus.sh |   31 +++++
 wt-status.c         |  314 +++++++++++++++++++++++++++++++++++++++++++--------
 wt-status.h         |   11 ++
 8 files changed, 418 insertions(+), 70 deletions(-)
 create mode 100755 t/t7060-wtstatus.sh

[PATCH 1/5] diff-index: report unmerged new entries

[Junio C Hamano]

Since an earlier change to diff-index by d1f2d7e (Make run_diff_index()
use unpack_trees(), not read_tree(), 2008-01-19), we stopped reporting an
unmerged path that does not exist in the tree, but we should.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-lib.c          |    4 ++--
 t/t7060-wtstatus.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)
 create mode 100755 t/t7060-wtstatus.sh

diff --git a/diff-lib.c b/diff-lib.c
index ad2a4cd..ad5b6ca 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -348,8 +348,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	match_missing = !revs->ignore_merges;
 
 	if (cached && idx && ce_stage(idx)) {
-		if (tree)
-			diff_unmerge(&revs->diffopt, idx->name, idx->ce_mode, idx->sha1);
+		diff_unmerge(&revs->diffopt, idx->name, idx->ce_mode,
+			     idx->sha1);
 		return;
 	}
 
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
new file mode 100755
index 0000000..5ad2cd1
--- /dev/null
+++ b/t/t7060-wtstatus.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='basic work tree status reporting'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit A &&
+	test_commit B oneside added &&
+	git checkout A^0 &&
+	test_commit C oneside created
+'
+
+test_expect_success 'A/A conflict' '
+	git checkout B^0 &&
+	test_must_fail git merge C
+'
+
+test_expect_success 'Report path with conflict' '
+	git diff --cached --name-status >actual &&
+	echo "U	oneside" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'Report new path with conflict' '
+	git diff --cached --name-status HEAD^ >actual &&
+	echo "U	oneside" >expect &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.6.4.18.g07a4a

[PATCH 2/5] diff-index: keep the original index intact

[Junio C Hamano]

When comparing the index and a tree, we used to read the contents of the
tree into stage #1 of the index and compared them with stage #0.  In order
not to lose sight of entries originally unmerged in the index, we hoisted
them to stage #3 before reading the tree.

Commit d1f2d7e (Make run_diff_index() use unpack_trees(), not read_tree(),
2008-01-19) changed all this.  These days, we instead use unpack_trees()
API to traverse the tree and compare the contents with the index, without
modifying the index at all.  There is no reason to hoist the unmerged
entries to stage #3 anymore.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-lib.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index ad5b6ca..2a82dac 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -309,22 +309,6 @@ static int show_modified(struct rev_info *revs,
 }
 
 /*
- * This turns all merge entries into "stage 3". That guarantees that
- * when we read in the new tree (into "stage 1"), we won't lose sight
- * of the fact that we had unmerged entries.
- */
-static void mark_merge_entries(void)
-{
-	int i;
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		if (!ce_stage(ce))
-			continue;
-		ce->ce_flags |= CE_STAGEMASK;
-	}
-}
-
-/*
  * This gets a mix of an existing index and a tree, one pathname entry
  * at a time. The index entry may be a single stage-0 one, but it could
  * also be multiple unmerged entries (in which case idx_pos/idx_nr will
@@ -435,8 +419,6 @@ int run_diff_index(struct rev_info *revs, int cached)
 	struct unpack_trees_options opts;
 	struct tree_desc t;
 
-	mark_merge_entries();
-
 	ent = revs->pending.objects[0].item;
 	tree_name = revs->pending.objects[0].name;
 	tree = parse_tree_indirect(ent->sha1);
-- 
1.6.4.18.g07a4a

[PATCH 3/5] wt-status.c: rework the way changes to the index and work tree are summarized

[Junio C Hamano]

Introduce a new infrastructure to find and summarize changes in a single
string list, and rewrite wt_status_print_{updated,changed} functions using
it.

The goal of this change is to give more information on conflicted paths in
the status output.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-commit.c |   13 ++-
 wt-status.c      |  252 ++++++++++++++++++++++++++++++++++++++++++++----------
 wt-status.h      |   11 +++
 3 files changed, 226 insertions(+), 50 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 4bcce06..6d12c2e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -217,12 +217,15 @@ static void create_base_index(void)
 		exit(128); /* We've already reported the error, finish dying */
 }
 
-static char *prepare_index(int argc, const char **argv, const char *prefix)
+static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
 {
 	int fd;
 	struct string_list partial;
 	const char **pathspec = NULL;
+	int refresh_flags = REFRESH_QUIET;
 
+	if (is_status)
+		refresh_flags |= REFRESH_UNMERGED;
 	if (interactive) {
 		if (interactive_add(argc, argv, prefix) != 0)
 			die("interactive add failed");
@@ -253,7 +256,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
-		refresh_cache(REFRESH_QUIET);
+		refresh_cache(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die("unable to write new_index file");
@@ -272,7 +275,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	 */
 	if (!pathspec || !*pathspec) {
 		fd = hold_locked_index(&index_lock, 1);
-		refresh_cache(REFRESH_QUIET);
+		refresh_cache(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    commit_locked_index(&index_lock))
 			die("unable to write new_index file");
@@ -825,7 +828,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	argc = parse_and_validate_options(argc, argv, builtin_status_usage, prefix);
 
-	index_file = prepare_index(argc, argv, prefix);
+	index_file = prepare_index(argc, argv, prefix, 1);
 
 	commitable = run_status(stdout, index_file, prefix, 0);
 
@@ -907,7 +910,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage, prefix);
 
-	index_file = prepare_index(argc, argv, prefix);
+	index_file = prepare_index(argc, argv, prefix, 0);
 
 	/* Set up everything for writing the commit object.  This includes
 	   running hooks, writing the trees, and interacting with the user.  */
diff --git a/wt-status.c b/wt-status.c
index 47735d8..1614352 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -20,6 +20,7 @@ static char wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RED,    /* WT_STATUS_CHANGED */
 	GIT_COLOR_RED,    /* WT_STATUS_UNTRACKED */
 	GIT_COLOR_RED,    /* WT_STATUS_NOBRANCH */
+	GIT_COLOR_YELLOW, /* WT_STATUS_UNMERGED */
 };
 
 enum untracked_status_type show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
@@ -56,6 +57,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->reference = "HEAD";
 	s->fp = stdout;
 	s->index_file = get_index_file();
+	s->change.strdup_strings = 1;
 }
 
 static void wt_status_print_cached_header(struct wt_status *s)
@@ -98,18 +100,35 @@ static void wt_status_print_trailer(struct wt_status *s)
 
 #define quote_path quote_path_relative
 
-static void wt_status_print_filepair(struct wt_status *s,
-				     int t, struct diff_filepair *p)
+static void wt_status_print_change_data(struct wt_status *s,
+					int change_type,
+					struct string_list_item *it)
 {
-	const char *c = color(t);
+	struct wt_status_change_data *d = it->util;
+	const char *c = color(change_type);
+	int status = status;
+	char *one_name;
+	char *two_name;
 	const char *one, *two;
 	struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
 
-	one = quote_path(p->one->path, -1, &onebuf, s->prefix);
-	two = quote_path(p->two->path, -1, &twobuf, s->prefix);
+	one_name = two_name = it->string;
+	switch (change_type) {
+	case WT_STATUS_UPDATED:
+		status = d->index_status;
+		if (d->head_path)
+			one_name = d->head_path;
+		break;
+	case WT_STATUS_CHANGED:
+		status = d->worktree_status;
+		break;
+	}
+
+	one = quote_path(one_name, -1, &onebuf, s->prefix);
+	two = quote_path(two_name, -1, &twobuf, s->prefix);
 
 	color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
-	switch (p->status) {
+	switch (status) {
 	case DIFF_STATUS_ADDED:
 		color_fprintf(s->fp, c, "new file:   %s", one);
 		break;
@@ -135,64 +154,114 @@ static void wt_status_print_filepair(struct wt_status *s,
 		color_fprintf(s->fp, c, "unmerged:   %s", one);
 		break;
 	default:
-		die("bug: unhandled diff status %c", p->status);
+		die("bug: unhandled diff status %c", status);
 	}
 	fprintf(s->fp, "\n");
 	strbuf_release(&onebuf);
 	strbuf_release(&twobuf);
 }
 
-static void wt_status_print_updated_cb(struct diff_queue_struct *q,
-		struct diff_options *options,
-		void *data)
+static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
+					 struct diff_options *options,
+					 void *data)
 {
 	struct wt_status *s = data;
-	int shown_header = 0;
 	int i;
+
+	if (!q->nr)
+		return;
+	s->workdir_dirty = 1;
 	for (i = 0; i < q->nr; i++) {
-		if (q->queue[i]->status == 'U')
-			continue;
-		if (!shown_header) {
-			wt_status_print_cached_header(s);
-			s->commitable = 1;
-			shown_header = 1;
+		struct diff_filepair *p;
+		struct string_list_item *it;
+		struct wt_status_change_data *d;
+
+		p = q->queue[i];
+		it = string_list_insert(p->one->path, &s->change);
+		d = it->util;
+		if (!d) {
+			d = xcalloc(1, sizeof(*d));
+			it->util = d;
 		}
-		wt_status_print_filepair(s, WT_STATUS_UPDATED, q->queue[i]);
+		if (!d->worktree_status)
+			d->worktree_status = p->status;
 	}
-	if (shown_header)
-		wt_status_print_trailer(s);
 }
 
-static void wt_status_print_changed_cb(struct diff_queue_struct *q,
-                        struct diff_options *options,
-                        void *data)
+static int unmerged_mask(const char *path)
+{
+	int pos, mask;
+	struct cache_entry *ce;
+
+	pos = cache_name_pos(path, strlen(path));
+	if (0 <= pos)
+		return 0;
+
+	mask = 0;
+	pos = -pos-1;
+	while (pos <= active_nr) {
+		ce = active_cache[pos++];
+		if (strcmp(ce->name, path))
+			break;
+		mask |= (1 << ce_stage(ce));
+	}
+	return mask;
+}
+
+static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
+					 struct diff_options *options,
+					 void *data)
 {
 	struct wt_status *s = data;
 	int i;
-	if (q->nr) {
-		int has_deleted = 0;
-		s->workdir_dirty = 1;
-		for (i = 0; i < q->nr; i++)
-			if (q->queue[i]->status == DIFF_STATUS_DELETED) {
-				has_deleted = 1;
-				break;
-			}
-		wt_status_print_dirty_header(s, has_deleted);
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p;
+		struct string_list_item *it;
+		struct wt_status_change_data *d;
+
+		p = q->queue[i];
+		it = string_list_insert(p->two->path, &s->change);
+		d = it->util;
+		if (!d) {
+			d = xcalloc(1, sizeof(*d));
+			it->util = d;
+		}
+		if (!d->index_status)
+			d->index_status = p->status;
+		switch (p->status) {
+		case DIFF_STATUS_COPIED:
+		case DIFF_STATUS_RENAMED:
+			d->head_path = xstrdup(p->one->path);
+			break;
+		case DIFF_STATUS_UNMERGED:
+			d->stagemask = unmerged_mask(p->two->path);
+			break;
+		}
 	}
-	for (i = 0; i < q->nr; i++)
-		wt_status_print_filepair(s, WT_STATUS_CHANGED, q->queue[i]);
-	if (q->nr)
-		wt_status_print_trailer(s);
 }
 
-static void wt_status_print_updated(struct wt_status *s)
+static void wt_status_collect_changes_worktree(struct wt_status *s)
+{
+	struct rev_info rev;
+
+	init_revisions(&rev, NULL);
+	setup_revisions(0, NULL, &rev, NULL);
+	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = wt_status_collect_changed_cb;
+	rev.diffopt.format_callback_data = s;
+	run_diff_files(&rev, 0);
+}
+
+static void wt_status_collect_changes_index(struct wt_status *s)
 {
 	struct rev_info rev;
+
 	init_revisions(&rev, NULL);
 	setup_revisions(0, NULL, &rev,
 		s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
-	rev.diffopt.format_callback = wt_status_print_updated_cb;
+	rev.diffopt.format_callback = wt_status_collect_updated_cb;
 	rev.diffopt.format_callback_data = s;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 200;
@@ -200,15 +269,106 @@ static void wt_status_print_updated(struct wt_status *s)
 	run_diff_index(&rev, 1);
 }
 
+static void wt_status_collect_changes_initial(struct wt_status *s)
+{
+	int i;
+
+	for (i = 0; i < active_nr; i++) {
+		struct string_list_item *it;
+		struct wt_status_change_data *d;
+		struct cache_entry *ce = active_cache[i];
+
+		it = string_list_insert(ce->name, &s->change);
+		d = it->util;
+		if (!d) {
+			d = xcalloc(1, sizeof(*d));
+			it->util = d;
+		}
+		if (ce_stage(ce)) {
+			d->index_status = DIFF_STATUS_UNMERGED;
+			d->stagemask |= (1 << (ce_stage(ce) - 1));
+		}
+		else
+			d->index_status = DIFF_STATUS_ADDED;
+	}
+}
+
+void wt_status_collect_changes(struct wt_status *s)
+{
+	wt_status_collect_changes_worktree(s);
+
+	if (s->is_initial)
+		wt_status_collect_changes_initial(s);
+	else
+		wt_status_collect_changes_index(s);
+}
+
+static void wt_status_print_updated(struct wt_status *s)
+{
+	int shown_header = 0;
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d;
+		struct string_list_item *it;
+		it = &(s->change.items[i]);
+		d = it->util;
+		if (!d->index_status ||
+		    d->index_status == DIFF_STATUS_UNMERGED)
+			continue;
+		if (!shown_header) {
+			wt_status_print_cached_header(s);
+			s->commitable = 1;
+			shown_header = 1;
+		}
+		wt_status_print_change_data(s, WT_STATUS_UPDATED, it);
+	}
+	if (shown_header)
+		wt_status_print_trailer(s);
+}
+
+/*
+ * -1 : has delete
+ *  0 : no change
+ *  1 : some change but no delete
+ */
+static int wt_status_check_worktree_changes(struct wt_status *s)
+{
+	int i;
+	int changes = 0;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d;
+		d = s->change.items[i].util;
+		if (!d->worktree_status)
+			continue;
+		changes = 1;
+		if (d->worktree_status == DIFF_STATUS_DELETED)
+			return -1;
+	}
+	return changes;
+}
+
 static void wt_status_print_changed(struct wt_status *s)
 {
-	struct rev_info rev;
-	init_revisions(&rev, "");
-	setup_revisions(0, NULL, &rev, NULL);
-	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
-	rev.diffopt.format_callback = wt_status_print_changed_cb;
-	rev.diffopt.format_callback_data = s;
-	run_diff_files(&rev, 0);
+	int i;
+	int worktree_changes = wt_status_check_worktree_changes(s);
+
+	if (!worktree_changes)
+		return;
+
+	wt_status_print_dirty_header(s, worktree_changes < 0);
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d;
+		struct string_list_item *it;
+		it = &(s->change.items[i]);
+		d = it->util;
+		if (!d->worktree_status)
+			continue;
+		wt_status_print_change_data(s, WT_STATUS_CHANGED, it);
+	}
+	wt_status_print_trailer(s);
 }
 
 static void wt_status_print_submodule_summary(struct wt_status *s)
@@ -337,6 +497,8 @@ void wt_status_print(struct wt_status *s)
 			wt_status_print_tracking(s);
 	}
 
+	wt_status_collect_changes(s);
+
 	if (s->is_initial) {
 		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
 		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "# Initial commit");
diff --git a/wt-status.h b/wt-status.h
index 78add09..f80142f 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -2,6 +2,7 @@
 #define STATUS_H
 
 #include <stdio.h>
+#include "string-list.h"
 
 enum color_wt_status {
 	WT_STATUS_HEADER,
@@ -9,6 +10,7 @@ enum color_wt_status {
 	WT_STATUS_CHANGED,
 	WT_STATUS_UNTRACKED,
 	WT_STATUS_NOBRANCH,
+	WT_STATUS_UNMERGED,
 };
 
 enum untracked_status_type {
@@ -18,6 +20,13 @@ enum untracked_status_type {
 };
 extern enum untracked_status_type show_untracked_files;
 
+struct wt_status_change_data {
+	int worktree_status;
+	int index_status;
+	int stagemask;
+	char *head_path;
+};
+
 struct wt_status {
 	int is_initial;
 	char *branch;
@@ -33,6 +42,7 @@ struct wt_status {
 	const char *index_file;
 	FILE *fp;
 	const char *prefix;
+	struct string_list change;
 };
 
 int git_status_config(const char *var, const char *value, void *cb);
@@ -40,5 +50,6 @@ extern int wt_status_use_color;
 extern int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
+void wt_status_collect_changes(struct wt_status *s);
 
 #endif /* STATUS_H */
-- 
1.6.4.18.g07a4a

[PATCH 4/5] status: show worktree status of conflicted paths separately

[Junio C Hamano]

When a path is unmerged in the index, we used to always say "unmerged" in
the "Changed but not updated" section, even when the path was deleted in
the work tree.

Remove unmerged entries from the "Updated" section, and create a new
section "Unmerged paths".  Describe how the different stages conflict
in more detail in this new section.

Note that with the current 3-way merge policy (with or without recursive),
certain combinations of index stages should never happen.  For example,
having only stage #2 means that a path that did not exist in the common
ancestor was added by us while the other branch did not do anything to it,
which would have autoresolved to take our addition.  The code nevertheless
prepares for the possibility that future merge policies may leave a path
in such a state.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 1614352..f73bf47 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -20,7 +20,7 @@ static char wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RED,    /* WT_STATUS_CHANGED */
 	GIT_COLOR_RED,    /* WT_STATUS_UNTRACKED */
 	GIT_COLOR_RED,    /* WT_STATUS_NOBRANCH */
-	GIT_COLOR_YELLOW, /* WT_STATUS_UNMERGED */
+	GIT_COLOR_RED,    /* WT_STATUS_UNMERGED */
 };
 
 enum untracked_status_type show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
@@ -38,6 +38,8 @@ static int parse_status_slot(const char *var, int offset)
 		return WT_STATUS_UNTRACKED;
 	if (!strcasecmp(var+offset, "nobranch"))
 		return WT_STATUS_NOBRANCH;
+	if (!strcasecmp(var+offset, "unmerged"))
+		return WT_STATUS_UNMERGED;
 	die("bad config variable '%s'", var);
 }
 
@@ -60,6 +62,18 @@ void wt_status_prepare(struct wt_status *s)
 	s->change.strdup_strings = 1;
 }
 
+static void wt_status_print_unmerged_header(struct wt_status *s)
+{
+	const char *c = color(WT_STATUS_HEADER);
+	color_fprintf_ln(s->fp, c, "# Unmerged paths:");
+	if (!s->is_initial)
+		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
+	else
+		color_fprintf_ln(s->fp, c, "#   (use \"git rm --cached <file>...\" to unstage)");
+	color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to mark resolution)");
+	color_fprintf_ln(s->fp, c, "#");
+}
+
 static void wt_status_print_cached_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER);
@@ -100,6 +114,29 @@ static void wt_status_print_trailer(struct wt_status *s)
 
 #define quote_path quote_path_relative
 
+static void wt_status_print_unmerged_data(struct wt_status *s,
+					  struct string_list_item *it)
+{
+	const char *c = color(WT_STATUS_UNMERGED);
+	struct wt_status_change_data *d = it->util;
+	struct strbuf onebuf = STRBUF_INIT;
+	const char *one, *how = "bug";
+
+	one = quote_path(it->string, -1, &onebuf, s->prefix);
+	color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
+	switch (d->stagemask >> 1) {
+	case 1: how = "both deleted"; break;
+	case 2: how = "added by us"; break;
+	case 3: how = "deleted by them"; break;
+	case 4: how = "added by them"; break;
+	case 5: how = "deleted by us"; break;
+	case 6: how = "both added"; break;
+	case 7: how = "both modified"; break;
+	}
+	color_fprintf(s->fp, c, "%-20s: %s\n", how, one);
+	strbuf_release(&onebuf);
+}
+
 static void wt_status_print_change_data(struct wt_status *s,
 					int change_type,
 					struct string_list_item *it)
@@ -303,6 +340,29 @@ void wt_status_collect_changes(struct wt_status *s)
 		wt_status_collect_changes_index(s);
 }
 
+static void wt_status_print_unmerged(struct wt_status *s)
+{
+	int shown_header = 0;
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d;
+		struct string_list_item *it;
+		it = &(s->change.items[i]);
+		d = it->util;
+		if (!d->stagemask)
+			continue;
+		if (!shown_header) {
+			wt_status_print_unmerged_header(s);
+			shown_header = 1;
+		}
+		wt_status_print_unmerged_data(s, it);
+	}
+	if (shown_header)
+		wt_status_print_trailer(s);
+
+}
+
 static void wt_status_print_updated(struct wt_status *s)
 {
 	int shown_header = 0;
@@ -364,7 +424,8 @@ static void wt_status_print_changed(struct wt_status *s)
 		struct string_list_item *it;
 		it = &(s->change.items[i]);
 		d = it->util;
-		if (!d->worktree_status)
+		if (!d->worktree_status ||
+		    d->worktree_status == DIFF_STATUS_UNMERGED)
 			continue;
 		wt_status_print_change_data(s, WT_STATUS_CHANGED, it);
 	}
@@ -505,6 +566,7 @@ void wt_status_print(struct wt_status *s)
 		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
 	}
 
+	wt_status_print_unmerged(s);
 	wt_status_print_updated(s);
 	wt_status_print_changed(s);
 	if (wt_status_submodule_summary)
-- 
1.6.4.18.g07a4a

[PATCH 5/5] shortstatus: a new command

[Junio C Hamano]

Add a new command that gives the status in

    XY PATH1 -> PATH2

format to be more machine readable than output from "git status", which is
about previewing of "git commit" with the same arguments.

PATH1 is the path in the HEAD, and " -> PATH2" part is shown only when
PATH1 corresponds to a different path in the index/worktree.

For unmerged entries, X shows the status of stage #2 (i.e. ours) and Y
shows the status of stage #3 (i.e. theirs).  For entries that do not have
conflicts, X shows the status of the index, and Y shows the status of the
work tree.

    X          Y     Meaning
    -------------------------------------------------
              [MD]   not updated
    M        [ MD]   updated in index
    A        [ MD]   added to index
    D        [ MD]   deleted from index
    R        [ MD]   renamed in index
    C        [ MD]   copied in index
    [MARC]           index and work tree matches
    [ MARC]     M    work tree changed since index
    [ MARC]     D    deleted in work tree
    D           D    unmerged, both deleted
    A           U    unmerged, added by us
    U           D    unmerged, deleted by them
    U           A    unmerged, added by them
    D           U    unmerged, deleted by us
    A           A    unmerged, both added
    U           U    unmerged, both modified

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile         |    1 +
 builtin-commit.c |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin.h        |    1 +
 git.c            |    1 +
 4 files changed, 97 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index daf4296..bcefbd3 100644
--- a/Makefile
+++ b/Makefile
@@ -378,6 +378,7 @@ BUILT_INS += git-init$X
 BUILT_INS += git-merge-subtree$X
 BUILT_INS += git-peek-remote$X
 BUILT_INS += git-repo-config$X
+BUILT_INS += git-shortstatus$X
 BUILT_INS += git-show$X
 BUILT_INS += git-stage$X
 BUILT_INS += git-status$X
diff --git a/builtin-commit.c b/builtin-commit.c
index 6d12c2e..7a4ddab 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -24,6 +24,7 @@
 #include "string-list.h"
 #include "rerere.h"
 #include "unpack-trees.h"
+#include "quote.h"
 
 static const char * const builtin_commit_usage[] = {
 	"git commit [options] [--] <filepattern>...",
@@ -35,6 +36,11 @@ static const char * const builtin_status_usage[] = {
 	NULL
 };
 
+static const char * const builtin_shortstatus_usage[] = {
+	"git shortstatus [options]",
+	NULL
+};
+
 static unsigned char head_sha1[20], merge_head_sha1[20];
 static char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
@@ -813,6 +819,94 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	return argc;
 }
 
+#define quote_path quote_path_relative
+
+static void show_unmerged(int null_termination, struct string_list_item *it,
+			  struct wt_status *s)
+{
+	struct wt_status_change_data *d = it->util;
+	const char *how = "??";
+
+	switch (d->stagemask >> 1) {
+	case 1: how = "DD"; break; /* both deleted */
+	case 2: how = "AU"; break; /* added by us */
+	case 3: how = "UD"; break; /* deleted by them */
+	case 4: how = "UA"; break; /* added by them */
+	case 5: how = "DU"; break; /* deleted by us */
+	case 6: how = "AA"; break; /* both added */
+	case 7: how = "UU"; break; /* both modified */
+	}
+	printf("%s ", how);
+	if (null_termination) {
+		fprintf(stdout, "%s%c", it->string, 0);
+	} else {
+		struct strbuf onebuf = STRBUF_INIT;
+		const char *one;
+		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		printf("%s\n", one);
+		strbuf_release(&onebuf);
+	}
+}
+
+static void show_status(int null_termination, struct string_list_item *it,
+		struct wt_status *s)
+{
+	struct wt_status_change_data *d = it->util;
+
+	printf("%c%c ",
+	       !d->index_status ? ' ' : d->index_status,
+	       !d->worktree_status ? ' ' : d->worktree_status);
+	if (null_termination) {
+		fprintf(stdout, "%s%c", it->string, 0);
+		if (d->head_path)
+			fprintf(stdout, "%s%c", d->head_path, 0);
+	} else {
+		struct strbuf onebuf = STRBUF_INIT;
+		const char *one;
+		if (d->head_path) {
+			one = quote_path(d->head_path, -1, &onebuf, s->prefix);
+			printf("%s -> ", one);
+			strbuf_release(&onebuf);
+		}
+		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		printf("%s\n", one);
+		strbuf_release(&onebuf);
+	}
+}
+
+int cmd_shortstatus(int argc, const char **argv, const char *prefix)
+{
+	struct wt_status s;
+	static int null_termination;
+	int i;
+	static struct option builtin_shortstatus_options[] = {
+		OPT_BOOLEAN('z', "null", &null_termination,
+			    "terminate entries with NUL"),
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, prefix,
+			     builtin_shortstatus_options,
+			     builtin_shortstatus_usage, 0);
+
+	read_cache();
+	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
+	wt_status_prepare(&s);
+	wt_status_collect_changes(&s);
+	for (i = 0; i < s.change.nr; i++) {
+		struct wt_status_change_data *d;
+		struct string_list_item *it;
+
+		it = &(s.change.items[i]);
+		d = it->util;
+		if (d->stagemask)
+			show_unmerged(null_termination, it, &s);
+		else
+			show_status(null_termination, it, &s);
+	}
+	return 0;
+}
+
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
 	const char *index_file;
diff --git a/builtin.h b/builtin.h
index 20427d2..825a96f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -96,6 +96,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_shortstatus(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_tag(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 807d875..3977d60 100644
--- a/git.c
+++ b/git.c
@@ -348,6 +348,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "rm", cmd_rm, RUN_SETUP },
 		{ "send-pack", cmd_send_pack, RUN_SETUP },
 		{ "shortlog", cmd_shortlog, USE_PAGER },
+		{ "shortstatus", cmd_shortstatus, RUN_SETUP | NEED_WORK_TREE },
 		{ "show-branch", cmd_show_branch, RUN_SETUP },
 		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
 		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
-- 
1.6.4.18.g07a4a

Re: [PATCH 5/5] shortstatus: a new command

[Jeff King]

On Wed, Aug 05, 2009 at 02:15:46AM -0700, Junio C Hamano wrote:

> ---
>  Makefile         |    1 +
>  builtin-commit.c |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  builtin.h        |    1 +
>  git.c            |    1 +

Missing docs, of course, and tests would be nice. Is this meant to be
plumbing? If not, the name is too long to type. ;)

> +int cmd_shortstatus(int argc, const char **argv, const char *prefix)
> +{
> +	struct wt_status s;
> +	static int null_termination;
> +	int i;
> +	static struct option builtin_shortstatus_options[] = {
> +		OPT_BOOLEAN('z', "null", &null_termination,
> +			    "terminate entries with NUL"),
> +		OPT_END(),
> +	};
> +
> +	argc = parse_options(argc, argv, prefix,
> +			     builtin_shortstatus_options,
> +			     builtin_shortstatus_usage, 0);

I think one of the most often-requested things for a "git status"
replacement is that "git status <path>" do path-limiting. Something like
(the extremely not very well tested):

---
diff --git a/builtin-commit.c b/builtin-commit.c
index 7a4ddab..8bc6269 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -891,7 +891,12 @@ int cmd_shortstatus(int argc, const char **argv, const char *prefix)
 
 	read_cache();
 	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
+
 	wt_status_prepare(&s);
+	ALLOC_GROW(s.path_limit, s.path_limit_nr + argc, s.path_limit_alloc);
+	for (i = 0; i < argc; i++)
+		s.path_limit[s.path_limit_nr++] = argv[i];
+
 	wt_status_collect_changes(&s);
 	for (i = 0; i < s.change.nr; i++) {
 		struct wt_status_change_data *d;
diff --git a/wt-status.c b/wt-status.c
index 5a68297..7399e1b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -60,6 +60,12 @@ void wt_status_prepare(struct wt_status *s)
 	s->fp = stdout;
 	s->index_file = get_index_file();
 	s->change.strdup_strings = 1;
+
+	s->path_limit = xmalloc(2 * sizeof(*s->path_limit));
+	s->path_limit_nr = 1;
+	s->path_limit_alloc = 2;
+	s->path_limit[0] = "dummy argv[0]";
+	s->path_limit[1] = NULL;
 }
 
 static void wt_status_print_unmerged_header(struct wt_status *s)
@@ -283,7 +289,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	struct rev_info rev;
 
 	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, NULL);
+	setup_revisions(s->path_limit_nr, s->path_limit, &rev, NULL);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_collect_changed_cb;
 	rev.diffopt.format_callback_data = s;
@@ -295,7 +301,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	struct rev_info rev;
 
 	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev,
+	setup_revisions(s->path_limit_nr, s->path_limit, &rev,
 		s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_collect_updated_cb;
@@ -315,6 +321,10 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 		struct wt_status_change_data *d;
 		struct cache_entry *ce = active_cache[i];
 
+		if (match_pathspec(s->path_limit + 1,
+				ce->name, strlen(ce->name), 0, NULL))
+			continue;
+
 		it = string_list_insert(ce->name, &s->change);
 		d = it->util;
 		if (!d) {
@@ -477,7 +487,7 @@ static void wt_status_print_untracked(struct wt_status *s)
 			DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
 	setup_standard_excludes(&dir);
 
-	fill_directory(&dir, NULL);
+	fill_directory(&dir, s->path_limit + 1);
 	for(i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
 		if (!cache_name_is_other(ent->name, ent->len))
@@ -559,6 +569,9 @@ void wt_status_print(struct wt_status *s)
 			wt_status_print_tracking(s);
 	}
 
+	ALLOC_GROW(s->path_limit, s->path_limit_nr + 1, s->path_limit_alloc);
+	s->path_limit[s->path_limit_nr] = NULL;
+
 	wt_status_collect_changes(s);
 
 	if (s->is_initial) {
diff --git a/wt-status.h b/wt-status.h
index f80142f..3099e99 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -43,6 +43,8 @@ struct wt_status {
 	FILE *fp;
 	const char *prefix;
 	struct string_list change;
+	const char **path_limit;
+	int path_limit_nr, path_limit_alloc;
 };
 
 int git_status_config(const char *var, const char *value, void *cb);

Breaking "git status" (was Re: [PATCH 5/5] shortstatus: a new command)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Missing docs, of course, and tests would be nice. Is this meant to be
> plumbing? If not, the name is too long to type. ;)

As I said, this was more of technology demonstration with an iffy UI.

I've been wondering if this should be "git status -T" (a la "ls-files -t",
but nice short-and-sweet -t is taken for rarely used --template).

> I think one of the most often-requested things for a "git status"
> replacement is that "git status <path>" do path-limiting.

For that matter, as 1.7.0 material that potentially breaks backward
compatibility in a big way, it is tempting to suggest that we might want
to depart from the traditional "status takes the same parameters to commit
and gives a preview of what would happen" semantics.  Even though the
current "status" takes the same set of parameters as "commit" takes, many
of the parameters, especially the ones related to message generation, do
not make sense.

Here is a possible transition plan.  I am not married to it, but throwing
it out as a discussion starter:

 * Introduce "git commit --dry-run", that takes place of the current
   "git status".

   We will keep "git commit --dry-run" forever so that people can simply
   do a "s/git status/git commit --dry-run/" to keep their own scripts
   that currently run "git status" before deciding to run "git commit" (or
   runs their own re-implementation of "git commit") working.

 * Introduce "status.traditional" configuration.  In 1.6.5

   - When set to true, "git status" behaves as "git commit --dry-run";

   - When set to false, "git status" uses a new semantics (TBD);

   - When unconfigured, the user gets a big incompatibility warning.

   and in 1.7.0, we will flip the default (i.e. unconfigured case) to
   false.

 * Implement "git status" that is not a preview of "git commit".  Its new
   semantics would include:

   - Reject any parameter that traditional "git status" ignored (i.e. -m);

   - Pathspecs are not about "git commit -o" anymore.  They are path
     limiters.

   - One of the options, -t, gives the "shortstatus".

If we go a route like this, we do not want to add "shortstatus" as a
standalone command.

Re: Breaking "git status" (was Re: [PATCH 5/5] shortstatus: a new command)

[Jeff King]

On Thu, Aug 06, 2009 at 09:23:48AM -0700, Junio C Hamano wrote:

> As I said, this was more of technology demonstration with an iffy UI.
> 
> I've been wondering if this should be "git status -T" (a la "ls-files -t",
> but nice short-and-sweet -t is taken for rarely used --template).

If the output format is the only change, then that probably makes sense,
but I think there is a desire for a command that is more substantially
different.

> Here is a possible transition plan.  I am not married to it, but throwing
> it out as a discussion starter:
> 
>  * Introduce "git commit --dry-run", that takes place of the current
>    "git status".
> 
>    We will keep "git commit --dry-run" forever so that people can simply
>    do a "s/git status/git commit --dry-run/" to keep their own scripts
>    that currently run "git status" before deciding to run "git commit" (or
>    runs their own re-implementation of "git commit") working.

That makes sense.

>  * Introduce "status.traditional" configuration.  In 1.6.5
> 
>    - When set to true, "git status" behaves as "git commit --dry-run";
> 
>    - When set to false, "git status" uses a new semantics (TBD);
> 
>    - When unconfigured, the user gets a big incompatibility warning.
> 
>    and in 1.7.0, we will flip the default (i.e. unconfigured case) to
>    false.

I wonder if introducing such a configuration option is not going to
cause more confusion in the long run than simply switching. As a
script-writer, you are not helping me at all because I can't make any
assumptions about how the user has set the variable.

I guess you are helping those who want to keep "git status" as-is
forever for their own typing. I'm not sure that such people exist, but I
guess it is better to be conservative.

>  * Implement "git status" that is not a preview of "git commit".  Its new
>    semantics would include:
> 
>    - Reject any parameter that traditional "git status" ignored (i.e. -m);
> 
>    - Pathspecs are not about "git commit -o" anymore.  They are path
>      limiters.
> 
>    - One of the options, -t, gives the "shortstatus".

Makes sense.

> If we go a route like this, we do not want to add "shortstatus" as a
> standalone command.

It may make sense to do it anyway, just to give a playground for people
to try out and refine the new interface. In other words, do:

  - introduce shortstatus (or newstatus, or whatever you want to call
    it)

  - wait a few cycles for it to prove itself

  - proceed with transition plan you described above

  - explain transition as "status is now newstatus, old status is now
    commit --dry-run".

-Peff

Re: Breaking "git status"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I wonder if introducing such a configuration option is not going to
> cause more confusion in the long run than simply switching. As a
> script-writer, you are not helping me at all because I can't make any
> assumptions about how the user has set the variable.
>
> I guess you are helping those who want to keep "git status" as-is
> forever for their own typing.

Not really.  I meant to be helpful to people who would say "Ok, I read the
transition plan, and I understand I eventually need to switch, but I
cannot afford to right at this momennt.  I'd transition on the timetable
of my choosing, rather than on the calendar git developers arbitrarily
set."

But you are right.  This would not really help scripts transition.

Re: Breaking "git status" (was Re: [PATCH 5/5] shortstatus: a new command)

[Sverre Rabbelier]

Heya,

On Thu, Aug 6, 2009 at 09:23, Junio C Hamano<gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>  * Implement "git status" that is not a preview of "git commit".  Its new
>   semantics would include:

FWIW, I think this is a a great idea, 'git status' being a preview of
'git commit' has never made sense to me from a UI perspective, and I
never actually use 'git status' for that purpose. While I do use it to
look at what I'm going to commit, I never use any 'git commit'
modifiers in combination with git status.

On that note, I agree with Jeff that a config option does not make a
lot of sense, especially if we introduce 'git commit -d|--dry-run'.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/5] status: show worktree status of conflicted paths separately

[Jeff King]

On Wed, Aug 05, 2009 at 02:15:45AM -0700, Junio C Hamano wrote:

> +	switch (d->stagemask >> 1) {
> +	case 1: how = "both deleted"; break;
> +	case 2: how = "added by us"; break;
> +	case 3: how = "deleted by them"; break;
> +	case 4: how = "added by them"; break;
> +	case 5: how = "deleted by us"; break;
> +	case 6: how = "both added"; break;
> +	case 7: how = "both modified"; break;
> +	}

Ugh. Can we use symbolic constants for the stagemask instead of a
bitshift and some magic numbers? I understand it's a mask and not just
an integer value, but we are clearly enumerating every possibility here.

> +	color_fprintf(s->fp, c, "%-20s: %s\n", how, one);

This produces output like:

#       both modified       : a
#       deleted by us       : b
#       deleted by them     : c

Maybe it is just me, but I think the whitespace with the colon looks
awful. You could do just:

#       both modified:        a
#       deleted by us:        b
#       deleted by them:      c

which matches the other status output, or even left-prefix the spaces:

#              both modified: a
#              deleted by us: b
#            deleted by them: c

-Peff

Re: [PATCH 3/5] wt-status.c: rework the way changes to the index and work tree are summarized

[Jeff King]

On Wed, Aug 05, 2009 at 02:15:44AM -0700, Junio C Hamano wrote:

> diff --git a/wt-status.c b/wt-status.c
> index 47735d8..1614352 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -20,6 +20,7 @@ static char wt_status_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_RED,    /* WT_STATUS_CHANGED */
>  	GIT_COLOR_RED,    /* WT_STATUS_UNTRACKED */
>  	GIT_COLOR_RED,    /* WT_STATUS_NOBRANCH */
> +	GIT_COLOR_YELLOW, /* WT_STATUS_UNMERGED */
>  };

Does this belong in 3/5? It looks like the WT_STATUS_UNMERGED symbol is
not used at all until 4/5, which seems like the more logical place
(since it deals explicitly with unmerged entries). Also, why does it
start yellow here and then turn to red in the next patch?

And related:

> diff --git a/wt-status.h b/wt-status.h
> index 78add09..f80142f 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> [...]
>  	WT_STATUS_NOBRANCH,
> +	WT_STATUS_UNMERGED,

-Peff

Re: [PATCH 3/5] wt-status.c: rework the way changes to the index and work tree are summarized

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Aug 05, 2009 at 02:15:44AM -0700, Junio C Hamano wrote:
>
>> diff --git a/wt-status.c b/wt-status.c
>> index 47735d8..1614352 100644
>> --- a/wt-status.c
>> +++ b/wt-status.c
>> @@ -20,6 +20,7 @@ static char wt_status_colors[][COLOR_MAXLEN] = {
>>  	GIT_COLOR_RED,    /* WT_STATUS_CHANGED */
>>  	GIT_COLOR_RED,    /* WT_STATUS_UNTRACKED */
>>  	GIT_COLOR_RED,    /* WT_STATUS_NOBRANCH */
>> +	GIT_COLOR_YELLOW, /* WT_STATUS_UNMERGED */
>>  };
>
> Does this belong in 3/5? It looks like the WT_STATUS_UNMERGED symbol is
> not used at all until 4/5, which seems like the more logical place
> (since it deals explicitly with unmerged entries). Also, why does it
> start yellow here and then turn to red in the next patch?
>
> And related:
>
>> diff --git a/wt-status.h b/wt-status.h
>> index 78add09..f80142f 100644
>> --- a/wt-status.h
>> +++ b/wt-status.h
>> [...]
>>  	WT_STATUS_NOBRANCH,
>> +	WT_STATUS_UNMERGED,

Thanks.  For the same reason, stagemask and unmerged_mask() helper
function should move from 3/5 to 4/5.

Re: [PATCH 0/5] Revamping "git status"

[Thomas Rast]

[-- Attachment #1: Type: Text/Plain, Size: 3953 bytes --]

Junio C Hamano wrote:
> 
> Junio C Hamano (5):
>   diff-index: report unmerged new entries
>   diff-index: keep the original index intact
>   wt-status.c: rework the way changes to the index and work tree are
>     summarized
>   status: show worktree status of conflicted paths separately
>   shortstatus: a new command

I was quite eager to try this, mainly for 4/5, and I still had the
testing repository from the last thread around:

  $ git ls-files -s
  100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 1       foo
  100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 2       foo

Here we go!

  $ git status
  # On branch master                            
  Segmentation fault

Uh oh.  So gdb it is then...

# On branch master                                                           

  Program received signal SIGSEGV, Segmentation fault.
  0x00007ffff7353844 in strcmp () from /lib64/libc.so.6
  (gdb) bt                                             
  #0  0x00007ffff7353844 in strcmp () from /lib64/libc.so.6
  #1  0x00000000004cc577 in unmerged_mask (path=Cannot access memory at address 0xfffff070                                                                        
  ) at wt-status.c:241                                                            
  Backtrace stopped: previous frame inner to this frame (corrupt stack?)

... or maybe not.  valgrind is slightly more helpful:

  ==29421== Invalid read of size 1
  ==29421==    at 0x4C26101: strcmp (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
  ==29421==    by 0x4CC576: unmerged_mask (wt-status.c:241)
  ==29421==    by 0x4CC6AA: wt_status_collect_updated_cb (wt-status.c:275)
  ==29421==    by 0x484172: diff_flush (diff.c:3337)
  ==29421==    by 0x47A0E5: run_diff_index (diff-lib.c:445)
  ==29421==    by 0x4CC804: wt_status_collect_changes_index (wt-status.c:306)
  ==29421==    by 0x4CC922: wt_status_collect_changes (wt-status.c:340)
  ==29421==    by 0x4CD1A0: wt_status_print (wt-status.c:561)
  ==29421==    by 0x41BFBD: run_status (builtin-commit.c:369)
  ==29421==    by 0x41D97F: cmd_status (builtin-commit.c:927)
  ==29421==    by 0x4048C2: run_builtin (git.c:246)
  ==29421==    by 0x404A4D: handle_internal_command (git.c:394)
  ==29421==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
  ==29421==
  ==29421== Process terminating with default action of signal 11 (SIGSEGV)
  ==29421==  Access not within mapped region at address 0x48
  ==29421==    at 0x4C26101: strcmp (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
  ==29421==    by 0x4CC576: unmerged_mask (wt-status.c:241)
  ==29421==    by 0x4CC6AA: wt_status_collect_updated_cb (wt-status.c:275)
  ==29421==    by 0x484172: diff_flush (diff.c:3337)
  ==29421==    by 0x47A0E5: run_diff_index (diff-lib.c:445)
  ==29421==    by 0x4CC804: wt_status_collect_changes_index (wt-status.c:306)
  ==29421==    by 0x4CC922: wt_status_collect_changes (wt-status.c:340)
  ==29421==    by 0x4CD1A0: wt_status_print (wt-status.c:561)
  ==29421==    by 0x41BFBD: run_status (builtin-commit.c:369)
  ==29421==    by 0x41D97F: cmd_status (builtin-commit.c:927)
  ==29421==    by 0x4048C2: run_builtin (git.c:246)
  ==29421==    by 0x404A4D: handle_internal_command (git.c:394)

I also tried finding out which exact commit was causing this, but 4/5
still segfaults and 1-3 don't even compile:

  builtin-commit.c: In function ‘show_unmerged’:                                  
  builtin-commit.c:827: error: dereferencing pointer to incomplete type           
  builtin-commit.c: In function ‘show_status’:                                    
  builtin-commit.c:854: error: dereferencing pointer to incomplete type
  [etc]

These are referring to use of a 'struct wt_status_change_data *', but
the struct declaration is only in 4/5.  Am I missing something?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/5] Revamping "git status"

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> I also tried finding out which exact commit was causing this, but 4/5
> still segfaults and 1-3 don't even compile:
>
>   builtin-commit.c: In function ‘show_unmerged’:                                  
>   builtin-commit.c:827: error: dereferencing pointer to incomplete type           
>   builtin-commit.c: In function ‘show_status’:                                    
>   builtin-commit.c:854: error: dereferencing pointer to incomplete type
>   [etc]
>
> These are referring to use of a 'struct wt_status_change_data *', but
> the struct declaration is only in 4/5.  Am I missing something?

I suspect you have a botched patch application.

The result of applying up to [PATCH 3/5] does not even have
show_unmerged/show_status in builtin-commit.c.  These two functions are
introduced by [PATCH 5/5] to implement shortstatus.

I've applied what came back on the list on top of 07a4a3b (Fix typos on
pt_BR/gittutorial.txt translation, 2009-07-31) and all five states compile
just fine.

    $ git rev-list 07a4a3b.. |
      while read sha1
      do
            git rev-parse $sha1^{tree}
      done
    86363b25b84041cf14110dcc3136f56915778f71
    4625f5bbc43e1158d00aca8b2356047606c0babd
    895698143b2c29af8bfe0cdf6c3f57de3bf080d8
    3c565041ba6e432ff9064a1d8302f49ef33c2605
    c0c2b9c2a55e3df9cce448d59e5d557d07d78a4b

Re: [PATCH 0/5] Revamping "git status"

[Thomas Rast]

Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > I also tried finding out which exact commit was causing this, but 4/5
> > still segfaults and 1-3 don't even compile:
> >
> >   builtin-commit.c: In function ‘show_unmerged’:                                  
> >   builtin-commit.c:827: error: dereferencing pointer to incomplete type           
> >   builtin-commit.c: In function ‘show_status’:                                    
> >   builtin-commit.c:854: error: dereferencing pointer to incomplete type
> >   [etc]
> >
> > These are referring to use of a 'struct wt_status_change_data *', but
> > the struct declaration is only in 4/5.  Am I missing something?
> 
> I suspect you have a botched patch application.

You're right, sorry.  I indeed had the patches applied in the wrong
order (through a pilot error with KMail), resulting in 5/5 being first
in the topic.  So it is no wonder the intermediate states never
compiled.

> I've applied what came back on the list on top of 07a4a3b (Fix typos on
> pt_BR/gittutorial.txt translation, 2009-07-31) and all five states compile
> just fine.
> 
>     $ git rev-list 07a4a3b.. |
>       while read sha1
>       do
>             git rev-parse $sha1^{tree}
>       done
>     86363b25b84041cf14110dcc3136f56915778f71
>     4625f5bbc43e1158d00aca8b2356047606c0babd
>     895698143b2c29af8bfe0cdf6c3f57de3bf080d8
>     3c565041ba6e432ff9064a1d8302f49ef33c2605
>     c0c2b9c2a55e3df9cce448d59e5d557d07d78a4b

I did exactly that, and our trees now agree.  However, I'm still
seeing a crash with this test:

-- 8< --
diff --git i/t/t7060-wtstatus.sh w/t/t7060-wtstatus.sh
index 5ad2cd1..2cc0833 100755
--- i/t/t7060-wtstatus.sh
+++ w/t/t7060-wtstatus.sh
@@ -28,4 +28,17 @@ test_expect_success 'Report new path with conflict' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'M/D conflict does not segfault' '
+	mkdir mdconflict &&
+	cd mdconflict &&
+	git init &&
+	test_commit initial foo '' &&
+	test_commit modify foo foo &&
+	git checkout -b side HEAD^ &&
+	git rm foo &&
+	git commit -m delete &&
+	git merge master &&
+	git status
+'
+
 test_done
-- >8 --

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 0/5] Revamping "git status"

[Thomas Rast]

Thomas Rast wrote:
> +test_expect_failure 'M/D conflict does not segfault' '
> +	mkdir mdconflict &&
> +	cd mdconflict &&
> +	git init &&
> +	test_commit initial foo '' &&
> +	test_commit modify foo foo &&
> +	git checkout -b side HEAD^ &&
> +	git rm foo &&
> +	git commit -m delete &&
> +	git merge master &&
> +	git status
> +'
> +

Meh, this stops before the git-status because merge signals a
conflict.  However, the following really crashes.  Promise.

-- 8< --
diff --git i/t/t7060-wtstatus.sh w/t/t7060-wtstatus.sh
index 5ad2cd1..a152a26 100755
--- i/t/t7060-wtstatus.sh
+++ w/t/t7060-wtstatus.sh
@@ -28,4 +28,17 @@ test_expect_success 'Report new path with conflict' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'M/D conflict does not segfault' '
+	mkdir mdconflict &&
+	cd mdconflict &&
+	git init &&
+	test_commit initial foo '' &&
+	test_commit modify foo foo &&
+	git checkout -b side HEAD^ &&
+	git rm foo &&
+	git commit -m delete &&
+	test_must_fail git merge master &&
+	git status
+'
+
 test_done
-- >8 --

Sorry for all the confusion.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 0/5] Revamping "git status"

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Meh, this stops before the git-status because merge signals a
> conflict.  However, the following really crashes.  Promise.

Thanks.

 wt-status.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index f73bf47..6370fe2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -236,7 +236,7 @@ static int unmerged_mask(const char *path)
 
 	mask = 0;
 	pos = -pos-1;
-	while (pos <= active_nr) {
+	while (pos < active_nr) {
 		ce = active_cache[pos++];
 		if (strcmp(ce->name, path))
 			break;

Re: [PATCH 0/5] Revamping "git status"

[Thomas Rast]

Junio C Hamano wrote:
> 
> -	while (pos <= active_nr) {
> +	while (pos < active_nr) {

Thanks, that fixes it.

It gives a weird output now, however:

  # On branch side
  # Unmerged paths:
  #   (use "git reset HEAD <file>..." to unstage)
  #   (use "git add <file>..." to mark resolution)
  #
  #	deleted by us       : foo
  #
  # Changed but not updated:
  #   (use "git add <file>..." to update what will be committed)
  #   (use "git checkout -- <file>..." to discard changes in working directory)
  #
  #
  no changes added to commit (use "git add" and/or "git commit -a")

So it detects there are worktree changes, but then decides not to show
them because it's an unmerged entry.  I think the following should go
in 3/5, but note that I haven't looked at the rest of the code to
check if it breaks anything:

-- 8< --
diff --git i/wt-status.c w/wt-status.c
index 6370fe2..5a68297 100644
--- i/wt-status.c
+++ w/wt-status.c
@@ -400,7 +400,8 @@ static int wt_status_check_worktree_changes(struct wt_status *s)
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		d = s->change.items[i].util;
-		if (!d->worktree_status)
+		if (!d->worktree_status
+		    || d->index_status == DIFF_STATUS_UNMERGED)
 			continue;
 		changes = 1;
 		if (d->worktree_status == DIFF_STATUS_DELETED)
-- >8 --

And with that fixed, the test I posted earlier should be changed to
the following so that it checks the output (the old version detects
failure even for a working git-status, because that exits 1):

-- 8< --
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 5ad2cd1..76b20b2 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -28,4 +28,29 @@ test_expect_success 'Report new path with conflict' '
 	test_cmp expect actual
 '
 
+cat > expect <<EOF
+# On branch side
+# Unmerged paths:
+#   (use "git reset HEAD <file>..." to unstage)
+#   (use "git add <file>..." to mark resolution)
+#
+#	deleted by us       : foo
+#
+no changes added to commit (use "git add" and/or "git commit -a")
+EOF
+
+test_expect_success 'M/D conflict does not segfault' '
+	mkdir mdconflict &&
+	cd mdconflict &&
+	git init &&
+	test_commit initial foo '' &&
+	test_commit modify foo foo &&
+	git checkout -b side HEAD^ &&
+	git rm foo &&
+	git commit -m delete &&
+	test_must_fail git merge master &&
+	test_must_fail git status > ../actual &&
+	test_cmp ../expect ../actual
+'
+
 test_done
-- >8 --

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 0/5] Revamping "git status"

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> So it detects there are worktree changes, but then decides not to show
> them because it's an unmerged entry.  I think the following should go
> in 3/5, but note that I haven't looked at the rest of the code to
> check if it breaks anything:

Thanks.  Shouldn't it go in 4/5 instead, though?

> -- 8< --
> diff --git i/wt-status.c w/wt-status.c
> index 6370fe2..5a68297 100644
> --- i/wt-status.c
> +++ w/wt-status.c
> @@ -400,7 +400,8 @@ static int wt_status_check_worktree_changes(struct wt_status *s)
>  	for (i = 0; i < s->change.nr; i++) {
>  		struct wt_status_change_data *d;
>  		d = s->change.items[i].util;
> -		if (!d->worktree_status)
> +		if (!d->worktree_status
> +		    || d->index_status == DIFF_STATUS_UNMERGED)
>  			continue;
>  		changes = 1;
>  		if (d->worktree_status == DIFF_STATUS_DELETED)
> -- >8 --

Not "d->worktree_status"?  That would be more consistent with what
wt_status_print_changed() actually ends up checking.

Re: [PATCH 0/5] Revamping "git status"

[Thomas Rast]

Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > So it detects there are worktree changes, but then decides not to show
> > them because it's an unmerged entry.  I think the following should go
> > in 3/5, but note that I haven't looked at the rest of the code to
> > check if it breaks anything:
> 
> Thanks.  Shouldn't it go in 4/5 instead, though?

Er, yeah. *sigh*

> > -- 8< --
> > diff --git i/wt-status.c w/wt-status.c
> > index 6370fe2..5a68297 100644
> > --- i/wt-status.c
> > +++ w/wt-status.c
> > @@ -400,7 +400,8 @@ static int wt_status_check_worktree_changes(struct wt_status *s)
> >  	for (i = 0; i < s->change.nr; i++) {
> >  		struct wt_status_change_data *d;
> >  		d = s->change.items[i].util;
> > -		if (!d->worktree_status)
> > +		if (!d->worktree_status
> > +		    || d->index_status == DIFF_STATUS_UNMERGED)
> >  			continue;
> >  		changes = 1;
> >  		if (d->worktree_status == DIFF_STATUS_DELETED)
> > -- >8 --
> 
> Not "d->worktree_status"?  That would be more consistent with what
> wt_status_print_changed() actually ends up checking.

Hmm, true.  I just picked index_status because unmerged state is an
index property.  It's probably better if the two functions agree on
the criterion.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch