[PATCH] correct git-status Porcelain Format documentation

[PATCH] correct git-status Porcelain Format documentation

[Jeff King]

From: Zak Johnson <zakj@nox.cx>

The existing documentation implies that "git status --porcelain" has a branch
line while "git status --porcelain -z" does not; in fact, neither includes a
branch line.

Signed-off-by: Zak Johnson <zakj@nox.cx>
---
This was forward to me by the original author, who had trouble posting
to the list. So I'm trying it (I didn't see anything from the taboo list
in the patch, but we'll see...).

The patch itself looks obviously correct and describes the current
behavior. But I have to wonder: the --short format will also not produce
the branch line unless you provide "-b". So why is it that the porcelain
format does not respect "-b", since anybody who asked for it would
obviously be expecting to find and parse it?

Should this bit of documentation be dropped in favor of just making "-b"
work properly?

 Documentation/git-status.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 2883a28..3e12020 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -176,6 +176,8 @@ format, with a few exceptions:
 2. The user's status.relativePaths configuration is not respected; paths
    shown will always be relative to the repository root.
 
+3. There is no branch line.
+
 There is also an alternate -z format recommended for machine parsing. In
 that format, the status field is the same, but some other things
 change.  First, the '\->' is omitted from rename entries and the field
@@ -184,7 +186,7 @@ order is reversed (e.g 'from \-> to' becomes 'to from'). Second, a NUL
 and the terminating newline (but a space still separates the status
 field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
-backslash-escaping is performed. Fourth, there is no branch line.
+backslash-escaping is performed.
 
 CONFIGURATION
 -------------
-- 
1.7.10.1.14.gb97aca1

Re: [PATCH] correct git-status Porcelain Format documentation

[Jeff King]

[+cc Zak, who I forgot in the first email]

On Sun, May 06, 2012 at 09:29:59AM -0400, Jeff King wrote:

> From: Zak Johnson <zakj@nox.cx>
> 
> The existing documentation implies that "git status --porcelain" has a branch
> line while "git status --porcelain -z" does not; in fact, neither includes a
> branch line.
> 
> Signed-off-by: Zak Johnson <zakj@nox.cx>
> ---
> This was forward to me by the original author, who had trouble posting
> to the list. So I'm trying it (I didn't see anything from the taboo list
> in the patch, but we'll see...).
> 
> The patch itself looks obviously correct and describes the current
> behavior. But I have to wonder: the --short format will also not produce
> the branch line unless you provide "-b". So why is it that the porcelain
> format does not respect "-b", since anybody who asked for it would
> obviously be expecting to find and parse it?
> 
> Should this bit of documentation be dropped in favor of just making "-b"
> work properly?

So that patch could look like the one below. But that brings up new
confusion.

What should "git status --porcelain -z -b" look like? With my patch, it
prints the branch-line with only a newline, not respecting the NUL
termination. Which sounds like a bug, except that's what happens _now_
with "git status --short -z -b".  Which seems like a bug to me, but
maybe somebody is relying on that. It seems kind of broken to me.

Also, while looking at the documentation, we say of "-z": "This implies
the --porcelain output format if no other format is given". But the only
other format you could give is "--short", since there is no way to ask
for the long output (and nor do we handle NUL-termination in that code,
anyway). Should this be simplified to just "this implies porcelain"?
That would technically break somebody who wanted their
status.relativePaths config option respected. But it kind of seems crazy
to me.

-- >8 --
Subject: [PATCH] status: respect "-b" for porcelain format

There is no reason not to, as the user has to explicitly ask
for it, so we are not breaking compatibility by doing so.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-status.txt | 2 +-
 builtin/commit.c             | 4 ++--
 t/t7508-status.sh            | 8 ++++++--
 wt-status.c                  | 5 +++--
 wt-status.h                  | 2 +-
 5 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 2883a28..67e5f53 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -184,7 +184,7 @@ order is reversed (e.g 'from \-> to' becomes 'to from'). Second, a NUL
 and the terminating newline (but a space still separates the status
 field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
-backslash-escaping is performed. Fourth, there is no branch line.
+backslash-escaping is performed.
 
 CONFIGURATION
 -------------
diff --git a/builtin/commit.c b/builtin/commit.c
index a876a73..0e898e6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -504,7 +504,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 		wt_shortstatus_print(s, null_termination, status_show_branch);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_porcelain_print(s, null_termination);
+		wt_porcelain_print(s, null_termination, status_show_branch);
 		break;
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
@@ -1287,7 +1287,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		wt_shortstatus_print(&s, null_termination, status_show_branch);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_porcelain_print(&s, null_termination);
+		wt_porcelain_print(&s, null_termination, status_show_branch);
 		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 8f5cfac..695b6a8 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -671,11 +671,15 @@ test_expect_success 'status --porcelain ignores color.status' '
 git config --unset color.status
 git config --unset color.ui
 
-test_expect_success 'status --porcelain ignores -b' '
+test_expect_success 'status --porcelain respects -b' '
 
 	git status --porcelain -b >output &&
+	{
+		echo "## master" &&
+		cat expect
+	} >tmp &&
+	mv tmp expect &&
 	test_cmp expect output
-
 '
 
 cat >expect <<\EOF
diff --git a/wt-status.c b/wt-status.c
index eeef17e..7d2dbcd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -973,10 +973,11 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination, int show_br
 	}
 }
 
-void wt_porcelain_print(struct wt_status *s, int null_termination)
+void wt_porcelain_print(struct wt_status *s, int null_termination,
+			int show_branch)
 {
 	s->use_color = 0;
 	s->relative_paths = 0;
 	s->prefix = NULL;
-	wt_shortstatus_print(s, null_termination, 0);
+	wt_shortstatus_print(s, null_termination, show_branch);
 }
diff --git a/wt-status.h b/wt-status.h
index 6dd7207..b8e8758 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -74,7 +74,7 @@ void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
 
 void wt_shortstatus_print(struct wt_status *s, int null_termination, int show_branch);
-void wt_porcelain_print(struct wt_status *s, int null_termination);
+void wt_porcelain_print(struct wt_status *s, int null_termination, int show_branch);
 
 void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...)
 	;
-- 
1.7.10.1.14.gb97aca1

Re: [PATCH] correct git-status Porcelain Format documentation

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> What should "git status --porcelain -z -b" look like? With my patch, it
> prints the branch-line with only a newline, not respecting the NUL
> termination. Which sounds like a bug, except that's what happens _now_
> with "git status --short -z -b".  Which seems like a bug to me, but
> maybe somebody is relying on that. It seems kind of broken to me.

It is broken; and it is not a problem if somebody relied on a broken
output without giving --porcelain in a script and such a script needs to
be updated.  If we are updating the code to give "## <branch>" in the
output under the "--porcelain", let's do that right from the beginning.

> Also, while looking at the documentation, we say of "-z": "This implies
> the --porcelain output format if no other format is given". But the only
> other format you could give is "--short", since there is no way to ask
> for the long output (and nor do we handle NUL-termination in that code,
> anyway).

... nor do we define long porcelain format to begin with.

> Should this be simplified to just "this implies porcelain"?
>
> That would technically break somebody who wanted their
> status.relativePaths config option respected. But it kind of seems crazy
> to me.

I do not think it is worth changing it.  It is not too much touble to
spell "status --porcelain -z <whatever else>" in a Porcelain script only
once and forget about it.

Re: [PATCH] correct git-status Porcelain Format documentation

[Jeff King]

On Mon, May 07, 2012 at 11:10:23AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > What should "git status --porcelain -z -b" look like? With my patch, it
> > prints the branch-line with only a newline, not respecting the NUL
> > termination. Which sounds like a bug, except that's what happens _now_
> > with "git status --short -z -b".  Which seems like a bug to me, but
> > maybe somebody is relying on that. It seems kind of broken to me.
> 
> It is broken; and it is not a problem if somebody relied on a broken
> output without giving --porcelain in a script and such a script needs to
> be updated.  If we are updating the code to give "## <branch>" in the
> output under the "--porcelain", let's do that right from the beginning.

OK, that matches my feelings.

> > Should this be simplified to just "this implies porcelain"?
> >
> > That would technically break somebody who wanted their
> > status.relativePaths config option respected. But it kind of seems crazy
> > to me.
> 
> I do not think it is worth changing it.  It is not too much touble to
> spell "status --porcelain -z <whatever else>" in a Porcelain script only
> once and forget about it.

OK. I think it makes it conceptually simpler, but of the problems I
listed, that is the least bug-like.

I'll prepare a series to fix "-b -z" and "-b --porcelain".

-Peff

Re: [PATCH] correct git-status Porcelain Format documentation

[Jeff King]

On Mon, May 07, 2012 at 02:13:48PM -0400, Jeff King wrote:

> I'll prepare a series to fix "-b -z" and "-b --porcelain".

Here's that series:

  [1/5]: commit: refactor option parsing
  [2/5]: status: refactor colopts handling
  [3/5]: status: refactor null_termination option
  [4/5]: status: fix null termination with "-b"
  [5/5]: status: respect "-b" for porcelain format

Patches 1 and 3 are cleanups to aid 4 and 5, which are the real fixes.
Patch 2 is an optional cleanup I noticed while in the area.

-Peff

[PATCH 1/5] commit: refactor option parsing

[Jeff King]

The options are declared as a static global, but really they
need only be accessible from cmd_commit. Putting them there
will allow us to directly reference local variables in the
options list, which will facilitate further cleanups.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c | 113 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 57 insertions(+), 56 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a876a73..5f48e8a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -131,59 +131,6 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static struct option builtin_commit_options[] = {
-	OPT__QUIET(&quiet, "suppress summary after successful commit"),
-	OPT__VERBOSE(&verbose, "show diff in commit message template"),
-
-	OPT_GROUP("Commit message options"),
-	OPT_FILENAME('F', "file", &logfile, "read message from file"),
-	OPT_STRING(0, "author", &force_author, "author", "override author for commit"),
-	OPT_STRING(0, "date", &force_date, "date", "override date for commit"),
-	OPT_CALLBACK('m', "message", &message, "message", "commit message", opt_parse_m),
-	OPT_STRING('c', "reedit-message", &edit_message, "commit", "reuse and edit message from specified commit"),
-	OPT_STRING('C', "reuse-message", &use_message, "commit", "reuse message from specified commit"),
-	OPT_STRING(0, "fixup", &fixup_message, "commit", "use autosquash formatted message to fixup specified commit"),
-	OPT_STRING(0, "squash", &squash_message, "commit", "use autosquash formatted message to squash specified commit"),
-	OPT_BOOLEAN(0, "reset-author", &renew_authorship, "the commit is authored by me now (used with -C/-c/--amend)"),
-	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
-	OPT_FILENAME('t', "template", &template_file, "use specified template file"),
-	OPT_BOOL('e', "edit", &edit_flag, "force edit of commit"),
-	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
-	OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
-	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, "key id",
-	  "GPG sign commit", PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
-	/* end commit message options */
-
-	OPT_GROUP("Commit contents options"),
-	OPT_BOOLEAN('a', "all", &all, "commit all changed files"),
-	OPT_BOOLEAN('i', "include", &also, "add specified files to index for commit"),
-	OPT_BOOLEAN(0, "interactive", &interactive, "interactively add files"),
-	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactively add changes"),
-	OPT_BOOLEAN('o', "only", &only, "commit only specified files"),
-	OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
-	OPT_BOOLEAN(0, "dry-run", &dry_run, "show what would be committed"),
-	OPT_SET_INT(0, "short", &status_format, "show status concisely",
-		    STATUS_FORMAT_SHORT),
-	OPT_BOOLEAN(0, "branch", &status_show_branch, "show branch information"),
-	OPT_SET_INT(0, "porcelain", &status_format,
-		    "machine-readable output", STATUS_FORMAT_PORCELAIN),
-	OPT_BOOLEAN('z', "null", &null_termination,
-		    "terminate entries with NUL"),
-	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
-	OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
-	{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
-	/* end commit contents options */
-
-	{ OPTION_BOOLEAN, 0, "allow-empty", &allow_empty, NULL,
-	  "ok to record an empty change",
-	  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
-	{ OPTION_BOOLEAN, 0, "allow-empty-message", &allow_empty_message, NULL,
-	  "ok to record a change with an empty message",
-	  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
-
-	OPT_END()
-};
-
 static void determine_whence(struct wt_status *s)
 {
 	if (file_exists(git_path("MERGE_HEAD")))
@@ -1037,6 +984,7 @@ static const char *read_commit_message(const char *name)
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
+				      const struct option *options,
 				      const char * const usage[],
 				      const char *prefix,
 				      struct commit *current_head,
@@ -1044,8 +992,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 {
 	int f = 0;
 
-	argc = parse_options(argc, argv, prefix, builtin_commit_options, usage,
-			     0);
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (force_author && !strchr(force_author, '>'))
 		force_author = find_author_by_nickname(force_author);
@@ -1422,6 +1369,59 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
+	static struct option builtin_commit_options[] = {
+		OPT__QUIET(&quiet, "suppress summary after successful commit"),
+		OPT__VERBOSE(&verbose, "show diff in commit message template"),
+
+		OPT_GROUP("Commit message options"),
+		OPT_FILENAME('F', "file", &logfile, "read message from file"),
+		OPT_STRING(0, "author", &force_author, "author", "override author for commit"),
+		OPT_STRING(0, "date", &force_date, "date", "override date for commit"),
+		OPT_CALLBACK('m', "message", &message, "message", "commit message", opt_parse_m),
+		OPT_STRING('c', "reedit-message", &edit_message, "commit", "reuse and edit message from specified commit"),
+		OPT_STRING('C', "reuse-message", &use_message, "commit", "reuse message from specified commit"),
+		OPT_STRING(0, "fixup", &fixup_message, "commit", "use autosquash formatted message to fixup specified commit"),
+		OPT_STRING(0, "squash", &squash_message, "commit", "use autosquash formatted message to squash specified commit"),
+		OPT_BOOLEAN(0, "reset-author", &renew_authorship, "the commit is authored by me now (used with -C/-c/--amend)"),
+		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
+		OPT_FILENAME('t', "template", &template_file, "use specified template file"),
+		OPT_BOOL('e', "edit", &edit_flag, "force edit of commit"),
+		OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
+		OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
+		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, "key id",
+		  "GPG sign commit", PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+		/* end commit message options */
+
+		OPT_GROUP("Commit contents options"),
+		OPT_BOOLEAN('a', "all", &all, "commit all changed files"),
+		OPT_BOOLEAN('i', "include", &also, "add specified files to index for commit"),
+		OPT_BOOLEAN(0, "interactive", &interactive, "interactively add files"),
+		OPT_BOOLEAN('p', "patch", &patch_interactive, "interactively add changes"),
+		OPT_BOOLEAN('o', "only", &only, "commit only specified files"),
+		OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
+		OPT_BOOLEAN(0, "dry-run", &dry_run, "show what would be committed"),
+		OPT_SET_INT(0, "short", &status_format, "show status concisely",
+			    STATUS_FORMAT_SHORT),
+		OPT_BOOLEAN(0, "branch", &status_show_branch, "show branch information"),
+		OPT_SET_INT(0, "porcelain", &status_format,
+			    "machine-readable output", STATUS_FORMAT_PORCELAIN),
+		OPT_BOOLEAN('z', "null", &null_termination,
+			    "terminate entries with NUL"),
+		OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
+		OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
+		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+		/* end commit contents options */
+
+		{ OPTION_BOOLEAN, 0, "allow-empty", &allow_empty, NULL,
+		  "ok to record an empty change",
+		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
+		{ OPTION_BOOLEAN, 0, "allow-empty-message", &allow_empty_message, NULL,
+		  "ok to record a change with an empty message",
+		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
+
+		OPT_END()
+	};
+
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf author_ident = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
@@ -1449,7 +1449,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (!current_head || parse_commit(current_head))
 			die(_("could not parse HEAD commit"));
 	}
-	argc = parse_and_validate_options(argc, argv, builtin_commit_usage,
+	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
+					  builtin_commit_usage,
 					  prefix, current_head, &s);
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
-- 
1.7.10.1.12.gd79f7ab

[PATCH 2/5] status: refactor colopts handling

[Jeff King]

The current code reads the config and command-line options
into a separate "colopts" variable, and then copies the
contents of that variable into the "struct wt_status". We
can eliminate the extra variable and copy just write
straight into the wt_status struct.

This simplifies the "status" code a little bit.
Unfortunately, it makes the "commit" code one line more
complex; a side effect of the separate variable was that
"commit" did not copy the colopts variable, so any
column.status configuration had no effect.

The result still ends up cleaner, though. In the previous
version, it was unclear whether commit simply forgot to copy
the colopt variable, or whether it was intentional. Now it
explicitly turns off column options. Furthermore, if commit
later learns to respect column.status, this will make the
end result simpler. I punted on just adding that feature
now, because it was sufficiently non-obvious that it should
not go into a refactoring patch.

Signed-off-by: Jeff King <peff@peff.net>
---
For the reasons mentioned above, this ended up as a less impressive
cleanup than I had hoped. I think it's still worth doing, but I won't be
too sad if it gets dropped.

 builtin/commit.c | 13 ++++++-------
 wt-status.h      |  2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5f48e8a..1a69cb0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -89,7 +89,6 @@ static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
 static char *sign_commit;
-static unsigned int colopts;
 
 /*
  * The default commit message cleanup mode will remove the lines
@@ -1123,7 +1122,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 	struct wt_status *s = cb;
 
 	if (!prefixcmp(k, "column."))
-		return git_column_config(k, v, "status", &colopts);
+		return git_column_config(k, v, "status", &s->colopts);
 	if (!strcmp(k, "status.submodulesummary")) {
 		int is_bool;
 		s->submodule_summary = git_config_bool_or_int(k, v, &is_bool);
@@ -1166,7 +1165,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
-	struct wt_status s;
+	static struct wt_status s;
 	int fd;
 	unsigned char sha1[20];
 	static struct option builtin_status_options[] = {
@@ -1189,7 +1188,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		{ OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, "when",
 		  "ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)",
 		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
-		OPT_COLUMN(0, "column", &colopts, "list untracked files in columns"),
+		OPT_COLUMN(0, "column", &s.colopts, "list untracked files in columns"),
 		OPT_END(),
 	};
 
@@ -1203,8 +1202,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix,
 			     builtin_status_options,
 			     builtin_status_usage, 0);
-	finalize_colopts(&colopts, -1);
-	s.colopts = colopts;
+	finalize_colopts(&s.colopts, -1);
 
 	if (null_termination && status_format == STATUS_FORMAT_LONG)
 		status_format = STATUS_FORMAT_PORCELAIN;
@@ -1369,6 +1367,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
+	static struct wt_status s;
 	static struct option builtin_commit_options[] = {
 		OPT__QUIET(&quiet, "suppress summary after successful commit"),
 		OPT__VERBOSE(&verbose, "show diff in commit message template"),
@@ -1431,7 +1430,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct commit_list *parents = NULL, **pptr = &parents;
 	struct stat statbuf;
 	int allow_fast_forward = 1;
-	struct wt_status s;
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
 
@@ -1441,6 +1439,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	wt_status_prepare(&s);
 	git_config(git_commit_config, &s);
 	determine_whence(&s);
+	s.colopts = 0;
 
 	if (get_sha1("HEAD", sha1))
 		current_head = NULL;
diff --git a/wt-status.h b/wt-status.h
index 6dd7207..5386077 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -56,7 +56,7 @@ struct wt_status {
 	enum untracked_status_type show_untracked_files;
 	const char *ignore_submodule_arg;
 	char color_palette[WT_STATUS_MAXSLOT][COLOR_MAXLEN];
-	int colopts;
+	unsigned colopts;
 
 	/* These are computed during processing of the individual sections */
 	int commitable;
-- 
1.7.10.1.12.gd79f7ab

Re: [PATCH 2/5] status: refactor colopts handling

[Nguyen Thai Ngoc Duy]

On Tue, May 8, 2012 at 4:23 AM, Jeff King <peff@peff.net> wrote:
> The current code reads the config and command-line options
> into a separate "colopts" variable, and then copies the
> contents of that variable into the "struct wt_status". We
> can eliminate the extra variable and copy just write
> straight into the wt_status struct.
>
> This simplifies the "status" code a little bit.
> Unfortunately, it makes the "commit" code one line more
> complex; a side effect of the separate variable was that
> "commit" did not copy the colopts variable, so any
> column.status configuration had no effect.
>
> The result still ends up cleaner, though. In the previous
> version, it was unclear whether commit simply forgot to copy
> the colopt variable, or whether it was intentional. Now it
> explicitly turns off column options. Furthermore, if commit
> later learns to respect column.status, this will make the
> end result simpler. I punted on just adding that feature
> now, because it was sufficiently non-obvious that it should
> not go into a refactoring patch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> For the reasons mentioned above, this ended up as a less impressive
> cleanup than I had hoped. I think it's still worth doing, but I won't be
> too sad if it gets dropped.

I'm neutral. But this patch reduces one line so it wins a "yes" from me.

>  builtin/commit.c | 13 ++++++-------
>  wt-status.h      |  2 +-
>  2 files changed, 7 insertions(+), 8 deletions(-)
-- 
Duy

[PATCH 3/5] status: refactor null_termination option

[Jeff King]

This option is passed separately to the wt_status printing
functions, whereas every other formatting option is
contained in the wt_status struct itself. Let's do the same
here, so we can avoid passing it around through the call
stack.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c | 17 ++++++++---------
 wt-status.c      | 26 +++++++++++++-------------
 wt-status.h      |  5 +++--
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1a69cb0..11271c0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -110,7 +110,6 @@ static int show_ignored_in_status;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
-static int null_termination;
 static enum {
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
@@ -447,10 +446,10 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
-		wt_shortstatus_print(s, null_termination, status_show_branch);
+		wt_shortstatus_print(s, status_show_branch);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_porcelain_print(s, null_termination);
+		wt_porcelain_print(s);
 		break;
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
@@ -1076,7 +1075,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (all && argc > 0)
 		die(_("Paths with -a does not make sense."));
 
-	if (null_termination && status_format == STATUS_FORMAT_LONG)
+	if (s->null_termination && status_format == STATUS_FORMAT_LONG)
 		status_format = STATUS_FORMAT_PORCELAIN;
 	if (status_format != STATUS_FORMAT_LONG)
 		dry_run = 1;
@@ -1177,7 +1176,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    "machine-readable output",
 			    STATUS_FORMAT_PORCELAIN),
-		OPT_BOOLEAN('z', "null", &null_termination,
+		OPT_BOOLEAN('z', "null", &s.null_termination,
 			    "terminate entries with NUL"),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg,
 		  "mode",
@@ -1204,7 +1203,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			     builtin_status_usage, 0);
 	finalize_colopts(&s.colopts, -1);
 
-	if (null_termination && status_format == STATUS_FORMAT_LONG)
+	if (s.null_termination && status_format == STATUS_FORMAT_LONG)
 		status_format = STATUS_FORMAT_PORCELAIN;
 
 	handle_untracked_files_arg(&s);
@@ -1229,10 +1228,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
-		wt_shortstatus_print(&s, null_termination, status_show_branch);
+		wt_shortstatus_print(&s, status_show_branch);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_porcelain_print(&s, null_termination);
+		wt_porcelain_print(&s);
 		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
@@ -1404,7 +1403,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN(0, "branch", &status_show_branch, "show branch information"),
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    "machine-readable output", STATUS_FORMAT_PORCELAIN),
-		OPT_BOOLEAN('z', "null", &null_termination,
+		OPT_BOOLEAN('z', "null", &s.null_termination,
 			    "terminate entries with NUL"),
 		OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 		OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
diff --git a/wt-status.c b/wt-status.c
index eeef17e..284dc61 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -801,7 +801,7 @@ void wt_status_print(struct wt_status *s)
 	}
 }
 
-static void wt_shortstatus_unmerged(int null_termination, struct string_list_item *it,
+static void wt_shortstatus_unmerged(struct string_list_item *it,
 			   struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
@@ -817,7 +817,7 @@ static void wt_shortstatus_unmerged(int null_termination, struct string_list_ite
 	case 7: how = "UU"; break; /* both modified */
 	}
 	color_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), "%s", how);
-	if (null_termination) {
+	if (s->null_termination) {
 		fprintf(stdout, " %s%c", it->string, 0);
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
@@ -828,7 +828,7 @@ static void wt_shortstatus_unmerged(int null_termination, struct string_list_ite
 	}
 }
 
-static void wt_shortstatus_status(int null_termination, struct string_list_item *it,
+static void wt_shortstatus_status(struct string_list_item *it,
 			 struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
@@ -842,7 +842,7 @@ static void wt_shortstatus_status(int null_termination, struct string_list_item
 	else
 		putchar(' ');
 	putchar(' ');
-	if (null_termination) {
+	if (s->null_termination) {
 		fprintf(stdout, "%s%c", it->string, 0);
 		if (d->head_path)
 			fprintf(stdout, "%s%c", d->head_path, 0);
@@ -870,10 +870,10 @@ static void wt_shortstatus_status(int null_termination, struct string_list_item
 	}
 }
 
-static void wt_shortstatus_other(int null_termination, struct string_list_item *it,
+static void wt_shortstatus_other(struct string_list_item *it,
 				 struct wt_status *s, const char *sign)
 {
-	if (null_termination) {
+	if (s->null_termination) {
 		fprintf(stdout, "%s %s%c", sign, it->string, 0);
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
@@ -941,7 +941,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	color_fprintf_ln(s->fp, header_color, "]");
 }
 
-void wt_shortstatus_print(struct wt_status *s, int null_termination, int show_branch)
+void wt_shortstatus_print(struct wt_status *s, int show_branch)
 {
 	int i;
 
@@ -955,28 +955,28 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination, int show_br
 		it = &(s->change.items[i]);
 		d = it->util;
 		if (d->stagemask)
-			wt_shortstatus_unmerged(null_termination, it, s);
+			wt_shortstatus_unmerged(it, s);
 		else
-			wt_shortstatus_status(null_termination, it, s);
+			wt_shortstatus_status(it, s);
 	}
 	for (i = 0; i < s->untracked.nr; i++) {
 		struct string_list_item *it;
 
 		it = &(s->untracked.items[i]);
-		wt_shortstatus_other(null_termination, it, s, "??");
+		wt_shortstatus_other(it, s, "??");
 	}
 	for (i = 0; i < s->ignored.nr; i++) {
 		struct string_list_item *it;
 
 		it = &(s->ignored.items[i]);
-		wt_shortstatus_other(null_termination, it, s, "!!");
+		wt_shortstatus_other(it, s, "!!");
 	}
 }
 
-void wt_porcelain_print(struct wt_status *s, int null_termination)
+void wt_porcelain_print(struct wt_status *s)
 {
 	s->use_color = 0;
 	s->relative_paths = 0;
 	s->prefix = NULL;
-	wt_shortstatus_print(s, null_termination, 0);
+	wt_shortstatus_print(s, 0);
 }
diff --git a/wt-status.h b/wt-status.h
index 5386077..26dd21a 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -57,6 +57,7 @@ struct wt_status {
 	const char *ignore_submodule_arg;
 	char color_palette[WT_STATUS_MAXSLOT][COLOR_MAXLEN];
 	unsigned colopts;
+	int null_termination;
 
 	/* These are computed during processing of the individual sections */
 	int commitable;
@@ -73,8 +74,8 @@ void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
 
-void wt_shortstatus_print(struct wt_status *s, int null_termination, int show_branch);
-void wt_porcelain_print(struct wt_status *s, int null_termination);
+void wt_shortstatus_print(struct wt_status *s, int show_branch);
+void wt_porcelain_print(struct wt_status *s);
 
 void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...)
 	;
-- 
1.7.10.1.12.gd79f7ab

[PATCH 4/5] status: fix null termination with "-b"

[Jeff King]

When the "-z" option is given to status, we are supposed to
NUL-terminate each record. However, the "-b" code to show
the tracking branch did not respect this, and always ended
with a newline.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7508-status.sh | 9 +++++++++
 wt-status.c       | 7 ++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 8f5cfac..f60f49b 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -295,6 +295,15 @@ test_expect_success 'status -s -b' '
 
 '
 
+test_expect_success 'status -s -z -b' '
+	tr "\\n" Q <expect >expect.q &&
+	mv expect.q expect &&
+	git status -s -z -b >output &&
+	nul_to_q <output >output.q &&
+	mv output.q output &&
+	test_cmp expect output
+'
+
 test_expect_success 'setup dir3' '
 	mkdir dir3 &&
 	: >dir3/untracked1 &&
diff --git a/wt-status.c b/wt-status.c
index 284dc61..21c0d4d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -913,8 +913,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	if (s->is_initial)
 		color_fprintf(s->fp, header_color, _("Initial commit on "));
 	if (!stat_tracking_info(branch, &num_ours, &num_theirs)) {
-		color_fprintf_ln(s->fp, branch_color_local,
-			"%s", branch_name);
+		color_fprintf(s->fp, branch_color_local, "%s", branch_name);
+		fputc(s->null_termination ? '\0' : '\n', s->fp);
 		return;
 	}
 
@@ -938,7 +938,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 		color_fprintf(s->fp, branch_color_remote, "%d", num_theirs);
 	}
 
-	color_fprintf_ln(s->fp, header_color, "]");
+	color_fprintf(s->fp, header_color, "]");
+	fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
 void wt_shortstatus_print(struct wt_status *s, int show_branch)
-- 
1.7.10.1.12.gd79f7ab

[PATCH 5/5] status: respect "-b" for porcelain format

[Jeff King]

There is no reason not to, as the user has to explicitly ask
for it, so we are not breaking compatibility by doing so. We
can do this simply by moving the "show_branch" flag into
the wt_status struct. As a bonus, this saves us from passing
it explicitly, simplifying the code.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c  | 9 ++++-----
 t/t7508-status.sh | 7 ++++++-
 wt-status.c       | 6 +++---
 wt-status.h       | 3 ++-
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 11271c0..a2ec73d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -115,7 +115,6 @@ static enum {
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN
 } status_format = STATUS_FORMAT_LONG;
-static int status_show_branch;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
@@ -446,7 +445,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
-		wt_shortstatus_print(s, status_show_branch);
+		wt_shortstatus_print(s);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
 		wt_porcelain_print(s);
@@ -1171,7 +1170,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		OPT__VERBOSE(&verbose, "be verbose"),
 		OPT_SET_INT('s', "short", &status_format,
 			    "show status concisely", STATUS_FORMAT_SHORT),
-		OPT_BOOLEAN('b', "branch", &status_show_branch,
+		OPT_BOOLEAN('b', "branch", &s.show_branch,
 			    "show branch information"),
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    "machine-readable output",
@@ -1228,7 +1227,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
-		wt_shortstatus_print(&s, status_show_branch);
+		wt_shortstatus_print(&s);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
 		wt_porcelain_print(&s);
@@ -1400,7 +1399,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN(0, "dry-run", &dry_run, "show what would be committed"),
 		OPT_SET_INT(0, "short", &status_format, "show status concisely",
 			    STATUS_FORMAT_SHORT),
-		OPT_BOOLEAN(0, "branch", &status_show_branch, "show branch information"),
+		OPT_BOOLEAN(0, "branch", &s.show_branch, "show branch information"),
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    "machine-readable output", STATUS_FORMAT_PORCELAIN),
 		OPT_BOOLEAN('z', "null", &s.null_termination,
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index f60f49b..28e1848 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -680,9 +680,14 @@ test_expect_success 'status --porcelain ignores color.status' '
 git config --unset color.status
 git config --unset color.ui
 
-test_expect_success 'status --porcelain ignores -b' '
+test_expect_success 'status --porcelain respects -b' '
 
 	git status --porcelain -b >output &&
+	{
+		echo "## master" &&
+		cat expect
+	} >tmp &&
+	mv tmp expect &&
 	test_cmp expect output
 
 '
diff --git a/wt-status.c b/wt-status.c
index 21c0d4d..dd6d8c4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -942,11 +942,11 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
-void wt_shortstatus_print(struct wt_status *s, int show_branch)
+void wt_shortstatus_print(struct wt_status *s)
 {
 	int i;
 
-	if (show_branch)
+	if (s->show_branch)
 		wt_shortstatus_print_tracking(s);
 
 	for (i = 0; i < s->change.nr; i++) {
@@ -979,5 +979,5 @@ void wt_porcelain_print(struct wt_status *s)
 	s->use_color = 0;
 	s->relative_paths = 0;
 	s->prefix = NULL;
-	wt_shortstatus_print(s, 0);
+	wt_shortstatus_print(s);
 }
diff --git a/wt-status.h b/wt-status.h
index 26dd21a..14aa9f7 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -58,6 +58,7 @@ struct wt_status {
 	char color_palette[WT_STATUS_MAXSLOT][COLOR_MAXLEN];
 	unsigned colopts;
 	int null_termination;
+	int show_branch;
 
 	/* These are computed during processing of the individual sections */
 	int commitable;
@@ -74,7 +75,7 @@ void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
 
-void wt_shortstatus_print(struct wt_status *s, int show_branch);
+void wt_shortstatus_print(struct wt_status *s);
 void wt_porcelain_print(struct wt_status *s);
 
 void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...)
-- 
1.7.10.1.12.gd79f7ab

Re: [PATCH 5/5] status: respect "-b" for porcelain format

[Jeff King]

On Mon, May 07, 2012 at 05:25:04PM -0400, Jeff King wrote:

> There is no reason not to, as the user has to explicitly ask
> for it, so we are not breaking compatibility by doing so. We
> can do this simply by moving the "show_branch" flag into
> the wt_status struct. As a bonus, this saves us from passing
> it explicitly, simplifying the code.

I forgot this, which should probably be squashed in:

-- >8 --
Subject: status: fix "--porcelain -z -b" documentation

It is no longer the case that "-z" will not respect "-b".

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-status.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 2883a28..67e5f53 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -184,7 +184,7 @@ order is reversed (e.g 'from \-> to' becomes 'to from'). Second, a NUL
 and the terminating newline (but a space still separates the status
 field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
-backslash-escaping is performed. Fourth, there is no branch line.
+backslash-escaping is performed.
 
 CONFIGURATION
 -------------
-- 
1.7.10.1.12.gd79f7ab

Re: [PATCH 5/5] status: respect "-b" for porcelain format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, May 07, 2012 at 05:25:04PM -0400, Jeff King wrote:
>
>> There is no reason not to, as the user has to explicitly ask
>> for it, so we are not breaking compatibility by doing so. We
>> can do this simply by moving the "show_branch" flag into
>> the wt_status struct. As a bonus, this saves us from passing
>> it explicitly, simplifying the code.
>
> I forgot this, which should probably be squashed in:
>
> -- >8 --
> Subject: status: fix "--porcelain -z -b" documentation
>
> It is no longer the case that "-z" will not respect "-b".
>
> Signed-off-by: Jeff King <peff@peff.net>

All patches looked good.  I thought that this deserved to eventually land
on 'maint', so I did my usual "am first to master primarily to populate
the object database with exact blob objects, then rebase the result to
'maint' while resolving conflicts both textual and semantic, and finally
attempt to merge it back to 'master'" dance to come up with a two topic
branches.  The result is queued in 'pu'.

Thanks.

Re: [PATCH 5/5] status: respect "-b" for porcelain format

[Jeff King]

On Mon, May 07, 2012 at 09:55:05PM -0700, Junio C Hamano wrote:

> All patches looked good.  I thought that this deserved to eventually land
> on 'maint', so I did my usual "am first to master primarily to populate
> the object database with exact blob objects, then rebase the result to
> 'maint' while resolving conflicts both textual and semantic, and finally
> attempt to merge it back to 'master'" dance to come up with a two topic
> branches.  The result is queued in 'pu'.

I eyeballed the result. It all looks good except for patch 2 (refactor
colopts handling), which now has a totally bogus commit message. Since
colopts came much later, the only thing left in the patch is the
movement of the local "struct wt_status s" in cmd_{commit,status} to a
static at the top of each function (so that it can be referenced
directly by the options list).

Probably that patch (94f98829) should just be squashed into its parent.
And then the colopts bits can be moved to the very end, after the rest
of it, and be a part of only the non-maint topic.

I've pushed that result out to the jk/{maint-,}status-porcelain-z-b
branches at git://github.com/peff/git.git if that helps.

-Peff

Re: [PATCH 5/5] status: respect "-b" for porcelain format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Probably that patch (94f98829) should just be squashed into its parent.
> And then the colopts bits can be moved to the very end, after the rest
> of it, and be a part of only the non-maint topic.

Makes sense.  Thanks.

>
> I've pushed that result out to the jk/{maint-,}status-porcelain-z-b
> branches at git://github.com/peff/git.git if that helps.
>
> -Peff

Re: [PATCH] correct git-status Porcelain Format documentation

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The patch itself looks obviously correct and describes the current
> behavior. But I have to wonder: the --short format will also not produce
> the branch line unless you provide "-b". So why is it that the porcelain
> format does not respect "-b", since anybody who asked for it would
> obviously be expecting to find and parse it?
>
> Should this bit of documentation be dropped in favor of just making "-b"
> work properly?

I would say so.  Also "branch line" was an undefined term and I had to
read the whole thing to find out what the updated documentation was
talking about; we may want to consistently say "branch and tracking info"
as the earlier part of the document says.