threads / patch / 42883

patch, 6 partsPorcelain Status V2

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

## tl;dr

41 messages between Jul 19, 2016 and Jul 22, 2016. Diffs are folded; open one to read it.

replies: 40people: 6as markdown or json

Jeff Hostetler· Jul 19, 2016, 22:10 UTC · lore

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
Jeff Hostetler· Jul 19, 2016, 22:10 UTC · re: Jeff Hostetler · lore

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

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(-)
Show changes to 4 files +43 −14

Documentation/git-commit.txt, Documentation/git-status.txt, builtin/commit.c, wt-status.h

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
Johannes Schindelin· Jul 20, 2016, 15:08 UTC · re: Jeff Hostetler · lore

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

Hi Jeff,
On Tue, 19 Jul 2016, Jeff Hostetler wrote:
Show 8 quoted lines
> 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.
Show 22 quoted lines
> 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.
Show 8 quoted lines
> +	} 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, ...
Show 10 quoted lines
> @@ -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

Jeff Hostetler· Jul 20, 2016, 15:38 UTC · re: Johannes Schindelin · lore

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

On 07/20/2016 11:08 AM, Johannes Schindelin wrote:
Show 18 quoted lines
> 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.
Show 25 quoted lines
> 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
Johannes Schindelin· Jul 21, 2016, 14:28 UTC · re: Jeff Hostetler · lore

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

Hi Jeff,
On Wed, 20 Jul 2016, Jeff Hostetler wrote:
Show 29 quoted lines
> 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

Jeff King· Jul 20, 2016, 15:58 UTC · re: Jeff Hostetler · lore

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

On Tue, Jul 19, 2016 at 06:10:53PM -0400, Jeff Hostetler wrote:
Show 5 quoted lines
> +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.

Show 6 quoted lines
> +	} 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.

Show 6 quoted lines
> @@ -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
Jeff Hostetler· Jul 20, 2016, 17:26 UTC · re: Jeff King · lore

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

On 07/20/2016 11:58 AM, Jeff King wrote:
Show 19 quoted lines
> 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.
Show 32 quoted lines
>
>> +	} 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.

Show 15 quoted lines
>
>> @@ -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
Junio C Hamano· Jul 20, 2016, 20:46 UTC · re: Jeff Hostetler · lore

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

Jeff Hostetler <jeffhost@microsoft.com> writes:
Show 13 quoted lines
> 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.

Show 17 quoted lines
> 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.

Jeff Hostetler· Jul 19, 2016, 22:10 UTC · re: Jeff Hostetler · lore

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

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(+)
Show changes to 2 files +35 −0

t/t7060-wtstatus.sh, t/t7501-commit.sh

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
Johannes Schindelin· Jul 20, 2016, 15:19 UTC · re: Jeff Hostetler · lore

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

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

Jeff Hostetler· Jul 20, 2016, 15:51 UTC · re: Johannes Schindelin · lore

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

On 07/20/2016 11:19 AM, Johannes Schindelin wrote:
Show 20 quoted lines
> 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
Jeff King· Jul 20, 2016, 16:00 UTC · re: Jeff Hostetler · lore

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

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
Jeff King· Jul 20, 2016, 16:03 UTC · re: Jeff King · lore

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

On Wed, Jul 20, 2016 at 10:00:07AM -0600, Jeff King wrote:
Show 9 quoted lines
> 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
Jeff Hostetler· Jul 20, 2016, 17:31 UTC · re: Jeff King · lore

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

On 07/20/2016 12:03 PM, Jeff King wrote:
Show 20 quoted lines
> 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
Jeff Hostetler· Jul 20, 2016, 17:29 UTC · re: Jeff King · lore

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

On 07/20/2016 12:00 PM, Jeff King wrote:
Show 9 quoted lines
> 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
Jeff Hostetler· Jul 19, 2016, 22:10 UTC · re: Jeff Hostetler · lore

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

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(-)
Show changes to 3 files +419 −1

builtin/commit.c, wt-status.c, wt-status.h

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
Junio C Hamano· Jul 20, 2016, 20:50 UTC · re: Jeff Hostetler · lore

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

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?
Show 7 quoted lines
> 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).

Johannes Schindelin· Jul 21, 2016, 14:19 UTC · re: Junio C Hamano · lore

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

Hi,
On Wed, 20 Jul 2016, Junio C Hamano wrote:
Show 24 quoted lines
> 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.

Junio C Hamano· Jul 20, 2016, 21:31 UTC · re: Jeff Hostetler · lore

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

Jeff Hostetler <jeffhost@microsoft.com> writes:
Show 12 quoted lines
> 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.
         */
Show 31 quoted lines
> +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.

Show 23 quoted lines
> +}
> +
> +/* 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?
Show 16 quoted lines
> +	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.

Show 43 quoted lines
> +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.

Show 29 quoted lines
> +/* 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.

Show 20 quoted lines
> +/* 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.

Show 9 quoted lines
> +	}
> +
> +	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/
Show 33 quoted lines
> +			 * 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.

Show 7 quoted lines
> +	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.

Show 37 quoted lines
> +	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?

Show 17 quoted lines
> +/*
> + * 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.

Show 31 quoted lines
> +	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.

Jeff Hostetler· Jul 21, 2016, 18:58 UTC · re: Junio C Hamano · lore

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

On 07/20/2016 05:31 PM, Junio C Hamano wrote:
Show 5 quoted lines
> 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.

Show 25 quoted lines
>> +}
>> +
>> +/* 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.
Show 35 quoted lines
>
>> +/* 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.

Show 25 quoted lines
>
>> +/* 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.

Show 30 quoted lines
>> +/*
>> + * 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
Show 5 quoted lines
> 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) ?

Show 13 quoted lines
>> +	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.

Show 18 quoted lines
>> +	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.

Show 8 quoted lines
> 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

Junio C Hamano· Jul 22, 2016, 17:01 UTC · re: Jeff Hostetler · lore

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

Jeff Hostetler <git@jeffhostetler.com> writes:
Show 12 quoted lines
>>> +	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"?
Show 21 quoted lines
>>> +	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.
Show 20 quoted lines
>>> +	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.
Jeff Hostetler· Jul 19, 2016, 22:10 UTC · re: Jeff Hostetler · lore

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

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(+)
Show changes to 3 files +96 −0

builtin/commit.c, wt-status.c, wt-status.h

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
Jeff King· Jul 20, 2016, 16:06 UTC · re: Jeff Hostetler · lore

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

On Tue, Jul 19, 2016 at 06:10:56PM -0400, Jeff Hostetler wrote:
Show 7 quoted lines
> +	} 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
Jeff Hostetler· Jul 20, 2016, 18:20 UTC · re: Jeff King · lore

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

On 07/20/2016 12:06 PM, Jeff King wrote:
Show 16 quoted lines
> 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
Jeff King· Jul 20, 2016, 20:54 UTC · re: Jeff Hostetler · lore

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

On Wed, Jul 20, 2016 at 02:20:24PM -0400, Jeff Hostetler wrote:
Show 16 quoted lines
> > 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
Johannes Schindelin· Jul 21, 2016, 15:46 UTC · re: Jeff King · lore

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

Hi Peff & Jeff ;-)
On Wed, 20 Jul 2016, Jeff King wrote:
Show 25 quoted lines
> 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

Jeff Hostetler· Jul 21, 2016, 19:03 UTC · re: Johannes Schindelin · lore

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

On 07/21/2016 11:46 AM, Johannes Schindelin wrote:
Show 35 quoted lines
> 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
Jeff Hostetler· Jul 19, 2016, 22:10 UTC · re: Jeff Hostetler · lore

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

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
Show changes to 2 files +470 −0

t/t7064-wtstatus-pv2.sh, t/t7501-commit.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
Jakub Narębski· Jul 20, 2016, 15:30 UTC · re: Jeff Hostetler · lore

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

W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
Show 8 quoted lines
> +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
Jeff Hostetler· Jul 20, 2016, 15:47 UTC · re: Jakub Narębski · lore

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

On 07/20/2016 11:30 AM, Jakub Narębski wrote:
Show 13 quoted lines
> 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
Jakub Narębski· Jul 20, 2016, 16:01 UTC · re: Jeff Hostetler · lore

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

On 20 July 2016 at 17:47, Jeff Hostetler <git@jeffhostetler.com> wrote:
Show 18 quoted lines
> 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
Johannes Schindelin· Jul 21, 2016, 15:54 UTC · re: Jakub Narębski · lore

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

Hi,
On Wed, 20 Jul 2016, Jakub Narębski wrote:
Show 23 quoted lines
> 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

Jeff Hostetler· Jul 19, 2016, 22:10 UTC · re: Jeff Hostetler · lore

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

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(-)
Show changes to Documentation/git-status.txt +59 −3
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
Jakub Narębski· Jul 20, 2016, 15:29 UTC · re: Jeff Hostetler · lore

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

W dniu 2016-07-20 o 00:10, Jeff Hostetler pisze:
Show 12 quoted lines
> +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
Jeff Hostetler· Jul 20, 2016, 15:42 UTC · re: Jakub Narębski · lore

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

On 07/20/2016 11:29 AM, Jakub Narębski wrote:
Show 21 quoted lines
> 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
Jakub Narębski· Jul 20, 2016, 15:55 UTC · re: Jeff Hostetler · lore

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

On 20 July 2016 at 17:42, Jeff Hostetler <git@jeffhostetler.com> wrote:
Show 25 quoted lines
> 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
Junio C Hamano· Jul 20, 2016, 21:50 UTC · re: Jeff Hostetler · lore

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

Jeff Hostetler <jeffhost@microsoft.com> writes:
Show 8 quoted lines
> +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.

Show 9 quoted lines
> +    <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".

Jeff King· Jul 20, 2016, 16:15 UTC · re: Jeff Hostetler · lore

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

On Tue, Jul 19, 2016 at 06:10:52PM -0400, Jeff Hostetler wrote:
Show 11 quoted lines
> 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
Jeff Hostetler· Jul 20, 2016, 19:27 UTC · re: Jeff King · lore

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

On 07/20/2016 12:15 PM, Jeff King wrote:
Show 17 quoted lines
> 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
Jeff King· Jul 20, 2016, 20:57 UTC · re: Jeff Hostetler · lore

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

On Wed, Jul 20, 2016 at 03:27:45PM -0400, Jeff Hostetler wrote:
Show 22 quoted lines
> > 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
Johannes Schindelin· Jul 21, 2016, 16:02 UTC · re: Jeff King · lore

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

Hi Peff & Jeff ;-)
On Wed, 20 Jul 2016, Jeff King wrote:
Show 32 quoted lines
> 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

← back to recent threads