[PATCH v1 0/6] Porcelain Status V2

[PATCH v1 0/6] Porcelain Status V2

[Jeff Hostetler]

This patch series adds porcelain V2 format to status.
This provides detailed information about file changes
and about the current branch.

The new output is accessed via:
    git status --porcelain=2 [--branch]

An earlier draft of this work was submitted under the
"Add very verbose porcelain output to status" title.
This new version addresses the concerns about using
(abusing) "-vv" and simplifies the some of the formatting.

This version does not include the state header from my
first draft.  I agree that if guarded by a "--state"
argument, that output could be added to both formats.


Jeff Hostetler (6):
  Allow --porcelain[=<n>] in status and commit commands
  Status and checkout unit tests for --porcelain[=<n>]
  Per-file output for Porcelain Status V2
  Expanded branch header for Porcelain Status V2
  Add porcelain V2 documentation to status manpage
  Unit tests for V2 porcelain status

 Documentation/git-commit.txt |   2 +-
 Documentation/git-status.txt |  69 +++++-
 builtin/commit.c             |  54 +++--
 t/t7060-wtstatus.sh          |  21 ++
 t/t7064-wtstatus-pv2.sh      | 461 ++++++++++++++++++++++++++++++++++++++++
 t/t7501-commit.sh            |  23 ++
 wt-status.c                  | 487 ++++++++++++++++++++++++++++++++++++++++++-
 wt-status.h                  |  25 +++
 8 files changed, 1122 insertions(+), 20 deletions(-)
 create mode 100755 t/t7064-wtstatus-pv2.sh

-- 
2.8.0.rc4.17.gac42084.dirty

[PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

[Jeff Hostetler]

Update the --porcelain argument to take an optional
version number.  This will allow us to define new
porcelain formats in the future.

This default to 1 and represents the existing porcelain
format.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/git-commit.txt |  2 +-
 Documentation/git-status.txt |  7 +++++--
 builtin/commit.c             | 40 +++++++++++++++++++++++++++-------------
 wt-status.h                  | 10 ++++++++++
 4 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index b0a294d..0791573 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -104,7 +104,7 @@ OPTIONS
 --branch::
 	Show the branch and tracking info even in short-format.
 
---porcelain::
+--porcelain[=<version>]::
 	When doing a dry-run, give the output in a porcelain-ready
 	format. See linkgit:git-status[1] for details. Implies
 	`--dry-run`.
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index e1e8f57..de97729 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,11 +32,14 @@ OPTIONS
 --branch::
 	Show the branch and tracking info even in short-format.
 
---porcelain::
+--porcelain[=<version>]::
 	Give the output in an easy-to-parse format for scripts.
 	This is similar to the short output, but will remain stable
 	across Git versions and regardless of user configuration. See
 	below for details.
++
+The version parameter is used to specify the format version.
+This is optional and defaults to the original version '1' format.
 
 --long::
 	Give the output in the long-format. This is the default.
@@ -96,7 +99,7 @@ configuration variable documented in linkgit:git-config[1].
 
 -z::
 	Terminate entries with NUL, instead of LF.  This implies
-	the `--porcelain` output format if no other format is given.
+	the `--porcelain=1` output format if no other format is given.
 
 --column[=<options>]::
 --no-column::
diff --git a/builtin/commit.c b/builtin/commit.c
index 1f6dbcd..892d7f7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,24 @@ static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
-static enum status_format {
-	STATUS_FORMAT_NONE = 0,
-	STATUS_FORMAT_LONG,
-	STATUS_FORMAT_SHORT,
-	STATUS_FORMAT_PORCELAIN,
+static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
-	STATUS_FORMAT_UNSPECIFIED
-} status_format = STATUS_FORMAT_UNSPECIFIED;
+static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
+{
+	enum wt_status_format *value = (enum wt_status_format *)opt->value;
+	if (unset) {
+		*value = STATUS_FORMAT_UNSPECIFIED;
+	} else if (arg) {
+		int n = strtol(arg, NULL, 10);
+		if (n == 1)
+			*value = STATUS_FORMAT_PORCELAIN;
+		else
+			die("unsupported porcelain version");
+	} else {
+		*value = STATUS_FORMAT_PORCELAIN;
+	}
+	return 0;
+}
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
@@ -500,6 +510,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	s->fp = fp;
 	s->nowarn = nowarn;
 	s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+	s->status_format = status_format;
 
 	wt_status_collect(s);
 
@@ -1099,7 +1110,7 @@ static const char *read_commit_message(const char *name)
  * is not in effect here.
  */
 static struct status_deferred_config {
-	enum status_format status_format;
+	enum wt_status_format status_format;
 	int show_branch;
 } status_deferred_config = {
 	STATUS_FORMAT_UNSPECIFIED,
@@ -1336,9 +1347,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			    N_("show status concisely"), STATUS_FORMAT_SHORT),
 		OPT_BOOL('b', "branch", &s.show_branch,
 			 N_("show branch information")),
-		OPT_SET_INT(0, "porcelain", &status_format,
-			    N_("machine-readable output"),
-			    STATUS_FORMAT_PORCELAIN),
+		{ OPTION_CALLBACK, 0, "porcelain", &status_format,
+		  N_("version"), N_("machine-readable output"),
+		  PARSE_OPT_OPTARG, opt_parse_porcelain },
 		OPT_SET_INT(0, "long", &status_format,
 			    N_("show status in long format (default)"),
 			    STATUS_FORMAT_LONG),
@@ -1381,6 +1392,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
 	s.ignore_submodule_arg = ignore_submodule_arg;
+	s.status_format = status_format;
+
 	wt_status_collect(&s);
 
 	if (0 <= fd)
@@ -1622,8 +1635,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
 			    STATUS_FORMAT_SHORT),
 		OPT_BOOL(0, "branch", &s.show_branch, N_("show branch information")),
-		OPT_SET_INT(0, "porcelain", &status_format,
-			    N_("machine-readable output"), STATUS_FORMAT_PORCELAIN),
+		{ OPTION_CALLBACK, 0, "porcelain", &status_format,
+		  N_("version"), N_("machine-readable output"),
+		  PARSE_OPT_OPTARG, opt_parse_porcelain },
 		OPT_SET_INT(0, "long", &status_format,
 			    N_("show status in long format (default)"),
 			    STATUS_FORMAT_LONG),
diff --git a/wt-status.h b/wt-status.h
index 2ca93f6..fc80341 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -43,6 +43,15 @@ struct wt_status_change_data {
 	unsigned new_submodule_commits : 1;
 };
 
+ enum wt_status_format {
+	STATUS_FORMAT_NONE = 0,
+	STATUS_FORMAT_LONG,
+	STATUS_FORMAT_SHORT,
+	STATUS_FORMAT_PORCELAIN,
+
+	STATUS_FORMAT_UNSPECIFIED
+ };
+
 struct wt_status {
 	int is_initial;
 	char *branch;
@@ -66,6 +75,7 @@ struct wt_status {
 	int show_branch;
 	int hints;
 
+	enum wt_status_format status_format;
 	/* These are computed during processing of the individual sections */
 	int commitable;
 	int workdir_dirty;
-- 
2.8.0.rc4.17.gac42084.dirty

[PATCH v1 2/6] Status and checkout unit tests for --porcelain[=<n>]

[Jeff Hostetler]

Simple unit tests to validate the argument parsing.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t7060-wtstatus.sh | 21 +++++++++++++++++++++
 t/t7501-commit.sh   | 14 ++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 44bf1d8..a39b0e2 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -228,4 +228,25 @@ test_expect_success 'status --branch with detached HEAD' '
 	test_i18ncmp expected actual
 '
 
+## Duplicate the above test and verify --porcelain=1 has no effect.
+test_expect_success 'status --branch with detached HEAD' '
+	git reset --hard &&
+	git checkout master^0 &&
+	git status --branch --porcelain=1 >actual &&
+	cat >expected <<-EOF &&
+	## HEAD (no branch)
+	?? .gitconfig
+	?? actual
+	?? expect
+	?? expected
+	?? mdconflict/
+	EOF
+	test_i18ncmp expected actual
+'
+
+## Verify parser error on --porcelain argument.
+test_expect_failure 'status --porcelain=bogus' '
+	git status --porcelain=bogus
+'
+
 test_done
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a..1a74d75 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -73,6 +73,10 @@ test_expect_success '--porcelain fails with nothing to commit' '
 	test_must_fail git commit -m initial --porcelain
 '
 
+test_expect_success '--porcelain=1 fails with nothing to commit' '
+	test_must_fail git commit -m initial --porcelain=1
+'
+
 test_expect_success '--long fails with nothing to commit' '
 	test_must_fail git commit -m initial --long
 '
@@ -97,6 +101,16 @@ test_expect_failure '--porcelain with stuff to commit returns ok' '
 	git commit -m next -a --porcelain
 '
 
+test_expect_failure '--porcelain=1 with stuff to commit returns ok' '
+	echo bongo bongo bongo >>file &&
+	git commit -m next -a --porcelain=1
+'
+
+test_expect_failure '--porcelain=bogus with stuff to commit returns ok' '
+	echo bongo bongo bongo >>file &&
+	git commit -m next -a --porcelain=bogus
+'
+
 test_expect_success '--long with stuff to commit returns ok' '
 	echo bongo bongo bongo >>file &&
 	git commit -m next -a --long
-- 
2.8.0.rc4.17.gac42084.dirty

[PATCH v1 3/6] Per-file output for Porcelain Status V2

[Jeff Hostetler]

This commit sets up version 2 porcelain status and
defines the format of detail lines.  This includes
the usual XY and pathname fields.  It adds the various
file modes and SHAs and the rename score.  For regular
entries these values reflect the head, index and
worktree. For unmerged entries these values reflect
the stage 1, 2, and 3 values.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/commit.c |   9 ++
 wt-status.c      | 398 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 wt-status.h      |  13 ++
 3 files changed, 419 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 892d7f7..b5ec9b9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -153,6 +153,8 @@ static int opt_parse_porcelain(const struct option *opt, const char *arg, int un
 		int n = strtol(arg, NULL, 10);
 		if (n == 1)
 			*value = STATUS_FORMAT_PORCELAIN;
+		else if (n == 2)
+			*value = STATUS_FORMAT_PORCELAIN_V2;
 		else
 			die("unsupported porcelain version");
 	} else {
@@ -521,6 +523,9 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	case STATUS_FORMAT_PORCELAIN:
 		wt_porcelain_print(s);
 		break;
+	case STATUS_FORMAT_PORCELAIN_V2:
+		wt_porcelain_v2_print(s);
+		break;
 	case STATUS_FORMAT_UNSPECIFIED:
 		die("BUG: finalize_deferred_config() should have been called");
 		break;
@@ -1120,6 +1125,7 @@ static struct status_deferred_config {
 static void finalize_deferred_config(struct wt_status *s)
 {
 	int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
+				   status_format != STATUS_FORMAT_PORCELAIN_V2 &&
 				   !s->null_termination);
 
 	if (s->null_termination) {
@@ -1409,6 +1415,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	case STATUS_FORMAT_PORCELAIN:
 		wt_porcelain_print(&s);
 		break;
+	case STATUS_FORMAT_PORCELAIN_V2:
+		wt_porcelain_v2_print(&s);
+		break;
 	case STATUS_FORMAT_UNSPECIFIED:
 		die("BUG: finalize_deferred_config() should have been called");
 		break;
diff --git a/wt-status.c b/wt-status.c
index c19b52c..2d4829e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -406,6 +406,89 @@ static void wt_status_print_change_data(struct wt_status *s,
 	strbuf_release(&twobuf);
 }
 
+
+/* Copy info for both sides of a head-vs-index change
+ * into the Porcelain V2 data.
+ */
+static void porcelain_v2_updated_entry(
+	struct wt_status_change_data *d,
+	struct diff_filepair *p)
+{
+	switch (p->status) {
+	case DIFF_STATUS_ADDED:
+		d->porcelain_v2.mode_index = p->two->mode;
+		hashcpy(d->porcelain_v2.sha1_index, p->two->sha1);
+		break;
+
+	case DIFF_STATUS_DELETED:
+		d->porcelain_v2.mode_head = p->one->mode;
+		hashcpy(d->porcelain_v2.sha1_head, p->one->sha1);
+		break;
+
+	case DIFF_STATUS_RENAMED:
+		d->porcelain_v2.rename_score = p->score * 100 / MAX_SCORE;
+	case DIFF_STATUS_COPIED:
+	case DIFF_STATUS_MODIFIED:
+	case DIFF_STATUS_TYPE_CHANGED:
+	case DIFF_STATUS_UNMERGED:
+		d->porcelain_v2.mode_head = p->one->mode;
+		d->porcelain_v2.mode_index = p->two->mode;
+		hashcpy(d->porcelain_v2.sha1_head, p->one->sha1);
+		hashcpy(d->porcelain_v2.sha1_index, p->two->sha1);
+		break;
+
+	case DIFF_STATUS_UNKNOWN:
+		/* This should never happen. */
+		break;
+	}
+}
+
+/* Copy info for both sides of an index-vs-worktree change
+ * into the very verbose porcelain data.
+ */
+static void porcelain_v2_changed_entry(
+	struct wt_status_change_data *d,
+	const struct diff_filepair *p)
+{
+	switch (p->status) {
+	case DIFF_STATUS_ADDED:
+		d->porcelain_v2.mode_worktree = p->two->mode;
+		/* don't bother with worktree sha, since it is almost always zero. */
+		break;
+
+	case DIFF_STATUS_DELETED:
+		d->porcelain_v2.mode_index = p->one->mode;
+		hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
+		break;
+
+	case DIFF_STATUS_COPIED:
+	case DIFF_STATUS_MODIFIED:
+	case DIFF_STATUS_RENAMED:
+	case DIFF_STATUS_TYPE_CHANGED:
+		d->porcelain_v2.mode_index = p->one->mode;
+		d->porcelain_v2.mode_worktree = p->two->mode;
+		hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
+		/* don't bother with worktree sha, since it is almost always zero. */
+		break;
+
+	case DIFF_STATUS_UNKNOWN:
+		/* This should never happen. */
+		break;
+
+	case DIFF_STATUS_UNMERGED:
+		/* This should never happen. */
+		break;
+	}
+}
+
+static void porcelain_v2_added_initial_entry(
+	struct wt_status_change_data *d,
+	const struct cache_entry *ce)
+{
+	d->porcelain_v2.mode_index = ce->ce_mode;
+	hashcpy(d->porcelain_v2.sha1_index, ce->sha1);
+}
+
 static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 					 struct diff_options *options,
 					 void *data)
@@ -433,6 +516,9 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 		d->dirty_submodule = p->two->dirty_submodule;
 		if (S_ISGITLINK(p->two->mode))
 			d->new_submodule_commits = !!hashcmp(p->one->sha1, p->two->sha1);
+
+		if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
+			porcelain_v2_changed_entry(d, p);
 	}
 }
 
@@ -486,6 +572,9 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
 			d->stagemask = unmerged_mask(p->two->path);
 			break;
 		}
+
+		if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
+			porcelain_v2_updated_entry(d, p);
 	}
 }
 
@@ -565,8 +654,12 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 			d->index_status = DIFF_STATUS_UNMERGED;
 			d->stagemask |= (1 << (ce_stage(ce) - 1));
 		}
-		else
+		else {
 			d->index_status = DIFF_STATUS_ADDED;
+
+			if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
+				porcelain_v2_added_initial_entry(d, ce);
+		}
 	}
 }
 
@@ -1753,3 +1846,306 @@ void wt_porcelain_print(struct wt_status *s)
 	s->no_gettext = 1;
 	wt_shortstatus_print(s);
 }
+
+/* Convert various submodule status values into a
+ * string of characters in the buffer provided.
+ */
+static void wt_porcelain_v2_submodule_state(
+	struct wt_status_change_data *d,
+	char sub[5])
+{
+	int k = 0;
+
+	if (S_ISGITLINK(d->porcelain_v2.mode_head) ||
+		S_ISGITLINK(d->porcelain_v2.mode_index) ||
+		S_ISGITLINK(d->porcelain_v2.mode_worktree)) {
+		/* We have a submodule */
+		sub[k++] = 'S';
+
+		/* Sub-flags for each type of dirt */
+		if (d->new_submodule_commits)
+			sub[k++] = 'C';
+		if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+			sub[k++] = 'M';
+		if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+			sub[k++] = 'U';
+	} else {
+		/* Not a submodule */
+		sub[k++] = 'N';
+	}
+
+	sub[k] = 0;
+}
+
+/* Various fix-up steps before we start printing an item.
+ */
+static void wt_porcelain_v2_fix_up_status(
+	struct string_list_item *it,
+	struct wt_status *s)
+{
+	struct wt_status_change_data *d = it->util;
+
+	if (!d->index_status) {
+		if (d->worktree_status == DIFF_STATUS_MODIFIED ||
+			d->worktree_status == DIFF_STATUS_DELETED) {
+			/* X=' ' Y=[MD]
+			 * The item did not change in head-vs-index scan so the head
+			 * column was never set. (The index column was set during the
+			 * index-vs-worktree scan.)
+			 * Force set the head column to make the output complete.
+			 */
+			d->porcelain_v2.mode_head = d->porcelain_v2.mode_index;
+			hashcpy(d->porcelain_v2.sha1_head, d->porcelain_v2.sha1_index);
+		}
+	}
+
+	if (!d->worktree_status) {
+		if (d->index_status == DIFF_STATUS_MODIFIED ||
+			d->index_status == DIFF_STATUS_ADDED ||
+			d->index_status == DIFF_STATUS_RENAMED ||
+			d->index_status == DIFF_STATUS_COPIED) {
+			/* X=[MARC] Y=' '
+			 * The item did not changed in the index-vs-worktree scan so
+			 * the worktree column was never set.
+			 * Force set the worktree mode to make the output complete.
+			 */
+			d->porcelain_v2.mode_worktree = d->porcelain_v2.mode_index;
+		}
+	}
+}
+
+/*
+ * Define a single format for tracked entries. This includes:
+ * normal changes, rename changes, and unmerged changes.
+ *
+ * The meanings of modes_[abcd] and sha_[abc] depends on the
+ * change type, but are always present.
+ *
+ * Path(s) are C-Quoted if necessary. Current path is ALWAYS
+ * first. The rename source path is only present when necessary.
+ * A single TAB separates them (because paths can contain spaces
+ * and C-Quoting converts actual tabs in pathnames to a C escape
+ * sequence).
+ */
+static void wt_porcelain_v2_print_tracked_entry(
+	FILE *fp,
+	char x_staged,
+	char y_unstaged,
+	const char *submodule,
+	int mode_a,
+	int mode_b,
+	int mode_c,
+	int mode_d,
+	const unsigned char sha_a[GIT_SHA1_RAWSZ],
+	const unsigned char sha_b[GIT_SHA1_RAWSZ],
+	const unsigned char sha_c[GIT_SHA1_RAWSZ],
+	int rename_score,
+	const char *path_current,
+	const char *path_rename_src,
+	int null_termination)
+{
+	char sep_char = null_termination ? '\0' : '\t';
+	char eol_char = null_termination ? '\0' : '\n';
+
+	if (path_rename_src)
+		fprintf(fp, "%c%c %s %06o %06o %06o %06o %s %s %s R%d %s%c%s%c",
+				x_staged, y_unstaged, submodule,
+				mode_a, mode_b, mode_c, mode_d,
+				sha1_to_hex(sha_a), sha1_to_hex(sha_b), sha1_to_hex(sha_c),
+				rename_score,
+				path_current, sep_char, path_rename_src,
+				eol_char);
+	else
+		fprintf(fp, "%c%c %s %06o %06o %06o %06o %s %s %s R%d %s%c",
+				x_staged, y_unstaged, submodule,
+				mode_a, mode_b, mode_c, mode_d,
+				sha1_to_hex(sha_a), sha1_to_hex(sha_b), sha1_to_hex(sha_c),
+				rename_score,
+				path_current,
+				eol_char);
+}
+
+/*
+ * Print porcelain V2 info for normal tracked entries.
+ */
+static void wt_porcelain_v2_print_normal_entry(
+	struct string_list_item *it,
+	struct wt_status *s)
+{
+	static const unsigned char sha_zero[GIT_SHA1_RAWSZ] = {0};
+	struct wt_status_change_data *d = it->util;
+	struct strbuf buf_current = STRBUF_INIT;
+	struct strbuf buf_rename_src = STRBUF_INIT;
+	const char *path_current = NULL;
+	const char *path_rename_src = NULL;
+	char x_staged, y_unstaged;
+	char submodule[5];
+
+	wt_porcelain_v2_fix_up_status(it, s);
+	x_staged = d->index_status ? d->index_status : '.';
+	y_unstaged = d->worktree_status ? d->worktree_status : '.';
+	wt_porcelain_v2_submodule_state(d, submodule);
+
+	if (s->null_termination) {
+		path_current = it->string;
+		path_rename_src = d->head_path;
+	} else {
+		path_current = quote_path(it->string, s->prefix, &buf_current);
+		if (d->head_path)
+			path_rename_src = quote_path(d->head_path, s->prefix, &buf_rename_src);
+	}
+
+	wt_porcelain_v2_print_tracked_entry(
+		s->fp,
+		x_staged, y_unstaged, submodule,
+		d->porcelain_v2.mode_head,
+		d->porcelain_v2.mode_index,
+		d->porcelain_v2.mode_worktree,
+		0,
+		d->porcelain_v2.sha1_head,
+		d->porcelain_v2.sha1_index,
+		sha_zero,
+		d->porcelain_v2.rename_score,
+		path_current,
+		path_rename_src,
+		s->null_termination);
+
+	strbuf_release(&buf_current);
+	strbuf_release(&buf_rename_src);
+}
+
+/*
+ * Print very verbose porcelain status info for unmerged entries.
+ */
+static void wt_porcelain_v2_print_unmerged_entry(
+	struct string_list_item *it,
+	struct wt_status *s)
+{
+	struct wt_status_change_data *d = it->util;
+	const struct cache_entry *ce;
+	struct strbuf buf_current = STRBUF_INIT;
+	const char *path_current = NULL;
+	int pos, stage;
+	struct {
+		int mode;
+		unsigned char sha1[GIT_SHA1_RAWSZ];
+	} stages[3];
+	char x_us = 'U', y_them = 'U';
+	char submodule[5];
+
+	switch (d->stagemask) {
+	case 1: x_us = 'D'; y_them = 'D'; break; /* both deleted */
+	case 2: x_us = 'A'; y_them = 'U'; break; /* added by us */
+	case 3: x_us = 'U'; y_them = 'D'; break; /* deleted by them */
+	case 4: x_us = 'U'; y_them = 'A'; break; /* added by them */
+	case 5: x_us = 'D'; y_them = 'U'; break; /* deleted by us */
+	case 6: x_us = 'A'; y_them = 'A'; break; /* both added */
+	case 7: x_us = 'U'; y_them = 'U'; break; /* both modified */
+	}
+
+	wt_porcelain_v2_submodule_state(d, submodule);
+
+	/*
+	 * Disregard the V2 {mode,sha} values for head and index
+	 * that we computed from the diffs and lookup the actual
+	 * stage data.
+	 */
+	memset(stages, 0, sizeof(stages));
+	pos = cache_name_pos(it->string, strlen(it->string));
+	assert(pos < 0);
+	pos = -pos-1;
+	while (pos < active_nr) {
+		ce = active_cache[pos++];
+		stage = ce_stage(ce);
+		if (strcmp(ce->name, it->string) || !stage)
+			break;
+		stages[stage - 1].mode = ce->ce_mode;
+		hashcpy(stages[stage - 1].sha1, ce->sha1);
+	}
+
+	if (s->null_termination)
+		path_current = it->string;
+	else
+		path_current = quote_path(it->string, s->prefix, &buf_current);
+
+	wt_porcelain_v2_print_tracked_entry(
+		s->fp, x_us, y_them, submodule,
+		stages[0].mode, /* stage 1 */
+		stages[1].mode, /* stage 2 */
+		stages[2].mode, /* stage 3 */
+		d->porcelain_v2.mode_worktree,
+		stages[0].sha1, /* stage 1 */
+		stages[1].sha1, /* stage 2 */
+		stages[2].sha1, /* stage 3 */
+		0,
+		path_current,
+		NULL,
+		s->null_termination);
+
+	strbuf_release(&buf_current);
+}
+
+/*
+ * Print porcelain V2 status info for untracked and ignored entries.
+ */
+static void wt_porcelain_v2_print_other(
+	struct string_list_item *it,
+	struct wt_status *s,
+	const char *sign)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *path;
+	char eol;
+
+	if (s->null_termination) {
+		path = it->string;
+		eol = '\0';
+	} else {
+		path = quote_path(it->string, s->prefix, &buf);
+		eol = '\n';
+	}
+
+	fprintf(s->fp, "%s %s%c", sign, path, eol);
+
+	strbuf_release(&buf);
+}
+
+/* Print porcelain V2 status.
+ *
+ * [<v2_branch>]
+ * [<v2_tracked_items>]*
+ * [<v2_untracked_items>]*
+ * [<v2_ignored_items>]*
+ *
+ */
+void wt_porcelain_v2_print(struct wt_status *s)
+{
+	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)
+			wt_porcelain_v2_print_unmerged_entry(it, s);
+		else
+			wt_porcelain_v2_print_normal_entry(it, s);
+	}
+
+	for (i = 0; i < s->untracked.nr; i++) {
+		struct string_list_item *it;
+
+		it = &(s->untracked.items[i]);
+		wt_porcelain_v2_print_other(it, s, "??");
+	}
+
+	for (i = 0; i < s->ignored.nr; i++) {
+		struct string_list_item *it;
+
+		it = &(s->ignored.items[i]);
+		wt_porcelain_v2_print_other(it, s, "!!");
+	}
+}
diff --git a/wt-status.h b/wt-status.h
index fc80341..c062e30 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -39,6 +39,17 @@ struct wt_status_change_data {
 	int index_status;
 	int stagemask;
 	char *head_path;
+
+	struct
+	{
+		int rename_score;
+		int mode_head;
+		int mode_index;
+		int mode_worktree;
+		unsigned char sha1_head[GIT_SHA1_RAWSZ];
+		unsigned char sha1_index[GIT_SHA1_RAWSZ];
+	} porcelain_v2;
+
 	unsigned dirty_submodule       : 2;
 	unsigned new_submodule_commits : 1;
 };
@@ -48,6 +59,7 @@ struct wt_status_change_data {
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN,
+	STATUS_FORMAT_PORCELAIN_V2,
 
 	STATUS_FORMAT_UNSPECIFIED
  };
@@ -119,6 +131,7 @@ int wt_status_check_bisect(const struct worktree *wt,
 
 void wt_shortstatus_print(struct wt_status *s);
 void wt_porcelain_print(struct wt_status *s);
+void wt_porcelain_v2_print(struct wt_status *s);
 
 __attribute__((format (printf, 3, 4)))
 void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);
-- 
2.8.0.rc4.17.gac42084.dirty

[PATCH v1 4/6] Expanded branch header for Porcelain Status V2

[Jeff Hostetler]

This commit expand porcelain status V2 to print
detailed information about the current branch.
This includes the SHA of the current commit, the
current branch name, the upstream branch name,
and the ahead/behind counts.

This additional information is included when
the --branch argument is given.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/commit.c |  5 ++++
 wt-status.c      | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h      |  2 ++
 3 files changed, 96 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index b5ec9b9..830f688 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -512,6 +512,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	s->fp = fp;
 	s->nowarn = nowarn;
 	s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+	if (!s->is_initial)
+		hashcpy(s->sha_commit, sha1);
 	s->status_format = status_format;
 
 	wt_status_collect(s);
@@ -1397,6 +1399,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	fd = hold_locked_index(&index_lock, 0);
 
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
+	if (!s.is_initial)
+		hashcpy(s.sha_commit, sha1);
+
 	s.ignore_submodule_arg = ignore_submodule_arg;
 	s.status_format = status_format;
 
diff --git a/wt-status.c b/wt-status.c
index 2d4829e..9ba4da5 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1847,6 +1847,92 @@ void wt_porcelain_print(struct wt_status *s)
 	wt_shortstatus_print(s);
 }
 
+/*
+ * Print branch and tracking header for porcelain v2 output.
+ * This is printed when the '--branch' parameter is given.
+ *
+ *    "## branch: <commit> <head>[ <upstream>[ +<ahead> -<behind>]]<eol>"
+ *
+ *              <commit> ::= 40 character SHA1 of the current commit or
+ *                           "(initial)" literal to indicate initialized
+ *                           repo with no commits.
+ *
+ *                <head> ::= <branch_name> the current branch name or
+ *                           "(detached)" literal when detached head.
+ *
+ *            <upstream> ::= the upstream branch name, when set.
+ *
+ *               <ahead> ::= integer ahead value, when upstream set
+ *                           and commit is present.
+ *
+ *              <behind> ::= integer behind value, when upstream set
+ *                           and commit is present.
+ *
+ *
+ * The end-of-line is defined by the -z flag.
+ *
+ *                 <eol> ::= NUL when -z,
+ *                           LF when NOT -z.
+ *
+ */
+static void wt_porcelain_v2_print_tracking(struct wt_status *s)
+{
+	struct branch *branch;
+	const char *base;
+	const char *branch_name;
+	struct wt_status_state state;
+	int ab_info, nr_ahead, nr_behind;
+
+	memset(&state, 0, sizeof(state));
+	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
+
+	fprintf(s->fp, "## branch:");
+	fprintf(s->fp, " %s",
+		(s->is_initial ? "(initial)" : sha1_to_hex(s->sha_commit)));
+
+	if (s->branch) {
+		if (!strcmp(s->branch, "HEAD")) {
+			fprintf(s->fp, " %s", "(detached)");
+			if (state.rebase_in_progress || state.rebase_interactive_in_progress) {
+				branch_name = state.onto;
+			} else if (state.detached_from) {
+				branch_name = state.detached_from;
+			} else {
+				branch_name = "";
+			}
+		} else {
+			branch_name = NULL;
+			skip_prefix(s->branch, "refs/heads/", &branch_name);
+			fprintf(s->fp, " %s", branch_name);
+		}
+
+		/* Lookup stats on the upstream tracking branch, if set. */
+		branch = branch_get(branch_name);
+		base = NULL;
+		ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind, &base) == 0);
+		if (base) {
+			base = shorten_unambiguous_ref(base, 0);
+			fprintf(s->fp, " %s", base);
+			free((char *)base);
+
+			if (ab_info)
+				fprintf(s->fp, " +%d -%d", nr_ahead, nr_behind);
+		}
+	} else {
+		/*
+		 * TODO All of various print routines allow for s->branch to be null.
+		 * TODO When can this happen and what should we report here?
+		 */
+		fprintf(s->fp, " %s", "(unknown)");
+	}
+
+	fprintf(s->fp, "%c", (s->null_termination ? '\0' : '\n'));
+
+	free(state.branch);
+	free(state.onto);
+	free(state.detached_from);
+}
+
 /* Convert various submodule status values into a
  * string of characters in the buffer provided.
  */
@@ -2122,6 +2208,9 @@ void wt_porcelain_v2_print(struct wt_status *s)
 {
 	int i;
 
+	if (s->show_branch)
+		wt_porcelain_v2_print_tracking(s);
+
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		struct string_list_item *it;
diff --git a/wt-status.h b/wt-status.h
index c062e30..168556f 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -88,6 +88,8 @@ struct wt_status {
 	int hints;
 
 	enum wt_status_format status_format;
+	unsigned char sha_commit[GIT_SHA1_RAWSZ]; /* Commit SHA (when not Initial) */
+
 	/* These are computed during processing of the individual sections */
 	int commitable;
 	int workdir_dirty;
-- 
2.8.0.rc4.17.gac42084.dirty

[PATCH v1 5/6] Add porcelain V2 documentation to status manpage

[Jeff Hostetler]

This commit updates the status manpage to include
information about porcelain format V2.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/git-status.txt | 62 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index de97729..01c42c0 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -185,10 +185,10 @@ If -b is used the short-format status is preceded by a line
 
 ## branchname tracking info
 
-Porcelain Format
-~~~~~~~~~~~~~~~~
+Porcelain Format Version 1
+~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-The porcelain format is similar to the short format, but is guaranteed
+Version 1 porcelain format is similar to the short format, but is guaranteed
 not to change in a backwards-incompatible way between Git versions or
 based on user configuration. This makes it ideal for parsing by scripts.
 The description of the short format above also describes the porcelain
@@ -210,6 +210,62 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Porcelain Format Version 2
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+
+If `--branch` is given, a header line showing branch tracking information
+is printed.  This line begins with "### branch: ".  Fields are separated
+by a single space.
+
+    Field                    Meaning
+    --------------------------------------------------------
+    <sha> | (initial)        Current commit
+    <branch> | (detached)    Current branch
+    <upstream>               Upstream branch, if set
+    +<ahead>                 Ahead count, if upstream present
+    -<behind>                Behind count, if upstream present
+    --------------------------------------------------------
+
+A series of lines are then displayed for the tracked entries.
+
+    <xy> <sub> <mA> <mB> <mC> <mD> <shaA> <shaB> <shaC> R<nr> <path>[\t<pathSrc>]
+
+    Field       Meaning
+    --------------------------------------------------------
+    <xy>        The staged and unstaged values described earlier, with
+                unchanged indicated by a "." rather than a space.
+    <sub>       The submodule state. "N" when the entry is not a submodule.
+                "S[C][M][U]" when the entry is a submodule.
+                "C" indicates the submodule commit has changed.
+                "M" indicates the submodule has tracked changes.
+                "U" indicates the submodule has untracked changes.
+    <m*>        The file modes for the entry.
+                For unmerged entries, these are the stage 1, 2, and 3,
+                and the worktree modes.
+                For regular entries, these are the head, index, and
+                worktree modes; the fourth is zero.
+    <sha*>      The SHA1 values for the entry.
+                For unmerged entries, these are the stage 1,2, and 3 values.
+                For regular entries, these are the head and index values;
+                the third entry is zero.
+    R<nr>       The rename percentage score.
+    <path>      The current pathname. It is C-Quoted if necessary.
+    <pathSrc>   The original path. This is only present for staged renames.
+                It is C-Quoted if necessary.
+    --------------------------------------------------------
+
+A series of lines are then displayed for untracked and ignored entries.
+
+    <xx> <path>
+
+Where <xx> is "??" for untracked entries and "!!" for ignored entries.
+
+When the `-z` option is given, a NUL (zero) byte follows each pathname;
+serving as both a separator and line termination. No pathname quoting
+or backslash escaping is performed. All fields are output in the same
+order.
+
 CONFIGURATION
 -------------
 
-- 
2.8.0.rc4.17.gac42084.dirty

[PATCH v1 6/6] Unit tests for V2 porcelain status

[Jeff Hostetler]

This commit contains unit tests to exercise
the V2 porcelain status format.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t7064-wtstatus-pv2.sh | 461 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t7501-commit.sh       |   9 +
 2 files changed, 470 insertions(+)
 create mode 100755 t/t7064-wtstatus-pv2.sh

diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
new file mode 100755
index 0000000..1f4252a
--- /dev/null
+++ b/t/t7064-wtstatus-pv2.sh
@@ -0,0 +1,461 @@
+#!/bin/sh
+
+test_description='git status --porcelain=2
+
+This test exercises porcelain V2 output for git status.'
+
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_tick &&
+	git config --local core.autocrlf false &&
+	echo x >file_x &&
+	echo y >file_y &&
+	echo z >file_z &&
+	mkdir dir1 &&
+	echo a >dir1/file_a &&
+	echo b >dir1/file_b
+'
+
+
+##################################################################
+## Confirm VVP output prior to initial commit.
+##################################################################
+
+test_expect_success pre_initial_commit_0 '
+	printf "## branch: (initial) master\n" >expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? dir1/\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+	printf "?? file_x\n" >>expected &&
+	printf "?? file_y\n" >>expected &&
+	printf "?? file_z\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=normal >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success pre_initial_commit_1 '
+	git add file_x file_y file_z dir1 &&
+	SHA_A=`git hash-object -t blob -- dir1/file_a` &&
+	SHA_B=`git hash-object -t blob -- dir1/file_b` &&
+	SHA_X=`git hash-object -t blob -- file_x` &&
+	SHA_Y=`git hash-object -t blob -- file_y` &&
+	SHA_Z=`git hash-object -t blob -- file_z` &&
+	SHA_ZERO=0000000000000000000000000000000000000000 &&
+	printf "## branch: (initial) master\n" >expected &&
+	printf "A. N 000000 100644 100644 000000 $SHA_ZERO $SHA_A $SHA_ZERO R0 dir1/file_a\n" >>expected &&
+	printf "A. N 000000 100644 100644 000000 $SHA_ZERO $SHA_B $SHA_ZERO R0 dir1/file_b\n" >>expected &&
+	printf "A. N 000000 100644 100644 000000 $SHA_ZERO $SHA_X $SHA_ZERO R0 file_x\n" >>expected &&
+	printf "A. N 000000 100644 100644 000000 $SHA_ZERO $SHA_Y $SHA_ZERO R0 file_y\n" >>expected &&
+	printf "A. N 000000 100644 100644 000000 $SHA_ZERO $SHA_Z $SHA_ZERO R0 file_z\n" >>expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	test_cmp expected actual
+'
+
+
+##################################################################
+## Create first commit. Confirm commit sha in new track header.
+## Then make some changes on top of it.
+##################################################################
+
+test_expect_success initial_commit_0 '
+	git commit -m initial &&
+	H0=`git rev-parse HEAD` &&
+	printf "## branch: $H0 master\n" >expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_1 '
+	echo x >>file_x &&
+	SHA_X1=`git hash-object -t blob -- file_x` &&
+	rm file_z &&
+	H0=`git rev-parse HEAD` &&
+	printf "## branch: $H0 master\n" >expected &&
+	printf ".M N 100644 100644 100644 000000 $SHA_X $SHA_X $SHA_ZERO R0 file_x\n" >>expected &&
+	printf ".D N 100644 100644 000000 000000 $SHA_Z $SHA_Z $SHA_ZERO R0 file_z\n" >>expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_2 '
+	git add file_x &&
+	git rm file_z &&
+	H0=`git rev-parse HEAD` &&
+	printf "## branch: $H0 master\n" >expected &&
+	printf "M. N 100644 100644 100644 000000 $SHA_X $SHA_X1 $SHA_ZERO R0 file_x\n" >>expected &&
+	printf "D. N 100644 000000 000000 000000 $SHA_Z $SHA_ZERO $SHA_ZERO R0 file_z\n" >>expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_3 '
+	git mv file_y renamed_y &&
+	H0=`git rev-parse HEAD` &&
+	printf "## branch: $H0 master\n" >expected &&
+	printf "M. N 100644 100644 100644 000000 $SHA_X $SHA_X1 $SHA_ZERO R0 file_x\n" >>expected &&
+	printf "D. N 100644 000000 000000 000000 $SHA_Z $SHA_ZERO $SHA_ZERO R0 file_z\n" >>expected &&
+	printf "R. N 100644 100644 100644 000000 $SHA_Y $SHA_Y $SHA_ZERO R100 renamed_y\tfile_y\n" >>expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	test_cmp expected actual
+'
+
+
+##################################################################
+## Create second commit.
+##################################################################
+
+test_expect_success second_commit_0 '
+	git commit -m second &&
+	H1=`git rev-parse HEAD` &&
+	printf "## branch: $H1 master\n" >expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	test_cmp expected actual
+'
+
+
+##################################################################
+## Ignore a file
+##################################################################
+
+test_expect_success ignore_file_0 '
+	echo x.ign >.gitignore &&
+	echo "ignore me" >x.ign &&
+	H1=`git rev-parse HEAD` &&
+	printf "## branch: $H1 master\n" >expected &&
+	printf "?? .gitignore\n" >>expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+	printf "!! x.ign\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	rm x.ign &&
+	rm .gitignore &&
+	test_cmp expected actual
+'
+
+
+##################################################################
+## Create some conflicts.
+##################################################################
+
+test_expect_success conflict_AA '
+	git branch AA_A master &&
+	git checkout AA_A &&
+	echo "Branch AA_A" >conflict.txt &&
+	SHA_AA_A=`git hash-object -t blob -- conflict.txt` &&
+	git add conflict.txt &&
+	git commit -m "branch aa_a" &&
+
+	git branch AA_B master &&
+	git checkout AA_B &&
+	echo "Branch AA_B" >conflict.txt &&
+	SHA_AA_B=`git hash-object -t blob -- conflict.txt` &&
+	git add conflict.txt &&
+	git commit -m "branch aa_b" &&
+
+	git branch AA_M AA_B &&
+	git checkout AA_M &&
+	test_must_fail git merge AA_A &&
+
+	HM=`git rev-parse HEAD` &&
+	printf "## branch: $HM AA_M\n" >expected &&
+	printf "AA N 000000 100644 100644 100644 $SHA_ZERO $SHA_AA_B $SHA_AA_A R0 conflict.txt\n" >>expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	git reset --hard &&
+	test_cmp expected actual
+'
+
+
+test_expect_success conflict_UU '
+	git branch UU_ANC master &&
+	git checkout UU_ANC &&
+	echo "Ancestor" >conflict.txt &&
+	SHA_UU_ANC=`git hash-object -t blob -- conflict.txt` &&
+	git add conflict.txt &&
+	git commit -m "UU_ANC" &&
+
+	git branch UU_A UU_ANC &&
+	git checkout UU_A &&
+	echo "Branch UU_A" >conflict.txt &&
+	SHA_UU_A=`git hash-object -t blob -- conflict.txt` &&
+	git add conflict.txt &&
+	git commit -m "branch uu_a" &&
+
+	git branch UU_B UU_ANC &&
+	git checkout UU_B &&
+	echo "Branch UU_B" >conflict.txt &&
+	SHA_UU_B=`git hash-object -t blob -- conflict.txt` &&
+	git add conflict.txt &&
+	git commit -m "branch uu_b" &&
+
+	git branch UU_M UU_B &&
+	git checkout UU_M &&
+	test_must_fail git merge UU_A &&
+
+	HM=`git rev-parse HEAD` &&
+	printf "## branch: $HM UU_M\n" >expected &&
+	printf "UU N 100644 100644 100644 100644 $SHA_UU_ANC $SHA_UU_B $SHA_UU_A R0 conflict.txt\n" >>expected &&
+	printf "?? actual\n" >>expected &&
+	printf "?? expected\n" >>expected &&
+
+	git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+	git reset --hard &&
+	test_cmp expected actual
+'
+
+
+##################################################################
+## Test upstream fields in branch header
+##################################################################
+
+test_expect_success 'upstream_fields_0' '
+	git checkout master &&
+	git clone . sub_repo &&
+	(
+		## Confirm local master tracks remote master.
+		cd sub_repo &&
+		HUF=`git rev-parse HEAD` &&
+		printf "## branch: $HUF master origin/master +0 -0\n" >expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual &&
+
+		## Test ahead/behind.
+		echo xyz >file_xyz &&
+		git add file_xyz &&
+		git commit -m xyz &&
+
+		HUF=`git rev-parse HEAD` &&
+		printf "## branch: $HUF master origin/master +1 -0\n" >expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual &&
+
+		## Test upstream-gone case. Fake this by pointing origin/master at
+		## a non-existing commit.
+		OLD=`git rev-parse origin/master` &&
+		NEW=$SHA_ZERO &&
+		mv .git/packed-refs .git/old-packed-refs &&
+		sed "s/$OLD/$NEW/g" <.git/old-packed-refs >.git/packed-refs &&
+
+		HUF=`git rev-parse HEAD` &&
+		printf "## branch: $HUF master origin/master\n" >expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	) &&
+	rm -rf sub_repo
+'
+
+
+##################################################################
+## Test submodule status flags.
+##################################################################
+
+test_expect_success 'submodule_flags_0' '
+	git checkout master &&
+	git clone . sub_repo &&
+	git clone . super_repo &&
+	(	cd super_repo &&
+		git submodule add ../sub_repo sub1 &&
+
+		## Confirm stage/add of clean submodule.
+		HMOD=`git hash-object -t blob -- .gitmodules` &&
+		HSUP=`git rev-parse HEAD` &&
+		HSUB=$HSUP &&
+		printf "## branch: $HSUP master origin/master +0 -0\n" >expected &&
+		printf "A. N 000000 100644 100644 000000 $SHA_ZERO $HMOD $SHA_ZERO R0 .gitmodules\n" >>expected &&
+		printf "A. S 000000 160000 160000 000000 $SHA_ZERO $HSUB $SHA_ZERO R0 sub1\n" >>expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'submodule_flags_1' '
+	(	cd super_repo &&
+		## Make some untracked dirt in the submodule.
+		(	cd sub1 &&
+			echo "dirt" >file_in_sub
+		) &&
+
+		HMOD=`git hash-object -t blob -- .gitmodules` &&
+		HSUP=`git rev-parse HEAD` &&
+		HSUB=$HSUP &&
+		printf "## branch: $HSUP master origin/master +0 -0\n" >expected &&
+		printf "A. N 000000 100644 100644 000000 $SHA_ZERO $HMOD $SHA_ZERO R0 .gitmodules\n" >>expected &&
+		printf "AM SU 000000 160000 160000 000000 $SHA_ZERO $HSUB $SHA_ZERO R0 sub1\n" >>expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'submodule_flags_2' '
+	(	cd super_repo &&
+		## Make some staged dirt in the submodule.
+		(	cd sub1 &&
+			git add file_in_sub
+		) &&
+
+		HMOD=`git hash-object -t blob -- .gitmodules` &&
+		HSUP=`git rev-parse HEAD` &&
+		HSUB=$HSUP &&
+		printf "## branch: $HSUP master origin/master +0 -0\n" >expected &&
+		printf "A. N 000000 100644 100644 000000 $SHA_ZERO $HMOD $SHA_ZERO R0 .gitmodules\n" >>expected &&
+		printf "AM SM 000000 160000 160000 000000 $SHA_ZERO $HSUB $SHA_ZERO R0 sub1\n" >>expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'submodule_flags_3' '
+	(	cd super_repo &&
+		## Make some staged and unstaged dirt (on the same file) in the submodule.
+		## This does not cause us to get SMU (because the submodule does not report
+		## a "??" line for the unstaged changes).
+		(	cd sub1 &&
+			echo "more dirt" >>file_in_sub
+		) &&
+
+		HMOD=`git hash-object -t blob -- .gitmodules` &&
+		HSUP=`git rev-parse HEAD` &&
+		HSUB=$HSUP &&
+		printf "## branch: $HSUP master origin/master +0 -0\n" >expected &&
+		printf "A. N 000000 100644 100644 000000 $SHA_ZERO $HMOD $SHA_ZERO R0 .gitmodules\n" >>expected &&
+		printf "AM SM 000000 160000 160000 000000 $SHA_ZERO $HSUB $SHA_ZERO R0 sub1\n" >>expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'submodule_flags_4' '
+	(	cd super_repo &&
+		## Make some staged and untracked dirt (on different files) in the submodule.
+		(	cd sub1 &&
+			git add file_in_sub &&
+			echo "dirt" >>another_file_in_sub
+		) &&
+
+		HMOD=`git hash-object -t blob -- .gitmodules` &&
+		HSUP=`git rev-parse HEAD` &&
+		HSUB=$HSUP &&
+		printf "## branch: $HSUP master origin/master +0 -0\n" >expected &&
+		printf "A. N 000000 100644 100644 000000 $SHA_ZERO $HMOD $SHA_ZERO R0 .gitmodules\n" >>expected &&
+		printf "AM SMU 000000 160000 160000 000000 $SHA_ZERO $HSUB $SHA_ZERO R0 sub1\n" >>expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'submodule_flags_5' '
+	(	cd super_repo &&
+		## Make a new commit in the submodule.
+		(	cd sub1 &&
+			git add file_in_sub &&
+			rm -f another_file_in_sub &&
+			git commit -m "new commit"
+		) &&
+
+		HMOD=`git hash-object -t blob -- .gitmodules` &&
+		HSUP=`git rev-parse HEAD` &&
+		HSUB=$HSUP &&
+		printf "## branch: $HSUP master origin/master +0 -0\n" >expected &&
+		printf "A. N 000000 100644 100644 000000 $SHA_ZERO $HMOD $SHA_ZERO R0 .gitmodules\n" >>expected &&
+		printf "AM SC 000000 160000 160000 000000 $SHA_ZERO $HSUB $SHA_ZERO R0 sub1\n" >>expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'submodule_flags_6' '
+	(	cd super_repo &&
+		## Commit the new submodule commit in the super.
+		git add sub1 &&
+		git commit -m "super commit" &&
+
+		HSUP=`git rev-parse HEAD` &&
+		printf "## branch: $HSUP master origin/master +1 -0\n" >expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'submodule_flags_7' '
+	(	cd super_repo &&
+		## Make some untracked dirt in the submodule.
+		(	cd sub1 &&
+			echo "yet more dirt" >>file_in_sub
+		) &&
+
+		HMOD=`git hash-object -t blob -- .gitmodules` &&
+		HSUP=`git rev-parse HEAD` &&
+		HSUB=`(cd sub1 && git rev-parse HEAD)` &&
+		printf "## branch: $HSUP master origin/master +1 -0\n" >expected &&
+		printf ".M SM 160000 160000 160000 000000 $HSUB $HSUB $SHA_ZERO R0 sub1\n" >>expected &&
+		printf "?? actual\n" >>expected &&
+		printf "?? expected\n" >>expected &&
+
+		git status --porcelain=2 --branch --ignored --untracked-files=all >actual &&
+		test_cmp expected actual
+	)
+'
+
+##################################################################
+## The end.
+##################################################################
+
+test_done
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 1a74d75..753bae5 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -77,6 +77,10 @@ test_expect_success '--porcelain=1 fails with nothing to commit' '
 	test_must_fail git commit -m initial --porcelain=1
 '
 
+test_expect_success '--porcelain=2 fails with nothing to commit' '
+	test_must_fail git commit -m initial --porcelain=2
+'
+
 test_expect_success '--long fails with nothing to commit' '
 	test_must_fail git commit -m initial --long
 '
@@ -106,6 +110,11 @@ test_expect_failure '--porcelain=1 with stuff to commit returns ok' '
 	git commit -m next -a --porcelain=1
 '
 
+test_expect_failure '--porcelain=2 with stuff to commit returns ok' '
+	echo bongo bongo bongo >>file &&
+	git commit -m next -a --porcelain=2
+'
+
 test_expect_failure '--porcelain=bogus with stuff to commit returns ok' '
 	echo bongo bongo bongo >>file &&
 	git commit -m next -a --porcelain=bogus
-- 
2.8.0.rc4.17.gac42084.dirty

Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

[Johannes Schindelin]

Hi Jeff,

On Tue, 19 Jul 2016, Jeff Hostetler wrote:

> Update the --porcelain argument to take an optional
> version number.  This will allow us to define new
> porcelain formats in the future.
> 
> This default to 1 and represents the existing porcelain
> format.
> 
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>

Very nice introductory patch.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 1f6dbcd..892d7f7 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -142,14 +142,24 @@ static int show_ignored_in_status, have_option_m;
>  static const char *only_include_assumed;
>  static struct strbuf message = STRBUF_INIT;
>  
> -static enum status_format {
> -	STATUS_FORMAT_NONE = 0,
> -	STATUS_FORMAT_LONG,
> -	STATUS_FORMAT_SHORT,
> -	STATUS_FORMAT_PORCELAIN,
> +static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
>  
> -	STATUS_FORMAT_UNSPECIFIED
> -} status_format = STATUS_FORMAT_UNSPECIFIED;
> +static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
> +{
> +	enum wt_status_format *value = (enum wt_status_format *)opt->value;
> +	if (unset) {
> +		*value = STATUS_FORMAT_UNSPECIFIED;

In Git's source code, we skip the curly braces for one-liners.

> +	} else if (arg) {
> +		int n = strtol(arg, NULL, 10);
> +		if (n == 1)
> +			*value = STATUS_FORMAT_PORCELAIN;
> +		else
> +			die("unsupported porcelain version");
> +	} else {
> +		*value = STATUS_FORMAT_PORCELAIN;

This could be folded into the previous conditional:

	}
	else {
		int n = arg ? strtol(arg, NULL, 10) : 1;
		...

Having said this, ...

> @@ -1336,9 +1347,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  			    N_("show status concisely"), STATUS_FORMAT_SHORT),
>  		OPT_BOOL('b', "branch", &s.show_branch,
>  			 N_("show branch information")),
> -		OPT_SET_INT(0, "porcelain", &status_format,
> -			    N_("machine-readable output"),
> -			    STATUS_FORMAT_PORCELAIN),
> +		{ OPTION_CALLBACK, 0, "porcelain", &status_format,
> +		  N_("version"), N_("machine-readable output"),
> +		  PARSE_OPT_OPTARG, opt_parse_porcelain },

How about using a COUNTUP here instead? We could then set the status
format afterwards, like this:

	if (porcelain == 0)
		status_format = STATUS_FORMAT_UNSPECIFIED;
	else {
		status_format = STATUS_FORMAT_PORCELAIN;
		if (porcelain > 1)
			warning("No porcelain v%d; falling back to v1",
				porcelain);
	}

The rest of the patch looks good to me!

Ciao,
Johannes

Re: [PATCH v1 2/6] Status and checkout unit tests for --porcelain[=<n>]

[Johannes Schindelin]

Hi Jeff,

On Tue, 19 Jul 2016, Jeff Hostetler wrote:

> Simple unit tests to validate the argument parsing.
> 
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>

They are simple alright, but do we really need so many of them? I would
like to keep the ones in t7060, but I do not think that we necessarily
have to add the t7501 ones.

I know I am a bit at odds here with Junio, who frequently prefers more
tests. It's just that I have to run the complete test suite so often and
it does take 30-45 minutes to run here (due to the fact that the test
suite exercises quite a lot of the POSIX emulation layer via shell
scripting).

So do not take my suggestions as the sole basis for deciding how to go
from here.

Ciao,
Johannes

Re: [PATCH v1 5/6] Add porcelain V2 documentation to status manpage

[Jakub Narębski]

W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:

> +Porcelain Format Version 2
> +~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +
> +If `--branch` is given, a header line showing branch tracking information
> +is printed.  This line begins with "### branch: ".  Fields are separated
> +by a single space.
> +
> +    Field                    Meaning
> +    --------------------------------------------------------
> +    <sha> | (initial)        Current commit
> +    <branch> | (detached)    Current branch

I was wondering if all possible combinations are allowed.  It turns out
that for technical implementation reasons it is not possible to have
"(initial) (detached)".

Just something I was wondering about, no need for any change...
-- 
Jakub Narębski

Re: [PATCH v1 6/6] Unit tests for V2 porcelain status

[Jakub Narębski]

W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
> +test_expect_success pre_initial_commit_0 '
> +	printf "## branch: (initial) master\n" >expected &&
> +	printf "?? actual\n" >>expected &&
> +	printf "?? dir1/\n" >>expected &&
> +	printf "?? expected\n" >>expected &&
> +	printf "?? file_x\n" >>expected &&
> +	printf "?? file_y\n" >>expected &&
> +	printf "?? file_z\n" >>expected &&

Why not use heredoc syntax (cat <<\EOF), or prepare a file
with expected output in the testsuite?

-- 
Jakub Narębski

Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

[Jeff Hostetler]

On 07/20/2016 11:08 AM, Johannes Schindelin wrote:
> On Tue, 19 Jul 2016, Jeff Hostetler wrote:
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> +	} else if (arg) {
>> +		int n = strtol(arg, NULL, 10);
>> +		if (n == 1)
>> +			*value = STATUS_FORMAT_PORCELAIN;
>> +		else
>> +			die("unsupported porcelain version");
>> +	} else {
>> +		*value = STATUS_FORMAT_PORCELAIN;
>
> This could be folded into the previous conditional:
>
> 	}
> 	else {
> 		int n = arg ? strtol(arg, NULL, 10) : 1;
> 		...
>

I did it this way because I didn't want to make any assumptions
here on the numeric value of the enum values.  Or rather, I didn't
want to add specific assignments to the enum type.

This also helps make it easier to see my later commit:
         else if (n == 2) *value = STATUS_FORMAT_PORCELAIN_V2;

Also, I didn't want to alter the order of assignments to the global
status_format variable.  That is, if I capture the value of <n> in
a porcelain_version variable and assign that to status_format
afterwards, there is opportunity for mistakes if they type:
         git status --short --porcelain=1 --long
where the status_format variable is assigned 3 times.

> Having said this, ...
>
>> @@ -1336,9 +1347,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>>   			    N_("show status concisely"), STATUS_FORMAT_SHORT),
>>   		OPT_BOOL('b', "branch", &s.show_branch,
>>   			 N_("show branch information")),
>> -		OPT_SET_INT(0, "porcelain", &status_format,
>> -			    N_("machine-readable output"),
>> -			    STATUS_FORMAT_PORCELAIN),
>> +		{ OPTION_CALLBACK, 0, "porcelain", &status_format,
>> +		  N_("version"), N_("machine-readable output"),
>> +		  PARSE_OPT_OPTARG, opt_parse_porcelain },
>
> How about using a COUNTUP here instead? We could then set the status
> format afterwards, like this:
>
> 	if (porcelain == 0)
> 		status_format = STATUS_FORMAT_UNSPECIFIED;
> 	else {
> 		status_format = STATUS_FORMAT_PORCELAIN;
> 		if (porcelain > 1)
> 			warning("No porcelain v%d; falling back to v1",
> 				porcelain);
> 	}
>

Maybe I misread the COUNTUP docs, but it looked like it would
allow "--porcelain --porcelain", but not "--porcelain=2".

Jeff

Re: [PATCH v1 5/6] Add porcelain V2 documentation to status manpage

[Jeff Hostetler]

On 07/20/2016 11:29 AM, Jakub Narębski wrote:
> W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
>
>> +Porcelain Format Version 2
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +
>> +If `--branch` is given, a header line showing branch tracking information
>> +is printed.  This line begins with "### branch: ".  Fields are separated
>> +by a single space.
>> +
>> +    Field                    Meaning
>> +    --------------------------------------------------------
>> +    <sha> | (initial)        Current commit
>> +    <branch> | (detached)    Current branch
>
> I was wondering if all possible combinations are allowed.  It turns out
> that for technical implementation reasons it is not possible to have
> "(initial) (detached)".
>
> Just something I was wondering about, no need for any change...
>

Right. I don't think that combination is possible.  Not sure how
to document that succinctly.

Jeff

Re: [PATCH v1 6/6] Unit tests for V2 porcelain status

[Jeff Hostetler]

On 07/20/2016 11:30 AM, Jakub Narębski wrote:
> W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
>> +test_expect_success pre_initial_commit_0 '
>> +	printf "## branch: (initial) master\n" >expected &&
>> +	printf "?? actual\n" >>expected &&
>> +	printf "?? dir1/\n" >>expected &&
>> +	printf "?? expected\n" >>expected &&
>> +	printf "?? file_x\n" >>expected &&
>> +	printf "?? file_y\n" >>expected &&
>> +	printf "?? file_z\n" >>expected &&
>
> Why not use heredoc syntax (cat <<\EOF), or prepare a file
> with expected output in the testsuite?
>

The tests involving renames needed to embed a tab character
in the output and hiding a tab character in a heredoc seemed
error prone.  So to be consistent I made them all printf-style.

Also, some of the tests include SHAs for the commit and for
file content, so having pre-computed expected output is awkward.
Granted we could hard code the file SHAs, but not the commits.

Jeff

Re: [PATCH v1 2/6] Status and checkout unit tests for --porcelain[=<n>]

[Jeff Hostetler]

On 07/20/2016 11:19 AM, Johannes Schindelin wrote:
> Hi Jeff,
>
> On Tue, 19 Jul 2016, Jeff Hostetler wrote:
>
>> Simple unit tests to validate the argument parsing.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>
> They are simple alright, but do we really need so many of them? I would
> like to keep the ones in t7060, but I do not think that we necessarily
> have to add the t7501 ones.
>
> I know I am a bit at odds here with Junio, who frequently prefers more
> tests. It's just that I have to run the complete test suite so often and
> it does take 30-45 minutes to run here (due to the fact that the test
> suite exercises quite a lot of the POSIX emulation layer via shell
> scripting).
>
> So do not take my suggestions as the sole basis for deciding how to go
> from here.

I'm open to suggestion here.  I mainly wanted to be able to
prove that adding "=1" didn't affect the output and that an invalid
parameter throws.  We could eliminate several of the "more trivial"
ones if that would help.

Jeff

Re: [PATCH v1 5/6] Add porcelain V2 documentation to status manpage

[Jakub Narębski]

On 20 July 2016 at 17:42, Jeff Hostetler <git@jeffhostetler.com> wrote:
> On 07/20/2016 11:29 AM, Jakub Narębski wrote:
>> W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
>>
>>> +Porcelain Format Version 2
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +
>>> +If `--branch` is given, a header line showing branch tracking
>>> information
>>> +is printed.  This line begins with "### branch: ".  Fields are separated
>>> +by a single space.
>>> +
>>> +    Field                    Meaning
>>> +    --------------------------------------------------------
>>> +    <sha> | (initial)        Current commit
>>> +    <branch> | (detached)    Current branch
>>
>> I was wondering if all possible combinations are allowed.  It turns out
>> that for technical implementation reasons it is not possible to have
>> "(initial) (detached)".
>>
>> Just something I was wondering about, no need for any change...
>
> Right. I don't think that combination is possible.  Not sure how
> to document that succinctly.

I don't think it is something we need to document, at least not here.

-- 
Jakub Narębski

Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

[Jeff King]

On Tue, Jul 19, 2016 at 06:10:53PM -0400, Jeff Hostetler wrote:

> +static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
> +{
> +	enum wt_status_format *value = (enum wt_status_format *)opt->value;
> +	if (unset) {
> +		*value = STATUS_FORMAT_UNSPECIFIED;

Nice attention to detail here and below in handling "unset" and "!arg"
cases.  I think should be STATUS_FORMAT_NONE, though, which is what the
old code used to do (since "0" is the usual special value for --no-*
options). It only matters if you do:

  git status --no-porcelain

Right now that will switch to the long format, regardless of your
config. With your path it defaults to any configured value. It's
probably a case that nobody hits ever, but in the absence of a good
reason to do otherwise, I'd stick with the current behavior.

> +	} else if (arg) {
> +		int n = strtol(arg, NULL, 10);
> +		if (n == 1)
> +			*value = STATUS_FORMAT_PORCELAIN;
> +		else
> +			die("unsupported porcelain version");

This silently allows:

  git status --porcelain="1 for the money"

and later:

  git status --porcelain="2 for the show"

Probably not a big deal in practice, but since the list of formats is
constrained, we don't really care about parsing arbitrary numbers.
So:

  if (!strcmp(arg, "1"))
	*value = STATUS_FORMAT_PORCELAIN;

is actually simpler, and more robust.

I also wondered if:

  git status --porcelain=v1

is more self-documenting about the meaning of "1". It's purely
aesthetics, but it somehow looks better to me. Matching that is also
much easier with pure strcmps.

> @@ -1381,6 +1392,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  
>  	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
>  	s.ignore_submodule_arg = ignore_submodule_arg;
> +	s.status_format = status_format;
> +

I wonder what happens if you pass a "wt_status" with a format of "SHORT"
to the long-formatting code.

I think it is ignored completely, as you are just now introducing the
s.status_format field. But I wonder if there is room for further cleanup
in pushing the big switch statements from run_status() and cmd_status()
into wt-status.c.

-Peff

Re: [PATCH v1 2/6] Status and checkout unit tests for --porcelain[=<n>]

[Jeff King]

On Tue, Jul 19, 2016 at 06:10:54PM -0400, Jeff Hostetler wrote:

> +test_expect_failure '--porcelain=bogus with stuff to commit returns ok' '
> +	echo bongo bongo bongo >>file &&
> +	git commit -m next -a --porcelain=bogus
> +'

Hrm. That seems unexpected to me. Shouldn't it complain about
--porcelain=bogus?

-Peff

Re: [PATCH v1 6/6] Unit tests for V2 porcelain status

[Jakub Narębski]

On 20 July 2016 at 17:47, Jeff Hostetler <git@jeffhostetler.com> wrote:
> On 07/20/2016 11:30 AM, Jakub Narębski wrote:
>> W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
>>>
>>> +test_expect_success pre_initial_commit_0 '
>>> +       printf "## branch: (initial) master\n" >expected &&
>>> +       printf "?? actual\n" >>expected &&
>>> +       printf "?? dir1/\n" >>expected &&
>>> +       printf "?? expected\n" >>expected &&
>>> +       printf "?? file_x\n" >>expected &&
>>> +       printf "?? file_y\n" >>expected &&
>>> +       printf "?? file_z\n" >>expected &&
>>
>> Why not use heredoc syntax (cat <<\EOF), or prepare a file
>> with expected output in the testsuite?
>
> The tests involving renames needed to embed a tab character
> in the output and hiding a tab character in a heredoc seemed
> error prone.  So to be consistent I made them all printf-style.

Ah, so that's the case for series of printf. I think in some other
cases the Git testsuite simply uses HT variable for the TAB
character. I guess that "\t" for TAB is available in POSIX and
all shells that Git is run on?

See t3300-funny-names.sh, t3902-quoted.sh, t4213-log-tabexpand.sh

> Also, some of the tests include SHAs for the commit and for
> file content, so having pre-computed expected output is awkward.
> Granted we could hard code the file SHAs, but not the commits.

Right... but heredoc can include variable expansion (or rather
it includes variable expansion by default, and you can prevent
it by quoting end-of-heredoc marker).

-- 
Jakub Narębski

Re: [PATCH v1 2/6] Status and checkout unit tests for --porcelain[=<n>]

[Jeff King]

On Wed, Jul 20, 2016 at 10:00:07AM -0600, Jeff King wrote:

> On Tue, Jul 19, 2016 at 06:10:54PM -0400, Jeff Hostetler wrote:
> 
> > +test_expect_failure '--porcelain=bogus with stuff to commit returns ok' '
> > +	echo bongo bongo bongo >>file &&
> > +	git commit -m next -a --porcelain=bogus
> > +'
> 
> Hrm. That seems unexpected to me. Shouldn't it complain about
> --porcelain=bogus?

Pondering more, did you mean:

  test_expect_success '--porcelain=bogus complains about format' '
	echo bongo bongo bongo >>file &&
	test_must_fail git commit -m next -a --porcelain=bogus
  '

?

expect_failure is for tests which we _want_ to succeed, but do not yet
(so they get annotated in test results appropriately). expect_success is
for an outcome we expect to happen, but which may involve specific steps
returning failure.

The names are kind of confusing in that regard.

I wonder if just "test_expect" would be a better name for
test_expect_success, and an argument or environment variable to trigger
"we know this is currently broken" rather than having a separate
test_expect_failure function. That's clearly outside the scope of your
series, of course.

-Peff

Re: [PATCH v1 4/6] Expanded branch header for Porcelain Status V2

[Jeff King]

On Tue, Jul 19, 2016 at 06:10:56PM -0400, Jeff Hostetler wrote:

> +	} else {
> +		/*
> +		 * TODO All of various print routines allow for s->branch to be null.
> +		 * TODO When can this happen and what should we report here?
> +		 */
> +		fprintf(s->fp, " %s", "(unknown)");
> +	}

IIRC, it happens when HEAD points to a broken ref. So something like:

  git init
  echo broken >.git/refs/heads/master

would cause resolving HEAD to return NULL.

-Peff

Re: [PATCH v1 0/6] Porcelain Status V2

[Jeff King]

On Tue, Jul 19, 2016 at 06:10:52PM -0400, Jeff Hostetler wrote:

> This patch series adds porcelain V2 format to status.
> This provides detailed information about file changes
> and about the current branch.
> 
> The new output is accessed via:
>     git status --porcelain=2 [--branch]
> 
> An earlier draft of this work was submitted under the
> "Add very verbose porcelain output to status" title.
> This new version addresses the concerns about using
> (abusing) "-vv" and simplifies the some of the formatting.

I reviewed the first two, which look good except for a few minor
comments. I don't have time at the moment to dig carefully into the v2
format itself from the later patches (but from a cursory view they look
OK). I'm flying all day today, so probably won't get to a more thorough
review for a day or two (but if there are other reviewers, please don't
feel you have to wait for my input).

One final bit of food for thought.

Just yesterday somebody asked me about renewing the old idea of using a
more standardized format for machine-readable output, like --json.
That's obviously something that would exist alongside the existing
formats for compatibility, and it doesn't fundamentally change anything
about adding a new format as your patches do (it just becomes yet
another format).

However I wanted to mention it in case you are intrigued by the idea,
and would be interested in skipping porcelain-v2 entirely in favor of
moving to something like json.

A totally reasonable response is "haha no. Please stop moving the
goalposts". I just wanted to throw it out there as an option (and in
case you are interested, to let you think about it before any more work
goes into this direction).

-Peff

Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

[Jeff Hostetler]

On 07/20/2016 11:58 AM, Jeff King wrote:
> On Tue, Jul 19, 2016 at 06:10:53PM -0400, Jeff Hostetler wrote:
>
>> +static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
>> +{
>> +	enum wt_status_format *value = (enum wt_status_format *)opt->value;
>> +	if (unset) {
>> +		*value = STATUS_FORMAT_UNSPECIFIED;
>
> Nice attention to detail here and below in handling "unset" and "!arg"
> cases.  I think should be STATUS_FORMAT_NONE, though, which is what the
> old code used to do (since "0" is the usual special value for --no-*
> options). It only matters if you do:
>
>    git status --no-porcelain
>
> Right now that will switch to the long format, regardless of your
> config. With your path it defaults to any configured value. It's
> probably a case that nobody hits ever, but in the absence of a good
> reason to do otherwise, I'd stick with the current behavior.

Good catch. I'll make it _NONE.

>
>> +	} else if (arg) {
>> +		int n = strtol(arg, NULL, 10);
>> +		if (n == 1)
>> +			*value = STATUS_FORMAT_PORCELAIN;
>> +		else
>> +			die("unsupported porcelain version");
>
> This silently allows:
>
>    git status --porcelain="1 for the money"
>
> and later:
>
>    git status --porcelain="2 for the show"
>
> Probably not a big deal in practice, but since the list of formats is
> constrained, we don't really care about parsing arbitrary numbers.
> So:
>
>    if (!strcmp(arg, "1"))
> 	*value = STATUS_FORMAT_PORCELAIN;
>
> is actually simpler, and more robust.
>
> I also wondered if:
>
>    git status --porcelain=v1
>
> is more self-documenting about the meaning of "1". It's purely
> aesthetics, but it somehow looks better to me. Matching that is also
> much easier with pure strcmps.

I wondered about making it =v1 rather than just =1. It seemed
more aesthetically pleasing, even if it was an extra character
to type.  In a later email in this thread you mention a JSON
option.  If I switched this here to be "=v1" and "=v2", it would
be easy later to have a "=j2" or "=v2j" to do that.


>
>> @@ -1381,6 +1392,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>>
>>   	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
>>   	s.ignore_submodule_arg = ignore_submodule_arg;
>> +	s.status_format = status_format;
>> +
>
> I wonder what happens if you pass a "wt_status" with a format of "SHORT"
> to the long-formatting code.
>
> I think it is ignored completely, as you are just now introducing the
> s.status_format field. But I wonder if there is room for further cleanup
> in pushing the big switch statements from run_status() and cmd_status()
> into wt-status.c.

I'll look into that.

Jeff

Re: [PATCH v1 2/6] Status and checkout unit tests for --porcelain[=<n>]

[Jeff Hostetler]

On 07/20/2016 12:00 PM, Jeff King wrote:
> On Tue, Jul 19, 2016 at 06:10:54PM -0400, Jeff Hostetler wrote:
>
>> +test_expect_failure '--porcelain=bogus with stuff to commit returns ok' '
>> +	echo bongo bongo bongo >>file &&
>> +	git commit -m next -a --porcelain=bogus
>> +'
>
> Hrm. That seems unexpected to me. Shouldn't it complain about
> --porcelain=bogus?

Looks like a cut-n-paste operator error.

Jeff

Re: [PATCH v1 2/6] Status and checkout unit tests for --porcelain[=<n>]

[Jeff Hostetler]

On 07/20/2016 12:03 PM, Jeff King wrote:
> On Wed, Jul 20, 2016 at 10:00:07AM -0600, Jeff King wrote:
>
>> On Tue, Jul 19, 2016 at 06:10:54PM -0400, Jeff Hostetler wrote:
>>
>>> +test_expect_failure '--porcelain=bogus with stuff to commit returns ok' '
>>> +	echo bongo bongo bongo >>file &&
>>> +	git commit -m next -a --porcelain=bogus
>>> +'
>>
>> Hrm. That seems unexpected to me. Shouldn't it complain about
>> --porcelain=bogus?
>
> Pondering more, did you mean:
>
>    test_expect_success '--porcelain=bogus complains about format' '
> 	echo bongo bongo bongo >>file &&
> 	test_must_fail git commit -m next -a --porcelain=bogus
>    '
>
> ?

Yes.  That is what I meant.  And yes, some of the names are confusing.
Thanks.

Jeff

Re: [PATCH v1 4/6] Expanded branch header for Porcelain Status V2

[Jeff Hostetler]

On 07/20/2016 12:06 PM, Jeff King wrote:
> On Tue, Jul 19, 2016 at 06:10:56PM -0400, Jeff Hostetler wrote:
>
>> +	} else {
>> +		/*
>> +		 * TODO All of various print routines allow for s->branch to be null.
>> +		 * TODO When can this happen and what should we report here?
>> +		 */
>> +		fprintf(s->fp, " %s", "(unknown)");
>> +	}
>
> IIRC, it happens when HEAD points to a broken ref. So something like:
>
>    git init
>    echo broken >.git/refs/heads/master
>
> would cause resolving HEAD to return NULL.

That worked and I see "(unknown)".

This is a bit of a nit, but is there a value we'd like
to see there, such as "(unknown)" or "(broken)" or "(missing)"
in that case?  (And make it clear that this is a different
case from "(detached)".)

I'm thinking it would be nicer to always have a field
there for parsing.

Jeff

Re: [PATCH v1 0/6] Porcelain Status V2

[Jeff Hostetler]

On 07/20/2016 12:15 PM, Jeff King wrote:
> One final bit of food for thought.
>
> Just yesterday somebody asked me about renewing the old idea of using a
> more standardized format for machine-readable output, like --json.
> That's obviously something that would exist alongside the existing
> formats for compatibility, and it doesn't fundamentally change anything
> about adding a new format as your patches do (it just becomes yet
> another format).
>
> However I wanted to mention it in case you are intrigued by the idea,
> and would be interested in skipping porcelain-v2 entirely in favor of
> moving to something like json.
>
> A totally reasonable response is "haha no. Please stop moving the
> goalposts". I just wanted to throw it out there as an option (and in
> case you are interested, to let you think about it before any more work
> goes into this direction).

haha no.... :-)

Short term, I'd rather nail down what I have now (both content-wise
and format-wise) and see how we like it.  And have a follow-up task
to look at the --state header we spoke of earlier.  And save the JSON
version as an independent task for later.

I understand the motivation for a JSON option (and have thought
about it before) but I think it ought to be kept separate.
At a higher-level, it seems like a JSON option would be an
opportunity to start a project-wide conversation about formats,
consistency, plumbing, and etc.  A top-down conversation if you
will about which commands will/won't get enhanced, legacy cruft
that would not need to be converted, JSON style and naming and
consistency issues, current best practices in the node/whatever
community, and etc.  I could be wrong, but this feels like a
top-down feature conversation in a wider audience.

Jeff

Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

[Junio C Hamano]

Jeff Hostetler <jeffhost@microsoft.com> writes:

> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index b0a294d..0791573 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -104,7 +104,7 @@ OPTIONS
>  --branch::
>  	Show the branch and tracking info even in short-format.
>  
> ---porcelain::
> +--porcelain[=<version>]::
>  	When doing a dry-run, give the output in a porcelain-ready
>  	format. See linkgit:git-status[1] for details. Implies
>  	`--dry-run`.

I suspect that it would be easier to implement to allow "commit --porcelain",
but I am not sure if it truly makes sense.  Not a new problem, though.

> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index e1e8f57..de97729 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -32,11 +32,14 @@ OPTIONS
>  --branch::
>  	Show the branch and tracking info even in short-format.
>  
> ---porcelain::
> +--porcelain[=<version>]::
>  	Give the output in an easy-to-parse format for scripts.
>  	This is similar to the short output, but will remain stable
>  	across Git versions and regardless of user configuration. See
>  	below for details.
> ++
> +The version parameter is used to specify the format version.
> +This is optional and defaults to the original version '1' format.

I agree with Peff that "v1" would be easier to read, and also his
comment on parsing using strcmp() to require exact matches and
resetting to NONE, not UNSPECIFIED.

Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Junio C Hamano]

Jeff Hostetler <jeffhost@microsoft.com> writes:

Just to avoid later headaches...  please look at your commit titles
and imagine how they will look when listed among 400+ other changes
when they are included in a future release in "git shortlog" output.

> Subject: Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

Subject: status: per-file output for --porcelain=v2

or something like that, perhaps?

> This commit sets up version 2 porcelain status and
> defines the format of detail lines.  This includes
> the usual XY and pathname fields.  It adds the various
> file modes and SHAs and the rename score.  For regular
> entries these values reflect the head, index and
> worktree. For unmerged entries these values reflect
> the stage 1, 2, and 3 values.

Also, we usually do not say "This commit does this and that".

See Documentation/SubmittingPatches for more details regarding the
above two points (and more).

Re: [PATCH v1 4/6] Expanded branch header for Porcelain Status V2

[Jeff King]

On Wed, Jul 20, 2016 at 02:20:24PM -0400, Jeff Hostetler wrote:

> > IIRC, it happens when HEAD points to a broken ref. So something like:
> > 
> >    git init
> >    echo broken >.git/refs/heads/master
> > 
> > would cause resolving HEAD to return NULL.
> 
> That worked and I see "(unknown)".
> 
> This is a bit of a nit, but is there a value we'd like
> to see there, such as "(unknown)" or "(broken)" or "(missing)"
> in that case?  (And make it clear that this is a different
> case from "(detached)".)
> 
> I'm thinking it would be nicer to always have a field
> there for parsing.

My gut feeling is to err on the side of being vague, like "unknown".
This is something that _shouldn't_ ever happen, and if it does, it could
be a broken on-disk ref, a transient syscall error, or some other
weirdness. I don't think we need to get too specific in this context
(we'll likely have said something else useful on stderr already, I
think).

-Peff

Re: [PATCH v1 0/6] Porcelain Status V2

[Jeff King]

On Wed, Jul 20, 2016 at 03:27:45PM -0400, Jeff Hostetler wrote:

> > A totally reasonable response is "haha no. Please stop moving the
> > goalposts". I just wanted to throw it out there as an option (and in
> > case you are interested, to let you think about it before any more work
> > goes into this direction).
> 
> haha no.... :-)
> 
> Short term, I'd rather nail down what I have now (both content-wise
> and format-wise) and see how we like it.  And have a follow-up task
> to look at the --state header we spoke of earlier.  And save the JSON
> version as an independent task for later.
> 
> I understand the motivation for a JSON option (and have thought
> about it before) but I think it ought to be kept separate.
> At a higher-level, it seems like a JSON option would be an
> opportunity to start a project-wide conversation about formats,
> consistency, plumbing, and etc.  A top-down conversation if you
> will about which commands will/won't get enhanced, legacy cruft
> that would not need to be converted, JSON style and naming and
> consistency issues, current best practices in the node/whatever
> community, and etc.  I could be wrong, but this feels like a
> top-down feature conversation in a wider audience.

I agree with everything you've said here.

If we add JSON, we'd want to do it everywhere: lists of commits, lists
of refs, status output, etc. I mentioned that somebody had asked me
about it recently; they are working on a git client and finding that
libgit2 is not serving their needs well, so they'd like to shell out to
git more, and wanted to have a standard way to get the data back in.

-Peff

Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Junio C Hamano]

Jeff Hostetler <jeffhost@microsoft.com> writes:

> diff --git a/wt-status.c b/wt-status.c
> index c19b52c..2d4829e 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -406,6 +406,89 @@ static void wt_status_print_change_data(struct wt_status *s,
>  	strbuf_release(&twobuf);
>  }
>  
> +
> +/* Copy info for both sides of a head-vs-index change
> + * into the Porcelain V2 data.
> + */

	/* 
	 * Please don't use unbalanced/asymmetric comment.
         * Our multi-line comment blocks look more like
         * this.
         */

> +static void porcelain_v2_updated_entry(
> +	struct wt_status_change_data *d,
> +	struct diff_filepair *p)
> +{
> +	switch (p->status) {
> +	case DIFF_STATUS_ADDED:
> +		d->porcelain_v2.mode_index = p->two->mode;
> +		hashcpy(d->porcelain_v2.sha1_index, p->two->sha1);
> +		break;
> +
> +	case DIFF_STATUS_DELETED:
> +		d->porcelain_v2.mode_head = p->one->mode;
> +		hashcpy(d->porcelain_v2.sha1_head, p->one->sha1);
> +		break;
> +
> +	case DIFF_STATUS_RENAMED:
> +		d->porcelain_v2.rename_score = p->score * 100 / MAX_SCORE;
> +	case DIFF_STATUS_COPIED:
> +	case DIFF_STATUS_MODIFIED:
> +	case DIFF_STATUS_TYPE_CHANGED:
> +	case DIFF_STATUS_UNMERGED:
> +		d->porcelain_v2.mode_head = p->one->mode;
> +		d->porcelain_v2.mode_index = p->two->mode;
> +		hashcpy(d->porcelain_v2.sha1_head, p->one->sha1);
> +		hashcpy(d->porcelain_v2.sha1_index, p->two->sha1);
> +		break;
> +
> +	case DIFF_STATUS_UNKNOWN:
> +		/* This should never happen. */
> +		break;
> +	}

You may want to have

	die("BUG: status unknown???");

if you know "This should never happen.".

The code seems to assume that d->porcelain_v2.* fields are
initialized earlier in the callchain to reasonable values
(e.g. STATUS_ADDED case does not clear .mode_head to "missing"); I
am not sure if that is easier to read or fill in all the values
explicitly in this function.

> +}
> +
> +/* Copy info for both sides of an index-vs-worktree change
> + * into the very verbose porcelain data.
> + */
> +static void porcelain_v2_changed_entry(
> +	struct wt_status_change_data *d,
> +	const struct diff_filepair *p)
> +{
> +	switch (p->status) {
> +	case DIFF_STATUS_ADDED:
> +		d->porcelain_v2.mode_worktree = p->two->mode;
> +		/* don't bother with worktree sha, since it is almost always zero. */
> +		break;
> +
> +	case DIFF_STATUS_DELETED:
> +		d->porcelain_v2.mode_index = p->one->mode;
> +		hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
> +		break;
> +
> +	case DIFF_STATUS_COPIED:
> +	case DIFF_STATUS_MODIFIED:
> +	case DIFF_STATUS_RENAMED:

Can RENAMED or COPIED happen here?

> +	case DIFF_STATUS_TYPE_CHANGED:
> +		d->porcelain_v2.mode_index = p->one->mode;
> +		d->porcelain_v2.mode_worktree = p->two->mode;
> +		hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
> +		/* don't bother with worktree sha, since it is almost always zero. */
> +		break;
> +
> +	case DIFF_STATUS_UNKNOWN:
> +		/* This should never happen. */
> +		break;
> +
> +	case DIFF_STATUS_UNMERGED:
> +		/* This should never happen. */
> +		break;
> +	}
> +}

Other than that, the same comments as the ones for _updated_ apply
to this _changed_ function.

> +static void porcelain_v2_added_initial_entry(
> +	struct wt_status_change_data *d,
> +	const struct cache_entry *ce)
> +{
> +	d->porcelain_v2.mode_index = ce->ce_mode;
> +	hashcpy(d->porcelain_v2.sha1_index, ce->sha1);
> +}
> +
>  static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
>  					 struct diff_options *options,
>  					 void *data)
> @@ -433,6 +516,9 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
>  		d->dirty_submodule = p->two->dirty_submodule;
>  		if (S_ISGITLINK(p->two->mode))
>  			d->new_submodule_commits = !!hashcmp(p->one->sha1, p->two->sha1);
> +
> +		if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
> +			porcelain_v2_changed_entry(d, p);
>  	}
>  }
>  
> @@ -486,6 +572,9 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
>  			d->stagemask = unmerged_mask(p->two->path);
>  			break;
>  		}
> +
> +		if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
> +			porcelain_v2_updated_entry(d, p);
>  	}
>  }
> @@ -565,8 +654,12 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
>  			d->index_status = DIFF_STATUS_UNMERGED;
>  			d->stagemask |= (1 << (ce_stage(ce) - 1));
>  		}
> -		else
> +		else {
>  			d->index_status = DIFF_STATUS_ADDED;
> +
> +			if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
> +				porcelain_v2_added_initial_entry(d, ce);
> +		}
>  	}
>  }

The code would be simpler and we'll catch bugs more uniformly if
"collect" phase is made oblivious to s->status_format, I would
suspect, by making unconditional call to porcelain_v2_*_entry()
functions from all of these "collect" functions.

> +/* Convert various submodule status values into a
> + * string of characters in the buffer provided.
> + */
> +static void wt_porcelain_v2_submodule_state(
> +	struct wt_status_change_data *d,
> +	char sub[5])
> +{
> +	int k = 0;
> +
> +	if (S_ISGITLINK(d->porcelain_v2.mode_head) ||
> +		S_ISGITLINK(d->porcelain_v2.mode_index) ||
> +		S_ISGITLINK(d->porcelain_v2.mode_worktree)) {
> +		/* We have a submodule */
> +		sub[k++] = 'S';
> +
> +		/* Sub-flags for each type of dirt */
> +		if (d->new_submodule_commits)
> +			sub[k++] = 'C';
> +		if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
> +			sub[k++] = 'M';
> +		if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
> +			sub[k++] = 'U';
> +	} else {
> +		/* Not a submodule */
> +		sub[k++] = 'N';
> +	}
> +
> +	sub[k] = 0;
> +}

The above seems to do "up to 5"; if these come early in the output,
it may instead be easier to read if "fixed 5 columns, many of them
may be SP, and some may be C or M or U", which would align on fixed
column terminal people use for coding.

> +/* Various fix-up steps before we start printing an item.
> + */
> +static void wt_porcelain_v2_fix_up_status(
> +	struct string_list_item *it,
> +	struct wt_status *s)
> +{
> +	struct wt_status_change_data *d = it->util;
> +
> +	if (!d->index_status) {
> +		if (d->worktree_status == DIFF_STATUS_MODIFIED ||
> +			d->worktree_status == DIFF_STATUS_DELETED) {
> +			/* X=' ' Y=[MD]
> +			 * The item did not change in head-vs-index scan so the head
> +			 * column was never set. (The index column was set during the
> +			 * index-vs-worktree scan.)
> +			 * Force set the head column to make the output complete.
> +			 */
> +			d->porcelain_v2.mode_head = d->porcelain_v2.mode_index;
> +			hashcpy(d->porcelain_v2.sha1_head, d->porcelain_v2.sha1_index);
> +		}

Can there be "else" clause for this inner "if"?  When
d->index_status is not set, but worktree_status is not one of these
two, what does it indicate?  The same for the next one.

> +	}
> +
> +	if (!d->worktree_status) {
> +		if (d->index_status == DIFF_STATUS_MODIFIED ||
> +			d->index_status == DIFF_STATUS_ADDED ||
> +			d->index_status == DIFF_STATUS_RENAMED ||
> +			d->index_status == DIFF_STATUS_COPIED) {
> +			/* X=[MARC] Y=' '
> +			 * The item did not changed in the index-vs-worktree scan so

s/changed/change/

> +			 * the worktree column was never set.
> +			 * Force set the worktree mode to make the output complete.
> +			 */
> +			d->porcelain_v2.mode_worktree = d->porcelain_v2.mode_index;
> +		}
> +	}
> +}
> +
> +/*
> + * Define a single format for tracked entries. This includes:
> + * normal changes, rename changes, and unmerged changes.
> + *
> + * The meanings of modes_[abcd] and sha_[abc] depends on the
> + * change type, but are always present.
> + *
> + * Path(s) are C-Quoted if necessary. Current path is ALWAYS
> + * first. The rename source path is only present when necessary.
> + * A single TAB separates them (because paths can contain spaces
> + * and C-Quoting converts actual tabs in pathnames to a C escape
> + * sequence).
> + */
> +static void wt_porcelain_v2_print_tracked_entry(
> +	FILE *fp,
> +	char x_staged,
> +	char y_unstaged,
> +	const char *submodule,
> +	int mode_a,
> +	int mode_b,
> +	int mode_c,
> +	int mode_d,
> +	const unsigned char sha_a[GIT_SHA1_RAWSZ],
> +	const unsigned char sha_b[GIT_SHA1_RAWSZ],
> +	const unsigned char sha_c[GIT_SHA1_RAWSZ],

x, y, a, b, c, d are all mystery.  When we see a/b/c, they typically
mean stage#1, stage#2 and stage#3, but that does not seem to be it.
When we see a/b, they typically mean old and new, but that does not
seem to be the case here, either.

If you are saying in the above comment, which curiously is formatted
correctly unlike many other multi-line comments we see in this patch
;-), that the meaning of these are not fixed but the function is to
always show 2 things, then four things in the fixed order without
knowing what they mean, it would be easier to understand if you made
them into an array, i.e.e.g. instead of having x_staged and
y_unstaged, have "char prefix[2]" or something that tells the reader
that whatever meaning these have, these come early in the output.

> +	int rename_score,
> +	const char *path_current,
> +	const char *path_rename_src,
> +	int null_termination)
> +{
> +	char sep_char = null_termination ? '\0' : '\t';
> +	char eol_char = null_termination ? '\0' : '\n';

Mental note (i.e. shouldn't be done as part of this patch, but is
something we should fix, probably as a preparatory clean-up patch
before the series): these should be spelled nul_termination, as the
name of the '\0' character is NUL, not NULL.

> +	if (path_rename_src)
> +		fprintf(fp, "%c%c %s %06o %06o %06o %06o %s %s %s R%d %s%c%s%c",
> +				x_staged, y_unstaged, submodule,
> +				mode_a, mode_b, mode_c, mode_d,
> +				sha1_to_hex(sha_a), sha1_to_hex(sha_b), sha1_to_hex(sha_c),
> +				rename_score,
> +				path_current, sep_char, path_rename_src,
> +				eol_char);
> +	else
> +		fprintf(fp, "%c%c %s %06o %06o %06o %06o %s %s %s R%d %s%c",
> +				x_staged, y_unstaged, submodule,
> +				mode_a, mode_b, mode_c, mode_d,
> +				sha1_to_hex(sha_a), sha1_to_hex(sha_b), sha1_to_hex(sha_c),
> +				rename_score,
> +				path_current,
> +				eol_char);
> +}
> +
> +/*
> + * Print porcelain V2 info for normal tracked entries.
> + */
> +static void wt_porcelain_v2_print_normal_entry(
> +	struct string_list_item *it,
> +	struct wt_status *s)
> +{
> +	static const unsigned char sha_zero[GIT_SHA1_RAWSZ] = {0};
> +	struct wt_status_change_data *d = it->util;
> +	struct strbuf buf_current = STRBUF_INIT;
> +	struct strbuf buf_rename_src = STRBUF_INIT;
> +	const char *path_current = NULL;
> +	const char *path_rename_src = NULL;
> +	char x_staged, y_unstaged;
> +	char submodule[5];
> +
> +	wt_porcelain_v2_fix_up_status(it, s);
> +	x_staged = d->index_status ? d->index_status : '.';
> +	y_unstaged = d->worktree_status ? d->worktree_status : '.';

I see a curious and misleading use of the word "staged" around
here.  If these two are index-status and worktree-status (with
stand-in letter '.' when they are not available), why not call them
index-status and worktree-status instead?

> +/*
> + * Print very verbose porcelain status info for unmerged entries.
> + */
> +static void wt_porcelain_v2_print_unmerged_entry(
> +	struct string_list_item *it,
> +	struct wt_status *s)
> +{
> +	struct wt_status_change_data *d = it->util;
> +	const struct cache_entry *ce;
> +	struct strbuf buf_current = STRBUF_INIT;
> +	const char *path_current = NULL;
> +	int pos, stage;
> +	struct {
> +		int mode;
> +		unsigned char sha1[GIT_SHA1_RAWSZ];
> +	} stages[3];
> +	char x_us = 'U', y_them = 'U';

I really do not want to see these X_ and Y_ prefixes; they do not
convey any meaning; if X always corresponds to ours while Y always
corresponds to theirs, then they are even redundant.

> +	char submodule[5];
> +
> +	switch (d->stagemask) {
> +	case 1: x_us = 'D'; y_them = 'D'; break; /* both deleted */
> +	case 2: x_us = 'A'; y_them = 'U'; break; /* added by us */
> +	case 3: x_us = 'U'; y_them = 'D'; break; /* deleted by them */
> +	case 4: x_us = 'U'; y_them = 'A'; break; /* added by them */
> +	case 5: x_us = 'D'; y_them = 'U'; break; /* deleted by us */
> +	case 6: x_us = 'A'; y_them = 'A'; break; /* both added */
> +	case 7: x_us = 'U'; y_them = 'U'; break; /* both modified */
> +	}
> +
> +	wt_porcelain_v2_submodule_state(d, submodule);
> +
> +	/*
> +	 * Disregard the V2 {mode,sha} values for head and index
> +	 * that we computed from the diffs and lookup the actual
> +	 * stage data.
> +	 */
> +	memset(stages, 0, sizeof(stages));
> +	pos = cache_name_pos(it->string, strlen(it->string));
> +	assert(pos < 0);
> +	pos = -pos-1;
> +	while (pos < active_nr) {
> +		ce = active_cache[pos++];
> +		stage = ce_stage(ce);
> +		if (strcmp(ce->name, it->string) || !stage)
> +			break;
> +		stages[stage - 1].mode = ce->ce_mode;
> +		hashcpy(stages[stage - 1].sha1, ce->sha1);
> +	}

You did assert(pos < 0) to make sure that the path the caller told
you is unmerged indeed is unmerged, which is a very good check.  In
the same spirit, you would want to make sure that you got at least
one result from the above loop, to diagnose a bug where the path did
not even exist at all in the index.

Also, it is possible to have more than one stage#1 entries in the
index (stage#2 can only be one; I think stage#3 can theoretically
have more than one, but in practice, even merge-octopus does not do
that).  The above loop gives us the "last one wins" semantics to
make sure what you grab from the index always fit in stages[] array,
which is not wrong per-se, but that design decision may want to be
documented in a comment nearby for people to work on enhancing it in
the future.

Re: [PATCH v1 5/6] Add porcelain V2 documentation to status manpage

[Junio C Hamano]

Jeff Hostetler <jeffhost@microsoft.com> writes:

> +A series of lines are then displayed for the tracked entries.
> +
> +    <xy> <sub> <mA> <mB> <mC> <mD> <shaA> <shaB> <shaC> R<nr> <path>[\t<pathSrc>]
> +
> +    Field       Meaning
> +    --------------------------------------------------------
> +    <xy>        The staged and unstaged values described earlier, with
> +                unchanged indicated by a "." rather than a space.

Ahh, this is where these mysterious xy came from.  You just needed
two random consecutive letters, and they could have been ab, ij, or
jk.  I don't like any of them ;-)

Also I have trouble with the "staged and unstaged" here, especially
the latter, as the word implies the user did "git rm --cached" on
the path earlier, which is not the case.  You are saying what is in
the index and what is in the working tree.

Perhaps using iw instead of xy to make them in sync with the way
"git diff --mnemonic-prefix" denotes the contents in the index and
in the working tree?  Together with s/staged/in the index/ and
s/unstaged/in the working tree/, the result would become more
consistent with the rest of the system, I suspect.

> +    <m*>        The file modes for the entry.
> +                For unmerged entries, these are the stage 1, 2, and 3,
> +                and the worktree modes.
> +                For regular entries, these are the head, index, and
> +                worktree modes; the fourth is zero.
> +    <sha*>      The SHA1 values for the entry.
> +                For unmerged entries, these are the stage 1,2, and 3 values.
> +                For regular entries, these are the head and index values;
> +                the third entry is zero.

To future-proof, we should use "object name" for what you call "SHA1
value" here, I would think.

> +    R<nr>       The rename percentage score.

s/percentage score/score/.  Do you differentiate between renames and
copies?

> +    <path>      The current pathname. It is C-Quoted if necessary.
> +    <pathSrc>   The original path. This is only present for staged renames.
> +                It is C-Quoted if necessary.

Seeing "if necessary" makes me wonder if we want to define when it
is "necessary".

Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Johannes Schindelin]

Hi,

On Wed, 20 Jul 2016, Junio C Hamano wrote:

> Jeff Hostetler <jeffhost@microsoft.com> writes:
> 
> Just to avoid later headaches...  please look at your commit titles
> and imagine how they will look when listed among 400+ other changes
> when they are included in a future release in "git shortlog" output.
> 
> > Subject: Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2
> 
> Subject: status: per-file output for --porcelain=v2
> 
> or something like that, perhaps?
> 
> > This commit sets up version 2 porcelain status and
> > defines the format of detail lines.  This includes
> > the usual XY and pathname fields.  It adds the various
> > file modes and SHAs and the rename score.  For regular
> > entries these values reflect the head, index and
> > worktree. For unmerged entries these values reflect
> > the stage 1, 2, and 3 values.
> 
> Also, we usually do not say "This commit does this and that".
> 
> See Documentation/SubmittingPatches for more details regarding the
> above two points (and more).

Maybe something like this:

-- snipsnap --
status: per-file output for --porcelain=2

The output of `git status --porcelain` leaves out many details that,
say, an IDE would need to know about the current status.

Let's introduce version porcelain status v2 that adds the various file
modes and SHAs and the rename score. For regular entries these values
reflect the head, index and worktree. For unmerged entries these values
reflect the stage 1, 2, and 3 values.

Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

[Johannes Schindelin]

Hi Jeff,

On Wed, 20 Jul 2016, Jeff Hostetler wrote:

> On 07/20/2016 11:08 AM, Johannes Schindelin wrote:
> > On Tue, 19 Jul 2016, Jeff Hostetler wrote:
> > > @@ -1336,9 +1347,9 @@ int cmd_status(int argc, const char **argv, const
> > > char *prefix)
> > >     	    N_("show status concisely"), STATUS_FORMAT_SHORT),
> > >     OPT_BOOL('b', "branch", &s.show_branch,
> > >   			 N_("show branch information")),
> > > -		OPT_SET_INT(0, "porcelain", &status_format,
> > > -			    N_("machine-readable output"),
> > > -			    STATUS_FORMAT_PORCELAIN),
> > > +		{ OPTION_CALLBACK, 0, "porcelain", &status_format,
> > > +		  N_("version"), N_("machine-readable output"),
> > > +		  PARSE_OPT_OPTARG, opt_parse_porcelain },
> >
> > How about using a COUNTUP here instead? We could then set the status
> > format afterwards, like this:
> >
> >  if (porcelain == 0)
> >  	status_format = STATUS_FORMAT_UNSPECIFIED;
> >  else {
> >   status_format = STATUS_FORMAT_PORCELAIN;
> >   if (porcelain > 1)
> >    warning("No porcelain v%d; falling back to v1",
> >  			porcelain);
> >  }
> >
> 
> Maybe I misread the COUNTUP docs, but it looked like it would
> allow "--porcelain --porcelain", but not "--porcelain=2".

Whoops, you're right. It is *I* who misread the code (I did not bother
looking for the docs ;-))

Still, I would prefer to avoid that callback. IOW something like

	{ OPTION_INTEGER, 0, "porcelain", &status_format,
	  N_("version"), N_("machine-readable output"),
	  PARSE_OPT_OPTARG, NULL, 1 },

followed by the if () outlined above.

Ciao,
Johannes

Re: [PATCH v1 4/6] Expanded branch header for Porcelain Status V2

[Johannes Schindelin]

Hi Peff & Jeff ;-)

On Wed, 20 Jul 2016, Jeff King wrote:

> On Wed, Jul 20, 2016 at 02:20:24PM -0400, Jeff Hostetler wrote:
> 
> > > IIRC, it happens when HEAD points to a broken ref. So something like:
> > > 
> > >    git init
> > >    echo broken >.git/refs/heads/master
> > > 
> > > would cause resolving HEAD to return NULL.
> > 
> > That worked and I see "(unknown)".
> > 
> > This is a bit of a nit, but is there a value we'd like
> > to see there, such as "(unknown)" or "(broken)" or "(missing)"
> > in that case?  (And make it clear that this is a different
> > case from "(detached)".)
> > 
> > I'm thinking it would be nicer to always have a field
> > there for parsing.
> 
> My gut feeling is to err on the side of being vague, like "unknown".
> This is something that _shouldn't_ ever happen, and if it does, it could
> be a broken on-disk ref, a transient syscall error, or some other
> weirdness. I don't think we need to get too specific in this context
> (we'll likely have said something else useful on stderr already, I
> think).

FWIW I think "unknown" is a nice conservative way to shrug Git's
shoulders.

When we call `git status --porcelain=v2` and read "unknown", we could
always try to find out more using additional low-level tools and/or disk
access: this is such a rare case that it does not *really* matter all that
much.

Ciao,
Dscho

Re: [PATCH v1 6/6] Unit tests for V2 porcelain status

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

Hi,

On Wed, 20 Jul 2016, Jakub Narębski wrote:

> On 20 July 2016 at 17:47, Jeff Hostetler <git@jeffhostetler.com> wrote:
> > On 07/20/2016 11:30 AM, Jakub Narębski wrote:
> >> W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
> >>>
> >>> +test_expect_success pre_initial_commit_0 '
> >>> +       printf "## branch: (initial) master\n" >expected &&
> >>> +       printf "?? actual\n" >>expected &&
> >>> +       printf "?? dir1/\n" >>expected &&
> >>> +       printf "?? expected\n" >>expected &&
> >>> +       printf "?? file_x\n" >>expected &&
> >>> +       printf "?? file_y\n" >>expected &&
> >>> +       printf "?? file_z\n" >>expected &&
> >>
> >> Why not use heredoc syntax (cat <<\EOF), or prepare a file
> >> with expected output in the testsuite?
> >
> > The tests involving renames needed to embed a tab character
> > in the output and hiding a tab character in a heredoc seemed
> > error prone.  So to be consistent I made them all printf-style.
> 
> Ah, so that's the case for series of printf. I think in some other
> cases the Git testsuite simply uses HT variable for the TAB
> character.

Yeah, it would be more pleasant to read

	echo >expected <<-EOF
	## branch: (initial) master
	?? actual
	?? dir1/
	?? expected
	?? file_x
	?? file_y
	?? file_z
	EOF

And it is also easy to use $HT in there (unless you want to use <<-\EOF).

Actually, even if you want to use \EOF, you can easily use `sed` to
expand, say, "Q" to tabs, such as was done here:

	https://github.com/git/git/blob/v2.9.2/t/t4213-log-tabexpand.sh#L88-L92

Ciao,
Dscho

Re: [PATCH v1 0/6] Porcelain Status V2

[Johannes Schindelin]

Hi Peff & Jeff ;-)

On Wed, 20 Jul 2016, Jeff King wrote:

> On Wed, Jul 20, 2016 at 03:27:45PM -0400, Jeff Hostetler wrote:
> 
> > > A totally reasonable response is "haha no. Please stop moving the
> > > goalposts". I just wanted to throw it out there as an option (and in
> > > case you are interested, to let you think about it before any more work
> > > goes into this direction).
> > 
> > haha no.... :-)
> > 
> > Short term, I'd rather nail down what I have now (both content-wise
> > and format-wise) and see how we like it.  And have a follow-up task
> > to look at the --state header we spoke of earlier.  And save the JSON
> > version as an independent task for later.
> > 
> > I understand the motivation for a JSON option (and have thought
> > about it before) but I think it ought to be kept separate.
> > At a higher-level, it seems like a JSON option would be an
> > opportunity to start a project-wide conversation about formats,
> > consistency, plumbing, and etc.  A top-down conversation if you
> > will about which commands will/won't get enhanced, legacy cruft
> > that would not need to be converted, JSON style and naming and
> > consistency issues, current best practices in the node/whatever
> > community, and etc.  I could be wrong, but this feels like a
> > top-down feature conversation in a wider audience.
> 
> I agree with everything you've said here.
> 
> If we add JSON, we'd want to do it everywhere: lists of commits, lists
> of refs, status output, etc. I mentioned that somebody had asked me
> about it recently; they are working on a git client and finding that
> libgit2 is not serving their needs well, so they'd like to shell out to
> git more, and wanted to have a standard way to get the data back in.

Yeah, if we add JSON, we would want to add it everywhere. But we would
want to add that incrementally; otherwise it would be too humongous a
task.

And I think a good way forward was already suggested elsewhere in this
thread by Jeff: --porcelain=1j, or --porcelain=json (and maybe later
json-v2, json-v3, etc).

Ciao,
Dscho

Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Jeff Hostetler]

On 07/20/2016 05:31 PM, Junio C Hamano wrote:

> The code seems to assume that d->porcelain_v2.* fields are
> initialized earlier in the callchain to reasonable values
> (e.g. STATUS_ADDED case does not clear .mode_head to "missing"); I
> am not sure if that is easier to read or fill in all the values
> explicitly in this function.

The structure is initially zeroed when it allocated.
I'm only setting values for the side the we know are
actually defined (such as in an ADD we know there is
no head values). I'll look at making this more clear.

>> +}
>> +
>> +/* Copy info for both sides of an index-vs-worktree change
>> + * into the very verbose porcelain data.
>> + */
>> +static void porcelain_v2_changed_entry(
>> +	struct wt_status_change_data *d,
>> +	const struct diff_filepair *p)
>> +{
>> +	switch (p->status) {
>> +	case DIFF_STATUS_ADDED:
>> +		d->porcelain_v2.mode_worktree = p->two->mode;
>> +		/* don't bother with worktree sha, since it is almost always zero. */
>> +		break;
>> +
>> +	case DIFF_STATUS_DELETED:
>> +		d->porcelain_v2.mode_index = p->one->mode;
>> +		hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
>> +		break;
>> +
>> +	case DIFF_STATUS_COPIED:
>> +	case DIFF_STATUS_MODIFIED:
>> +	case DIFF_STATUS_RENAMED:
>
> Can RENAMED or COPIED happen here?

If we don't report renamed or copied for unstaged changes, then no.


> The code would be simpler and we'll catch bugs more uniformly if
> "collect" phase is made oblivious to s->status_format, I would
> suspect, by making unconditional call to porcelain_v2_*_entry()
> functions from all of these "collect" functions.

ok. will do.

>
>> +/* Convert various submodule status values into a
>> + * string of characters in the buffer provided.
>> + */
>> +static void wt_porcelain_v2_submodule_state(
>> +	struct wt_status_change_data *d,
>> +	char sub[5])
>> +{
>> +	int k = 0;
>> +
>> +	if (S_ISGITLINK(d->porcelain_v2.mode_head) ||
>> +		S_ISGITLINK(d->porcelain_v2.mode_index) ||
>> +		S_ISGITLINK(d->porcelain_v2.mode_worktree)) {
>> +		/* We have a submodule */
>> +		sub[k++] = 'S';
>> +
>> +		/* Sub-flags for each type of dirt */
>> +		if (d->new_submodule_commits)
>> +			sub[k++] = 'C';
>> +		if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
>> +			sub[k++] = 'M';
>> +		if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
>> +			sub[k++] = 'U';
>> +	} else {
>> +		/* Not a submodule */
>> +		sub[k++] = 'N';
>> +	}
>> +
>> +	sub[k] = 0;
>> +}
>
> The above seems to do "up to 5"; if these come early in the output,
> it may instead be easier to read if "fixed 5 columns, many of them
> may be SP, and some may be C or M or U", which would align on fixed
> column terminal people use for coding.

I debated fixed 4 columns vs a variable width token.
The latter seemed easier to parse, but does look more ugly
on the terminal.  I'll revisit this.  Perhaps make it fixed
4 columns with a '.' instead of SPACE, so both parsing
techniques work.

>
>> +/* Various fix-up steps before we start printing an item.
>> + */
>> +static void wt_porcelain_v2_fix_up_status(
>> +	struct string_list_item *it,
>> +	struct wt_status *s)
>> +{
>> +	struct wt_status_change_data *d = it->util;
>> +
>> +	if (!d->index_status) {
>> +		if (d->worktree_status == DIFF_STATUS_MODIFIED ||
>> +			d->worktree_status == DIFF_STATUS_DELETED) {
>> +			/* X=' ' Y=[MD]
>> +			 * The item did not change in head-vs-index scan so the head
>> +			 * column was never set. (The index column was set during the
>> +			 * index-vs-worktree scan.)
>> +			 * Force set the head column to make the output complete.
>> +			 */
>> +			d->porcelain_v2.mode_head = d->porcelain_v2.mode_index;
>> +			hashcpy(d->porcelain_v2.sha1_head, d->porcelain_v2.sha1_index);
>> +		}
>
> Can there be "else" clause for this inner "if"?  When
> d->index_status is not set, but worktree_status is not one of these
> two, what does it indicate?  The same for the next one.

For a normal entry (not unmerged) when X is ' ', Y can only
be 'M' or 'D'.  The only other value for Y is ' ', but then if
both were ' ' the entry would not be in the list.  So I think
we're OK, but I'll document that here.  And below.


>> +/*
>> + * Define a single format for tracked entries. This includes:
>> + * normal changes, rename changes, and unmerged changes.
>> + *
>> + * The meanings of modes_[abcd] and sha_[abc] depends on the
>> + * change type, but are always present.
>> + *
>> + * Path(s) are C-Quoted if necessary. Current path is ALWAYS
>> + * first. The rename source path is only present when necessary.
>> + * A single TAB separates them (because paths can contain spaces
>> + * and C-Quoting converts actual tabs in pathnames to a C escape
>> + * sequence).
>> + */
>> +static void wt_porcelain_v2_print_tracked_entry(
>> +	FILE *fp,
>> +	char x_staged,
>> +	char y_unstaged,
>> +	const char *submodule,
>> +	int mode_a,
>> +	int mode_b,
>> +	int mode_c,
>> +	int mode_d,
>> +	const unsigned char sha_a[GIT_SHA1_RAWSZ],
>> +	const unsigned char sha_b[GIT_SHA1_RAWSZ],
>> +	const unsigned char sha_c[GIT_SHA1_RAWSZ],
>
> x, y, a, b, c, d are all mystery.  When we see a/b/c, they typically
> mean stage#1, stage#2 and stage#3, but that does not seem to be it.
> When we see a/b, they typically mean old and new, but that does not
> seem to be the case here, either.

I took X and Y from the man-page as the names of first 2 columns
of the existing format.
>
> If you are saying in the above comment, which curiously is formatted
> correctly unlike many other multi-line comments we see in this patch
> ;-),

I read Linus' recent post on the subject mid-way thru this effort. :-)

 >      that the meaning of these are not fixed but the function is to
> always show 2 things, then four things in the fixed order without
> knowing what they mean, it would be easier to understand if you made
> them into an array, i.e.e.g. instead of having x_staged and
> y_unstaged, have "char prefix[2]" or something that tells the reader
> that whatever meaning these have, these come early in the output.

I'll revisit this.  For normal entries (modified and/or renamed) I
show the head, index, worktree modes and the head and index SHAs
and the last mode and SHA fields are zero.  For unmerged entries,
I show the stage 1, 2, 3 and worktree modes and the stage 1,2,3
SHAs.  This gives us the same number of fields for parsing.  An
earlier version had 2 different line formats. Would it be better to
have a different initial character/token (like the ":" vs "::" prefix
in diff-index) ?


>> +	x_staged = d->index_status ? d->index_status : '.';
>> +	y_unstaged = d->worktree_status ? d->worktree_status : '.';
>
> I see a curious and misleading use of the word "staged" around
> here.  If these two are index-status and worktree-status (with
> stand-in letter '.' when they are not available), why not call them
> index-status and worktree-status instead?
> ...
>> +	char x_us = 'U', y_them = 'U';
>
> I really do not want to see these X_ and Y_ prefixes; they do not
> convey any meaning; if X always corresponds to ours while Y always
> corresponds to theirs, then they are even redundant.

I was trying to preserve the X and Y link back to the man-page, but
also give meaning to the fields.  I'll revisit.

>> +	memset(stages, 0, sizeof(stages));
>> +	pos = cache_name_pos(it->string, strlen(it->string));
>> +	assert(pos < 0);
>> +	pos = -pos-1;
>> +	while (pos < active_nr) {
>> +		ce = active_cache[pos++];
>> +		stage = ce_stage(ce);
>> +		if (strcmp(ce->name, it->string) || !stage)
>> +			break;
>> +		stages[stage - 1].mode = ce->ce_mode;
>> +		hashcpy(stages[stage - 1].sha1, ce->sha1);
>> +	}
>
> You did assert(pos < 0) to make sure that the path the caller told
> you is unmerged indeed is unmerged, which is a very good check.  In
> the same spirit, you would want to make sure that you got at least
> one result from the above loop, to diagnose a bug where the path did
> not even exist at all in the index.

Would that be possible for a conflict/unmerged entry?
I can add an assert for that just to be safe.

> Also, it is possible to have more than one stage#1 entries in the
> index (stage#2 can only be one; I think stage#3 can theoretically
> have more than one, but in practice, even merge-octopus does not do
> that).  The above loop gives us the "last one wins" semantics to
> make sure what you grab from the index always fit in stages[] array,
> which is not wrong per-se, but that design decision may want to be
> documented in a comment nearby for people to work on enhancing it in
> the future.

Interesting, more than 1 entry in a single stage. I hadn't considered
that.  I think I pretty much stole the essence of this loop from
elsewhere in the file, so yes I should be consistent with elsewhere
in the code. I'll update the comments.  Good catch!

Thanks for all your time on this!
Jeff

Re: [PATCH v1 4/6] Expanded branch header for Porcelain Status V2

[Jeff Hostetler]

On 07/21/2016 11:46 AM, Johannes Schindelin wrote:
> On Wed, 20 Jul 2016, Jeff King wrote:
>
>> On Wed, Jul 20, 2016 at 02:20:24PM -0400, Jeff Hostetler wrote:
>>
>>>> IIRC, it happens when HEAD points to a broken ref. So something like:
>>>>
>>>>     git init
>>>>     echo broken >.git/refs/heads/master
>>>>
>>>> would cause resolving HEAD to return NULL.
>>>
>>> That worked and I see "(unknown)".
>>>
>>> This is a bit of a nit, but is there a value we'd like
>>> to see there, such as "(unknown)" or "(broken)" or "(missing)"
>>> in that case?  (And make it clear that this is a different
>>> case from "(detached)".)
>>>
>>> I'm thinking it would be nicer to always have a field
>>> there for parsing.
>>
>> My gut feeling is to err on the side of being vague, like "unknown".
>> This is something that _shouldn't_ ever happen, and if it does, it could
>> be a broken on-disk ref, a transient syscall error, or some other
>> weirdness. I don't think we need to get too specific in this context
>> (we'll likely have said something else useful on stderr already, I
>> think).
>
> FWIW I think "unknown" is a nice conservative way to shrug Git's
> shoulders.
>
> When we call `git status --porcelain=v2` and read "unknown", we could
> always try to find out more using additional low-level tools and/or disk
> access: this is such a rare case that it does not *really* matter all that
> much.

yes, this case causes even rev-parse fits.  I'l make it return a
known quantity so that users don't have to deal with stuff like:

$ more .git/HEAD
ref: refs/heads/foo
$ more .git/refs/heads/foo
brokwn
$ git rev-parse HEAD
HEAD
fatal: ambiguous argument 'HEAD': unknown revision or path not in the 
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
$ git rev-parse HEAD --
fatal: bad revision 'HEAD'
$


Jeff

Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

>>> +	case DIFF_STATUS_DELETED:
>>> +		d->porcelain_v2.mode_index = p->one->mode;
>>> +		hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
>>> +		break;
>>> +
>>> +	case DIFF_STATUS_COPIED:
>>> +	case DIFF_STATUS_MODIFIED:
>>> +	case DIFF_STATUS_RENAMED:
>>
>> Can RENAMED or COPIED happen here?
>
> If we don't report renamed or copied for unstaged changes, then no.

The question was "do we report such cases"?

>>> +	if (!d->index_status) {
>>> +		if (d->worktree_status == DIFF_STATUS_MODIFIED ||
>>> +			d->worktree_status == DIFF_STATUS_DELETED) {
>>> +			/* X=' ' Y=[MD]
>>> +			 * The item did not change in head-vs-index scan so the head
>>> +			 * column was never set. (The index column was set during the
>>> +			 * index-vs-worktree scan.)
>>> +			 * Force set the head column to make the output complete.
>>> +			 */
>>> +			d->porcelain_v2.mode_head = d->porcelain_v2.mode_index;
>>> +			hashcpy(d->porcelain_v2.sha1_head, d->porcelain_v2.sha1_index);
>>> +		}
>>
>> Can there be "else" clause for this inner "if"?  When
>> d->index_status is not set, but worktree_status is not one of these
>> two, what does it indicate?  The same for the next one.
>
> For a normal entry (not unmerged) when X is ' ', Y can only
> be 'M' or 'D'.  The only other value for Y is ' ', but then if
> both were ' ' the entry would not be in the list.  So I think
> we're OK, but I'll document that here.  And below.

Good.

>>> +	memset(stages, 0, sizeof(stages));
>>> +	pos = cache_name_pos(it->string, strlen(it->string));
>>> +	assert(pos < 0);
>>> +	pos = -pos-1;
>>> +	while (pos < active_nr) {
>>> +		ce = active_cache[pos++];
>>> +		stage = ce_stage(ce);
>>> +		if (strcmp(ce->name, it->string) || !stage)
>>> +			break;
>>> +		stages[stage - 1].mode = ce->ce_mode;
>>> +		hashcpy(stages[stage - 1].sha1, ce->sha1);
>>> +	}
>>
>> You did assert(pos < 0) to make sure that the path the caller told
>> you is unmerged indeed is unmerged, which is a very good check.  In
>> the same spirit, you would want to make sure that you got at least
>> one result from the above loop, to diagnose a bug where the path did
>> not even exist at all in the index.
>
> Would that be possible for a conflict/unmerged entry?

It is possible to exactly the same degree as it is possible you
would get !(pos < 0) answer from cache_name_pos() here, I would
think.  The assert() you have up above is protecting us from either
a broken caller to this function that gives an it that points at a
merged path (in which case the assert is violated) or a breakage in
cache_name_pos().  Making sure the loop finds at least one unmerged
entry protects us from similar kind of breakage that violates the
expectation this code has from the other parts of the code.


Thanks.