[RFC/PATCH 0/2] status -s: Use the same config as status

[RFC/PATCH 0/2] status -s: Use the same config as status

[Michael J Gruber]

This mini series is an RFC for bringing the short version of status in line
with the longer one. They already both obey status.showUntrackedFiles.
Currently, status -s does not obey status.relativePaths nor
color.status.

I think status should follow the priniciple of least surprise here in
the sense that -s should not change other aspects of the output
(although it is very different internally).

I think we don't have any tests for status -s yet, which is one reason
why this is an RFC: I don't expect to get through with it, like the
introducer of status -s did ;) Another reason is mentioned in 2/2.

Michael J Gruber (2):
  status -s: respect the status.relativePaths option
  status -s: obey color.status

 builtin-commit.c |   22 +++++++++++++++++-----
 wt-status.c      |    2 +-
 wt-status.h      |    1 +
 3 files changed, 19 insertions(+), 6 deletions(-)

[RFC/PATCH 1/2] status -s: respect the status.relativePaths option

[Michael J Gruber]

so that 'status' and 'status -s' in a subdir produce the same file
names.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Anything else would be highly surprising. Note that the porcelain case
is unchanged, of course.

 builtin-commit.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 7f888fe..c103beb 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1113,6 +1113,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
+		if (s.relative_paths)
+			s.prefix = prefix;
 		short_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-- 
1.6.6.rc0.274.g71380

[RFC/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>
---
This is RFC for several reasons:

* Should I rename wt-status.c's color() into something more unique when
  I export it?
* Is there any policy regarding use of putchar/puts vs. printf?
* The way it is done now I "color" a space, otherwise one would need to
  break down the print statements even more. Since we always color the
  foreground only it is no problem, is it?
* Even if I were to write tests for status -s: How do I test colors?

 builtin-commit.c |   20 +++++++++++++++-----
 wt-status.c      |    2 +-
 wt-status.h      |    1 +
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index c103beb..c38fc96 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -999,7 +999,7 @@ static void short_unmerged(int null_termination, struct string_list_item *it,
 	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);
 	} else {
@@ -1016,9 +1016,14 @@ static void short_status(int null_termination, struct string_list_item *it,
 {
 	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
+		printf("  ");
 	if (null_termination) {
 		fprintf(stdout, "%s%c", it->string, 0);
 		if (d->head_path)
@@ -1046,7 +1051,8 @@ static void short_untracked(int null_termination, struct string_list_item *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), "?? ");
+		puts(one);
 		strbuf_release(&onebuf);
 	}
 }
@@ -1115,6 +1121,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;
 		short_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
diff --git a/wt-status.c b/wt-status.c
index 3c2f580..c779b7c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -19,7 +19,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RED,    /* WT_STATUS_UNMERGED */
 };
 
-static const char *color(int slot, struct wt_status *s)
+const char *color(int slot, struct wt_status *s)
 {
 	return s->use_color > 0 ? s->color_palette[slot] : "";
 }
diff --git a/wt-status.h b/wt-status.h
index 09fd9f1..22b15b0 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -55,5 +55,6 @@ struct wt_status {
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
+const char *color(int slot, struct wt_status *s);
 
 #endif /* STATUS_H */
-- 
1.6.6.rc0.274.g71380

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

[Johannes Sixt]

Michael J Gruber schrieb:
> * Is there any policy regarding use of putchar/puts vs. printf?

If the printed string contains color escapes that should be obeyed, you
can use only fputs, printf, and fprintf. You should not use puts or putchar.

> * Even if I were to write tests for status -s: How do I test colors?

See t4034-diff-words.sh.

-- Hannes

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

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 26.11.2009 16:36:
> Michael J Gruber schrieb:
>> * Is there any policy regarding use of putchar/puts vs. printf?
> 
> If the printed string contains color escapes that should be obeyed, you
> can use only fputs, printf, and fprintf. You should not use puts or putchar.

Oh, I don't use them with colors. I use only color_fprintf for that. I
was wondering whether there's a preference of printf("%c", c) over
putchar(c), for example.

>> * Even if I were to write tests for status -s: How do I test colors?
> 
> See t4034-diff-words.sh.

Thanks, I'll keep that in mind for the case that I do have to write test ;)

Michael

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Michael J Gruber schrieb:
>> * Is there any policy regarding use of putchar/puts vs. printf?
>
> If the printed string contains color escapes that should be obeyed, you
> can use only fputs, printf, and fprintf. You should not use puts or putchar.

This msysgit-imposed restriction is something even I do not remember
offhand.  Could you please document it somewhere in a file any developer
would get by checking out the 'master' branch of git.git?

Re: [RFC/PATCH 1/2] status -s: respect the status.relativePaths option

[Junio C Hamano]

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

> so that 'status' and 'status -s' in a subdir produce the same file
> names.

This configuration is on by default which makes this change even more
important.

I'd squash this in and queue it at the tip of jk/1.7.0-status topic.

Thanks


diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 58d35fb..b3dfa42 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -114,8 +114,8 @@ compatibility) and `color.status.<slot>` configuration variables
 to colorize its output.
 
 If the config variable `status.relativePaths` is set to false, then all
-paths shown in the long format are relative to the repository root, not
-to the current directory.
+paths shown are relative to the repository root, not to the current
+directory.
 
 If `status.submodulesummary` is set to a non zero number or true (identical
 to -1 or an unlimited number), the submodule summary will be enabled for

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

[Junio C Hamano]

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

> * Should I rename wt-status.c's color() into something more unique when
>   I export it?

Is it an option to instead move short_unmerged(), short_status() and
friends to wt-status.c from builtin-commit.c?  It's been quite a while
since I worked on the code, so I don't recall why it needs such cross
references at low level between two files.

> * Is there any policy regarding use of putchar/puts vs. printf?

J6t addressed it.  You have mixture of putchar(' ') and printf(" ") which
looks somewhat funny ;-)

> * The way it is done now I "color" a space, otherwise one would need to
>   break down the print statements even more. Since we always color the
>   foreground only it is no problem, is it?

Some people do configure to use "reverse".  For example, I have:

    [diff.color]
            old = red reverse
            whitespace = blue reverse

    [status.color]
            updated = green
            changed = red
            untracked = blue reverse

The output should be consistent between long and short format (I do not
offhand recall what we do for the long format, though).

[PATCH] Add a notice that only certain functions can print color escape codes

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

We emulate color escape codes on Windows by overriding printf, fprintf,
and fputs. Warn users that these are the only functions that can be used
to print them.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Michael J Gruber schrieb:
>>> * Is there any policy regarding use of putchar/puts vs. printf?
>> If the printed string contains color escapes that should be obeyed, you
>> can use only fputs, printf, and fprintf. You should not use puts or putchar.
> 
> This msysgit-imposed restriction is something even I do not remember
> offhand.  Could you please document it somewhere in a file any developer
> would get by checking out the 'master' branch of git.git?

Like this?

 color.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/color.h b/color.h
index 7d8da6f..edeaa3e 100644
--- a/color.h
+++ b/color.h
@@ -4,6 +4,11 @@
 /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
 #define COLOR_MAXLEN 24

+/*
+ * IMPORTANT: Due to the way these color codes are emulated on Windows,
+ * write them only using printf, fprintf, and fputs. In particular,
+ * do not use puts.
+ */
 #define GIT_COLOR_NORMAL	""
 #define GIT_COLOR_RESET		"\033[m"
 #define GIT_COLOR_BOLD		"\033[1m"
-- 
1.6.6.rc0.43.g50037

Re: [RFC/PATCH 1/2] status -s: respect the status.relativePaths option

[Jeff King]

On Thu, Nov 26, 2009 at 04:24:38PM +0100, Michael J Gruber wrote:

> so that 'status' and 'status -s' in a subdir produce the same file
> names.

Thanks, I had been meaning to look into this (who knew it would only be
two lines ;) ).

Now that short and porcelain formats are diverging a bit, we should
probably have a few tests. I think there may already be a bug in

  git commit --dry-run --porcelain

as it looks like commit will use color and relative path configuration
even for the porcelain version.

-Peff

Re: [PATCH] Add a notice that only certain functions can print color escape codes

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Like this?

Yeah, and in addition to "puts", "write(2)" is also not supported, right?

>  color.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/color.h b/color.h
> index 7d8da6f..edeaa3e 100644
> --- a/color.h
> +++ b/color.h
> @@ -4,6 +4,11 @@
>  /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
>  #define COLOR_MAXLEN 24
>
> +/*
> + * IMPORTANT: Due to the way these color codes are emulated on Windows,
> + * write them only using printf, fprintf, and fputs. In particular,
> + * do not use puts.
> + */
>  #define GIT_COLOR_NORMAL	""
>  #define GIT_COLOR_RESET		"\033[m"
>  #define GIT_COLOR_BOLD		"\033[1m"
> -- 
> 1.6.6.rc0.43.g50037

Re: [PATCH] Add a notice that only certain functions can print color escape codes

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Like this?
> 
> Yeah, and in addition to "puts", "write(2)" is also not supported, right?

Correct, good catch!

--- 8< ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] Add a notice that only certain functions can print color escape codes

We emulate color escape codes on Windows by overriding printf, fprintf,
and fputs. Warn users that these are the only functions that can be used
to print them.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 color.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/color.h b/color.h
index 7d8da6f..3cb4b7f 100644
--- a/color.h
+++ b/color.h
@@ -4,6 +4,11 @@
 /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
 #define COLOR_MAXLEN 24

+/*
+ * IMPORTANT: Due to the way these color codes are emulated on Windows,
+ * write them only using printf(), fprintf(), and fputs(). In particular,
+ * do not use puts() or write().
+ */
 #define GIT_COLOR_NORMAL	""
 #define GIT_COLOR_RESET		"\033[m"
 #define GIT_COLOR_BOLD		"\033[1m"
-- 
1.6.6.rc0.43.g50037

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 27.11.2009 06:15:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> * Should I rename wt-status.c's color() into something more unique when
>>   I export it?
> 
> Is it an option to instead move short_unmerged(), short_status() and
> friends to wt-status.c from builtin-commit.c?  It's been quite a while
> since I worked on the code, so I don't recall why it needs such cross
> references at low level between two files.

I didn't notice, but I'll look into it during the rewrite.

>> * Is there any policy regarding use of putchar/puts vs. printf?
> 
> J6t addressed it.  You have mixture of putchar(' ') and printf(" ") which
> looks somewhat funny ;-)

;) I'm happy with printf("c"), I just thought someone would find printf
overkill for a format less char.

>> * The way it is done now I "color" a space, otherwise one would need to
>>   break down the print statements even more. Since we always color the
>>   foreground only it is no problem, is it?
> 
> Some people do configure to use "reverse".  For example, I have:
> 
>     [diff.color]
>             old = red reverse
>             whitespace = blue reverse
> 
>     [status.color]
>             updated = green
>             changed = red
>             untracked = blue reverse
> 
> The output should be consistent between long and short format (I do not
> offhand recall what we do for the long format, though).

Oh, I didn't know about reverse. In this case I have to change the code
and leave the space alone. (The one between ?? and the filename.) Will do.

In the long format, only the file name is colored. Note that in the
short format, it does not make sense to color the file name because one
line may represent two pieces of status information. That is why I color
the two status letters and not the file name.

Michael

Re: [PATCH] Add a notice that only certain functions can print color escape codes

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 27.11.2009 08:42:
> Junio C Hamano schrieb:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>
>>> Like this?
>>
>> Yeah, and in addition to "puts", "write(2)" is also not supported, right?
> 
> Correct, good catch!
> 
> --- 8< ---
> From: Johannes Sixt <j6t@kdbg.org>
> Subject: [PATCH] Add a notice that only certain functions can print color escape codes
> 
> We emulate color escape codes on Windows by overriding printf, fprintf,
> and fputs. Warn users that these are the only functions that can be used
> to print them.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  color.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/color.h b/color.h
> index 7d8da6f..3cb4b7f 100644
> --- a/color.h
> +++ b/color.h
> @@ -4,6 +4,11 @@
>  /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
>  #define COLOR_MAXLEN 24
> 
> +/*
> + * IMPORTANT: Due to the way these color codes are emulated on Windows,
> + * write them only using printf(), fprintf(), and fputs(). In particular,
> + * do not use puts() or write().

Nor putchar nor putc... but they're part of the puts family, of course.
More worthwhile then listing those negatives could be a hint on another
positive: color_fprintf and friends which are declared at the end of
color.h.

> + */
>  #define GIT_COLOR_NORMAL	""
>  #define GIT_COLOR_RESET		"\033[m"
>  #define GIT_COLOR_BOLD		"\033[1m"