[PATCH 0/2] Refactor status producers and make status -s use color

[PATCH 0/2] Refactor status producers and make status -s use color

[Michael J Gruber]

This is is the refactoring as suggested by Junio, and rebased on top is
the patch which makes status -s obey color.status. I also adjusted the
latter to not color the space separating the (colored) status letters
from the (uncolored) file name.

This mini series is directly on top of 91691ec (Merge branch
'jk/1.7.0-status' into next, 2009-11-27).

Michael J Gruber (2):
  builtin-commit: refactor short-status code into wt-status.c
  status -s: obey color.status

 builtin-commit.c |  105 ++++-------------------------------------------------
 wt-status.c      |   96 +++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h      |    2 +
 3 files changed, 106 insertions(+), 97 deletions(-)

[PATCH 1/2] builtin-commit: refactor short-status code into wt-status.c

[Michael J Gruber]

Currently, builtin-commit.c contains most code producing the
short-status output, whereas wt-status.c contains most of the code for
the long format.

Refactor so that most of the long and short format producing code
resides in wt-status.c and is named analogously.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin-commit.c |  101 ++---------------------------------------------------
 wt-status.c      |   89 +++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h      |    2 +
 3 files changed, 95 insertions(+), 97 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index c103beb..548bbf5 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -79,8 +79,6 @@ static enum {
 	STATUS_FORMAT_PORCELAIN,
 } status_format = STATUS_FORMAT_LONG;
 
-static void short_print(struct wt_status *s, int null_termination);
-
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
 	struct strbuf *buf = opt->value;
@@ -387,10 +385,10 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
-		short_print(s, null_termination);
+		wt_shortstatus_print(s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		short_print(s, null_termination);
+		wt_shortstatus_print(s, null_termination);
 		break;
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
@@ -982,97 +980,6 @@ static int git_status_config(const char *k, const char *v, void *cb)
 	return git_diff_ui_config(k, v, NULL);
 }
 
-#define quote_path quote_path_relative
-
-static void short_unmerged(int null_termination, struct string_list_item *it,
-			   struct wt_status *s)
-{
-	struct wt_status_change_data *d = it->util;
-	const char *how = "??";
-
-	switch (d->stagemask) {
-	case 1: how = "DD"; break; /* both deleted */
-	case 2: how = "AU"; break; /* added by us */
-	case 3: how = "UD"; break; /* deleted by them */
-	case 4: how = "UA"; break; /* added by them */
-	case 5: how = "DU"; break; /* deleted by us */
-	case 6: how = "AA"; break; /* both added */
-	case 7: how = "UU"; break; /* both modified */
-	}
-	printf("%s ", how);
-	if (null_termination) {
-		fprintf(stdout, "%s%c", it->string, 0);
-	} else {
-		struct strbuf onebuf = STRBUF_INIT;
-		const char *one;
-		one = quote_path(it->string, -1, &onebuf, s->prefix);
-		printf("%s\n", one);
-		strbuf_release(&onebuf);
-	}
-}
-
-static void short_status(int null_termination, struct string_list_item *it,
-			 struct wt_status *s)
-{
-	struct wt_status_change_data *d = it->util;
-
-	printf("%c%c ",
-	       !d->index_status ? ' ' : d->index_status,
-	       !d->worktree_status ? ' ' : d->worktree_status);
-	if (null_termination) {
-		fprintf(stdout, "%s%c", it->string, 0);
-		if (d->head_path)
-			fprintf(stdout, "%s%c", d->head_path, 0);
-	} else {
-		struct strbuf onebuf = STRBUF_INIT;
-		const char *one;
-		if (d->head_path) {
-			one = quote_path(d->head_path, -1, &onebuf, s->prefix);
-			printf("%s -> ", one);
-			strbuf_release(&onebuf);
-		}
-		one = quote_path(it->string, -1, &onebuf, s->prefix);
-		printf("%s\n", one);
-		strbuf_release(&onebuf);
-	}
-}
-
-static void short_untracked(int null_termination, struct string_list_item *it,
-			    struct wt_status *s)
-{
-	if (null_termination) {
-		fprintf(stdout, "?? %s%c", it->string, 0);
-	} else {
-		struct strbuf onebuf = STRBUF_INIT;
-		const char *one;
-		one = quote_path(it->string, -1, &onebuf, s->prefix);
-		printf("?? %s\n", one);
-		strbuf_release(&onebuf);
-	}
-}
-
-static void short_print(struct wt_status *s, int null_termination)
-{
-	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)
-			short_unmerged(null_termination, it, s);
-		else
-			short_status(null_termination, it, s);
-	}
-	for (i = 0; i < s->untracked.nr; i++) {
-		struct string_list_item *it;
-
-		it = &(s->untracked.items[i]);
-		short_untracked(null_termination, it, s);
-	}
-}
-
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
 	struct wt_status s;
@@ -1115,10 +1022,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	case STATUS_FORMAT_SHORT:
 		if (s.relative_paths)
 			s.prefix = prefix;
-		short_print(&s, null_termination);
+		wt_shortstatus_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		short_print(&s, null_termination);
+		wt_shortstatus_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
diff --git a/wt-status.c b/wt-status.c
index 3c2f580..93af994 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -600,3 +600,92 @@ void wt_status_print(struct wt_status *s)
 			printf("nothing to commit (working directory clean)\n");
 	}
 }
+
+static void wt_shortstatus_unmerged(int null_termination, struct string_list_item *it,
+			   struct wt_status *s)
+{
+	struct wt_status_change_data *d = it->util;
+	const char *how = "??";
+
+	switch (d->stagemask) {
+	case 1: how = "DD"; break; /* both deleted */
+	case 2: how = "AU"; break; /* added by us */
+	case 3: how = "UD"; break; /* deleted by them */
+	case 4: how = "UA"; break; /* added by them */
+	case 5: how = "DU"; break; /* deleted by us */
+	case 6: how = "AA"; break; /* both added */
+	case 7: how = "UU"; break; /* both modified */
+	}
+	printf("%s ", how);
+	if (null_termination) {
+		fprintf(stdout, "%s%c", it->string, 0);
+	} else {
+		struct strbuf onebuf = STRBUF_INIT;
+		const char *one;
+		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		printf("%s\n", one);
+		strbuf_release(&onebuf);
+	}
+}
+
+static void wt_shortstatus_status(int null_termination, struct string_list_item *it,
+			 struct wt_status *s)
+{
+	struct wt_status_change_data *d = it->util;
+
+	printf("%c%c ",
+		!d->index_status ? ' ' : d->index_status,
+		!d->worktree_status ? ' ' : d->worktree_status);
+	if (null_termination) {
+		fprintf(stdout, "%s%c", it->string, 0);
+		if (d->head_path)
+			fprintf(stdout, "%s%c", d->head_path, 0);
+	} else {
+		struct strbuf onebuf = STRBUF_INIT;
+		const char *one;
+		if (d->head_path) {
+			one = quote_path(d->head_path, -1, &onebuf, s->prefix);
+			printf("%s -> ", one);
+			strbuf_release(&onebuf);
+		}
+		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		printf("%s\n", one);
+		strbuf_release(&onebuf);
+	}
+}
+
+static void wt_shortstatus_untracked(int null_termination, struct string_list_item *it,
+			    struct wt_status *s)
+{
+	if (null_termination) {
+		fprintf(stdout, "?? %s%c", it->string, 0);
+	} else {
+		struct strbuf onebuf = STRBUF_INIT;
+		const char *one;
+		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		printf("?? %s\n", one);
+		strbuf_release(&onebuf);
+	}
+}
+
+void wt_shortstatus_print(struct wt_status *s, int null_termination)
+{
+	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_shortstatus_unmerged(null_termination, it, s);
+		else
+			wt_shortstatus_status(null_termination, it, s);
+	}
+	for (i = 0; i < s->untracked.nr; i++) {
+		struct string_list_item *it;
+
+		it = &(s->untracked.items[i]);
+		wt_shortstatus_untracked(null_termination, it, s);
+	}
+}
diff --git a/wt-status.h b/wt-status.h
index 09fd9f1..39c9aef 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -56,4 +56,6 @@ 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);
+
 #endif /* STATUS_H */
-- 
1.6.6.rc1.282.ge6667

[PATCH 2/2] status -s: obey color.status

[Michael J Gruber]

Make the short version of status obey the color.status boolean. We color
the status letters only, because they carry the state information and are
potentially colored differently, such as for a file with staged changes
as well as changes in the worktree against the index.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin-commit.c |    4 ++++
 wt-status.c      |   21 ++++++++++++++-------
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 548bbf5..ddcfffb 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1022,6 +1022,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	case STATUS_FORMAT_SHORT:
 		if (s.relative_paths)
 			s.prefix = prefix;
+		if (s.use_color == -1)
+			s.use_color = git_use_color_default;
+		if (diff_use_color_default == -1)
+			diff_use_color_default = git_use_color_default;
 		wt_shortstatus_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
diff --git a/wt-status.c b/wt-status.c
index 93af994..a8b6d05 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -616,14 +616,14 @@ static void wt_shortstatus_unmerged(int null_termination, struct string_list_ite
 	case 6: how = "AA"; break; /* both added */
 	case 7: how = "UU"; break; /* both modified */
 	}
-	printf("%s ", how);
+	color_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), "%s", how);
 	if (null_termination) {
-		fprintf(stdout, "%s%c", it->string, 0);
+		fprintf(stdout, " %s%c", it->string, 0);
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
 		one = quote_path(it->string, -1, &onebuf, s->prefix);
-		printf("%s\n", one);
+		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
 }
@@ -633,9 +633,15 @@ static void wt_shortstatus_status(int null_termination, struct string_list_item
 {
 	struct wt_status_change_data *d = it->util;
 
-	printf("%c%c ",
-		!d->index_status ? ' ' : d->index_status,
-		!d->worktree_status ? ' ' : d->worktree_status);
+	if (d->index_status)
+		color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status);
+	else
+		putchar(' ');
+	if (d->worktree_status)
+		color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%c", d->worktree_status);
+	else
+		putchar(' ');
+	putchar(' ');
 	if (null_termination) {
 		fprintf(stdout, "%s%c", it->string, 0);
 		if (d->head_path)
@@ -663,7 +669,8 @@ static void wt_shortstatus_untracked(int null_termination, struct string_list_it
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
 		one = quote_path(it->string, -1, &onebuf, s->prefix);
-		printf("?? %s\n", one);
+		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "??");
+		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
 }
-- 
1.6.6.rc1.282.ge6667

Re: [PATCH 1/2] builtin-commit: refactor short-status code into wt-status.c

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Currently, builtin-commit.c contains most code producing the
> short-status output, whereas wt-status.c contains most of the code for
> the long format.
>
> Refactor so that most of the long and short format producing code
> resides in wt-status.c and is named analogously.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  builtin-commit.c |  101 ++---------------------------------------------------
>  wt-status.c      |   89 +++++++++++++++++++++++++++++++++++++++++++++++
>  wt-status.h      |    2 +
>  3 files changed, 95 insertions(+), 97 deletions(-)

Nice.

Thanks.

Re: [PATCH 2/2] status -s: obey color.status

[Jeff King]

On Sat, Dec 05, 2009 at 04:04:38PM +0100, Michael J Gruber wrote:

> Make the short version of status obey the color.status boolean. We color
> the status letters only, because they carry the state information and are
> potentially colored differently, such as for a file with staged changes
> as well as changes in the worktree against the index.

This seems to also turn on color for --porcelain in some cases, because
git_status_config unconditionally sets s->use_color if you are using
color.status instead of color.ui. I think we are probably best just
explicitly disabling options for the "porcelain" format rather than
trying to come up with some trickery to make sure they never get set.
Like:

-- >8 --
Subject: [PATCH] status: disable color for porcelain format

The porcelain format is identical to the shortstatus format,
except that it should not respect any user configuration,
including color.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-commit.c |    4 ++--
 wt-status.c      |    6 ++++++
 wt-status.h      |    1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index ddcfffb..88b25aa 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -388,7 +388,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 		wt_shortstatus_print(s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_shortstatus_print(s, null_termination);
+		wt_porcelain_print(s, null_termination);
 		break;
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
@@ -1029,7 +1029,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		wt_shortstatus_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_shortstatus_print(&s, null_termination);
+		wt_porcelain_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
diff --git a/wt-status.c b/wt-status.c
index a8b6d05..e9bbfbc 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -696,3 +696,9 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination)
 		wt_shortstatus_untracked(null_termination, it, s);
 	}
 }
+
+void wt_porcelain_print(struct wt_status *s, int null_termination)
+{
+	s->use_color = 0;
+	wt_shortstatus_print(s, null_termination);
+}
diff --git a/wt-status.h b/wt-status.h
index 39c9aef..a4bddcf 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -57,5 +57,6 @@ 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);
+void wt_porcelain_print(struct wt_status *s, int null_termination);
 
 #endif /* STATUS_H */
-- 
1.6.6.rc1.292.gd8fe.dirty

Re: [PATCH 2/2] status -s: obey color.status

[Jeff King]

On Mon, Dec 07, 2009 at 12:17:15AM -0500, Jeff King wrote:

> This seems to also turn on color for --porcelain in some cases, because
> git_status_config unconditionally sets s->use_color if you are using
> color.status instead of color.ui. I think we are probably best just
> explicitly disabling options for the "porcelain" format rather than
> trying to come up with some trickery to make sure they never get set.

Also, this means we can hoist repeated code out of the switch statement,
like this:

-- >8 --
Subject: [PATCH] status: reduce duplicated setup code

We have three output formats: short, porcelain, and long.
The short and long formats respect user-config, and the
porcelain one does not. This led to us repeating
config-related setup code for the short and long formats.

Since the last commit, color config is explicitly cleared
when showing the porcelain format. Let's do the same with
relative-path configuration, which enables us to hoist the
duplicated code from the switch statement in cmd_status.

As a bonus, this fixes "commit --dry-run --porcelain", which
was unconditionally setting up that configuration, anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-commit.c |   19 +++++++------------
 wt-status.c      |    2 ++
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 88b25aa..a11e585 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1018,14 +1018,15 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
 	wt_status_collect(&s);
 
+	if (s.relative_paths)
+		s.prefix = prefix;
+	if (s.use_color == -1)
+		s.use_color = git_use_color_default;
+	if (diff_use_color_default == -1)
+		diff_use_color_default = git_use_color_default;
+
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
-		if (s.relative_paths)
-			s.prefix = prefix;
-		if (s.use_color == -1)
-			s.use_color = git_use_color_default;
-		if (diff_use_color_default == -1)
-			diff_use_color_default = git_use_color_default;
 		wt_shortstatus_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
@@ -1033,12 +1034,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
-		if (s.relative_paths)
-			s.prefix = prefix;
-		if (s.use_color == -1)
-			s.use_color = git_use_color_default;
-		if (diff_use_color_default == -1)
-			diff_use_color_default = git_use_color_default;
 		wt_status_print(&s);
 		break;
 	}
diff --git a/wt-status.c b/wt-status.c
index e9bbfbc..55b6696 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -700,5 +700,7 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination)
 void wt_porcelain_print(struct wt_status *s, int null_termination)
 {
 	s->use_color = 0;
+	s->relative_paths = 0;
+	s->prefix = NULL;
 	wt_shortstatus_print(s, null_termination);
 }
-- 
1.6.6.rc1.292.gd8fe.dirty

Re: [PATCH 2/2] status -s: obey color.status

[Michael J Gruber]

Jeff King venit, vidit, dixit 07.12.2009 06:17:
> On Sat, Dec 05, 2009 at 04:04:38PM +0100, Michael J Gruber wrote:
> 
>> Make the short version of status obey the color.status boolean. We color
>> the status letters only, because they carry the state information and are
>> potentially colored differently, such as for a file with staged changes
>> as well as changes in the worktree against the index.
> 
> This seems to also turn on color for --porcelain in some cases, because
> git_status_config unconditionally sets s->use_color if you are using
> color.status instead of color.ui. I think we are probably best just
> explicitly disabling options for the "porcelain" format rather than
> trying to come up with some trickery to make sure they never get set.
> Like:

Thanks. I let myself get fooled by the apparent option handling within
the switch statement (which is OK for relativePaths) (and some
subconscious link between null_termination and porcelain, which goes one
way only).

I guess this shows (again) that one needs tests for everything... If I
get around to I'll amend t$(relevantoneicantrecallrightnow) with tests
for long and short status with color and porcelain with the various options.

Michael