threads / patch / 21870

patch, 2 partsRefactor status producers and make status -s use color

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

## tl;dr

7 messages between Dec 5, 2009 and Dec 7, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Michael J Gruber· Dec 5, 2009, 15:04 UTC · lore

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(-)
Michael J Gruber· Dec 5, 2009, 15:04 UTC · re: Michael J Gruber · lore

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

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

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

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
Junio C Hamano· Dec 7, 2009, 00:30 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 13 quoted lines
> 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.
Michael J Gruber· Dec 5, 2009, 15:04 UTC · re: Michael J Gruber · lore

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

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

builtin-commit.c, wt-status.c

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
Jeff King· Dec 7, 2009, 05:17 UTC · re: Michael J Gruber · lore

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

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

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

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
Jeff King· Dec 7, 2009, 05:26 UTC · re: Jeff King · lore

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

On Mon, Dec 07, 2009 at 12:17:15AM -0500, Jeff King wrote:
Show 5 quoted lines
> 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(-)
Show changes to 2 files +9 −12

builtin-commit.c, wt-status.c

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
Michael J Gruber· Dec 7, 2009, 08:33 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 07.12.2009 06:17:
Show 13 quoted lines
> 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

← back to recent threads