Re: [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that

Re: [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that

[Baz]

2009/1/15 Markus Heidelberg <markus.heidelberg@web.de>:
> Adeodato Simó, 13.01.2009:
>> * Boyd Stephen Smith Jr. [Tue, 13 Jan 2009 14:03:11 -0600]:
>>
>> > On Tuesday 2009 January 13 10:45:18 Shawn O. Pearce wrote:
>> > >See [...] how the subject is a niceshort, one
>> > >line summary of the module impacted and the change?
>>
>> > My rule for this is absolutely no more than 80 characters.
>>
>> My rule for *all* of the commit message is "absolutely no more than 76
>> characters". With more than 76, `git log` wraps in a 80-column terminal.
>
> What about the 50 character limit proposed in the documentation
> (git-commit, gittutorial, user-manual)?
>
> At the beginning I tried to fulfil this limit, but often it's not easy.
> So should it be adjusted to a slightly higher value in the documentation
> or even split into a recommended limit (e.g. 50) and a recommended
> absolute maximum (e.g. 76)? Hmm, the split wouldn't make sense, I think.

The 50 character limit is for the first line, try "git log
--pretty=oneline" and it should be obvious why.
The rest of the lines can be longer, 72 is another popular choice.

I'm wondering if the default commit-msg hook should have something like:
perl -ne '$lim = (50,0,72)[($.>3?3:$.)-1];
            chomp;
            if (length($_) > $lim) {
              print STDERR "Line $. of commit message exceeded $lim
characters";
              exit 1;
            }' $1

to more forcefully suggest what's in the manual, like the pre-commit hook does.

(I wish I'd had something like this when one of the devs here pushed a
commit with a 346-line message,
just listing what files he was changing...doh)

-Baz

>
> Markus
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that

[Jeff King]

On Fri, Jan 16, 2009 at 03:06:34PM +0000, Baz wrote:

> > At the beginning I tried to fulfil this limit, but often it's not easy.
> > So should it be adjusted to a slightly higher value in the documentation
> > or even split into a recommended limit (e.g. 50) and a recommended
> > absolute maximum (e.g. 76)? Hmm, the split wouldn't make sense, I think.
> 
> The 50 character limit is for the first line, try "git log
> --pretty=oneline" and it should be obvious why.

I'm not sure it makes sense to make a workflow recommendation that the
git project itself does not follow. Of 14590 non-merge commits in
git.git, 6940 (nearly 50%) have subject lines longer than 50
characters.

In practice, is this a problem for people using git.git?

Personally, I find --pretty=oneline unreadable because so much of the
screen real estate is wasted on random characters that I don't care
about. I find --pretty=tformat:'%h %s' much nicer (yes, --abbrev-commit
works, too, but I find the '...' a pointless waste of space).

For reference, here are the percentiles of subject lines in git.git
longer than X:

 X | %
50 | 48%
60 | 24%
65 | 14%
70 |  8%

So it seems that quite a large chunk are between 50 and 65 characters.
Which still leaves room for "Subject: " or 8 characters of hash at the
beginning.

> perl -ne '$lim = (50,0,72)[($.>3?3:$.)-1];

Ugh. Would a little whitespace have killed you? ;P

> (I wish I'd had something like this when one of the devs here pushed a
> commit with a 346-line message,
> just listing what files he was changing...doh)

Well, yes. That's just insane.

-Peff

Re: [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that

[Markus Heidelberg]

Jeff King, 17.01.2009:
> On Fri, Jan 16, 2009 at 03:06:34PM +0000, Baz wrote:
> 
> > > At the beginning I tried to fulfil this limit, but often it's not easy.
> > > So should it be adjusted to a slightly higher value in the documentation
> > > or even split into a recommended limit (e.g. 50) and a recommended
> > > absolute maximum (e.g. 76)? Hmm, the split wouldn't make sense, I think.
> > 
> > The 50 character limit is for the first line, try "git log
> > --pretty=oneline" and it should be obvious why.
> 
> I'm not sure it makes sense to make a workflow recommendation that the
> git project itself does not follow.

I think, it doesn't.

> In practice, is this a problem for people using git.git?
> 
> Personally, I find --pretty=oneline unreadable because so much of the
> screen real estate is wasted on random characters that I don't care
> about. I find --pretty=tformat:'%h %s' much nicer

But it doesn't automatically color the SHA1.

> (yes, --abbrev-commit
> works, too, but I find the '...' a pointless waste of space).

And they are annoying for selecting with the mouse doubleclick, at least
Konsole selects the SHA1 including the '...'. Xterm doesn't and I
haven't looked if Konsole can be configured to do so, but without the
'...' it would probably work regardless of the terminal emulator and
its setting.

Markus

Re: [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that

[Jeff King]

On Sat, Jan 17, 2009 at 01:37:48PM +0100, Markus Heidelberg wrote:

> > Personally, I find --pretty=oneline unreadable because so much of the
> > screen real estate is wasted on random characters that I don't care
> > about. I find --pretty=tformat:'%h %s' much nicer
> 
> But it doesn't automatically color the SHA1.

True. I was going to suggest %Cyellow, but for some reason only red,
green, and blue are implemented. How silly. Patch series to follow.

-Peff

[PATCH 1/2] color: make it easier for non-config to parse color specs

[Jeff King]

We have very featureful color-parsing routines which are
used for color.diff.* and other options. Let's make it
easier to use those routines from other parts of the code.

This patch adds a color_parse_mem() helper function which
takes a length-bounded string instead of a NUL-terminated
one. While the helper is only a few lines long, it is nice
to abstract this out so that:

 - callers don't forget to free() the temporary buffer

 - right now, it is implemented in terms of color_parse().
   But it would be more efficient to reverse this and
   implement color_parse in terms of color_parse_mem.

This also changes the error string for an invalid color not
to mention the word "config", since it is not always
appropriate (and when it is, the context is obvious since
the offending config variable is given).

Finally, while we are in the area, we clean up the parameter
names in the declaration of color_parse; the var and value
parameters were reversed from the actual implementation.

Signed-off-by: Jeff King <peff@peff.net>
---
Just setup for the next patch.

 color.c |    9 ++++++++-
 color.h |    3 ++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/color.c b/color.c
index fc0b72a..54a3da1 100644
--- a/color.c
+++ b/color.c
@@ -115,7 +115,7 @@ void color_parse(const char *value, const char *var, char *dst)
 	*dst = 0;
 	return;
 bad:
-	die("bad config value '%s' for variable '%s'", value, var);
+	die("bad color value '%s' for variable '%s'", value, var);
 }
 
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
@@ -191,3 +191,10 @@ int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...)
 	va_end(args);
 	return r;
 }
+
+void color_parse_mem(const char *value, int len, const char *var, char *dst)
+{
+	char *tmp = xmemdupz(value, len);
+	color_parse(tmp, var, dst);
+	free(tmp);
+}
diff --git a/color.h b/color.h
index 6cf5c88..7066099 100644
--- a/color.h
+++ b/color.h
@@ -16,7 +16,8 @@ extern int git_use_color_default;
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
-void color_parse(const char *var, const char *value, char *dst);
+void color_parse(const char *value, const char *var, char *dst);
+void color_parse_mem(const char *value, int len, const char *var, char *dst);
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 
-- 
1.6.1.238.g32268.dirty

Re: [PATCH 1/2] color: make it easier for non-config to parse color specs

[René Scharfe]

Jeff King schrieb:
> We have very featureful color-parsing routines which are
> used for color.diff.* and other options. Let's make it
> easier to use those routines from other parts of the code.
> 
> This patch adds a color_parse_mem() helper function which
> takes a length-bounded string instead of a NUL-terminated
> one. While the helper is only a few lines long, it is nice
> to abstract this out so that:
> 
>  - callers don't forget to free() the temporary buffer
> 
>  - right now, it is implemented in terms of color_parse().
>    But it would be more efficient to reverse this and
>    implement color_parse in terms of color_parse_mem.

Thusly?

diff --git a/color.c b/color.c
index fc0b72a..14eac93 100644
--- a/color.c
+++ b/color.c
@@ -41,6 +41,11 @@ static int parse_attr(const char *name, int len)
 
 void color_parse(const char *value, const char *var, char *dst)
 {
+	color_parse_mem(value, strlen(value), var, dst);
+}
+
+void color_parse_mem(const char *value, int len, const char *var, char *dst)
+{
 	const char *ptr = value;
 	int attr = -1;
 	int fg = -2;
@@ -52,18 +57,22 @@ void color_parse(const char *value, const char *var, char *dst)
 	}
 
 	/* [fg [bg]] [attr] */
-	while (*ptr) {
+	while (len > 0) {
 		const char *word = ptr;
-		int val, len = 0;
+		int val, wordlen = 0;
 
-		while (word[len] && !isspace(word[len]))
-			len++;
+		while (len > 0 && !isspace(word[wordlen])) {
+			wordlen++;
+			len--;
+		}
 
-		ptr = word + len;
-		while (*ptr && isspace(*ptr))
+		ptr = word + wordlen;
+		while (len > 0 && isspace(*ptr)) {
 			ptr++;
+			len--;
+		}
 
-		val = parse_color(word, len);
+		val = parse_color(word, wordlen);
 		if (val >= -1) {
 			if (fg == -2) {
 				fg = val;
@@ -75,7 +84,7 @@ void color_parse(const char *value, const char *var, char *dst)
 			}
 			goto bad;
 		}
-		val = parse_attr(word, len);
+		val = parse_attr(word, wordlen);
 		if (val < 0 || attr != -1)
 			goto bad;
 		attr = val;

Re: [PATCH 1/2] color: make it easier for non-config to parse color specs

[Jeff King]

On Sun, Jan 18, 2009 at 06:10:36PM +0100, René Scharfe wrote:

> >  - right now, it is implemented in terms of color_parse().
> >    But it would be more efficient to reverse this and
> >    implement color_parse in terms of color_parse_mem.
> 
> Thusly?

Yes, except for the bugs you introduced. :)

> +void color_parse_mem(const char *value, int len, const char *var, char *dst)
> +{
>  	const char *ptr = value;
>  	int attr = -1;
>  	int fg = -2;

What's missing in the context here (because it wasn't changed) is:

>	if (!strcasecmp(value, "reset")) {
>		strcpy(dst, "\033[m");
>		return;
>	}

which doesn't work, since our string is actually something like
"reset)\0" or even "reset)some totally unrelated string". So we would
need a "memcasecmp" here.

And then in the error case, we call:

> die("bad color value '%s' for variable '%s'", value, var);

which is also bogus.

I don't know if this is really even worth it. The timing difference is
pretty minimal:

  $ time ./git log --pretty=tformat:'%Credfoo%Creset' >/dev/null
  real    0m0.673s
  user    0m0.652s
  sys     0m0.016s
  $ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
  real    0m0.692s
  user    0m0.660s
  sys     0m0.032s

That's about 1 microsecond per commit.

-Peff

Re: [PATCH 1/2] color: make it easier for non-config to parse color specs

[Jeff King]

On Sun, Jan 18, 2009 at 12:28:02PM -0500, Jeff King wrote:

> I don't know if this is really even worth it. The timing difference is
> pretty minimal:

BTW, if we really care about every inch of performance in
--pretty=format:, it would probably make sense to pre-parse the string
into a mini-bytecode. So any complex parsing or lookup that is not
dependent on the commit itself can be done once, instead of per-commit.

I don't think it would make a huge performance difference now, but there
has been talk of more complex substitution syntax (and this %C() is an
example), which would probably benefit.

-Peff

Re: [PATCH 1/2] color: make it easier for non-config to parse color specs

[René Scharfe]

Jeff King schrieb:
> On Sun, Jan 18, 2009 at 06:10:36PM +0100, René Scharfe wrote:
> 
>>>  - right now, it is implemented in terms of color_parse().
>>>    But it would be more efficient to reverse this and
>>>    implement color_parse in terms of color_parse_mem.
>> Thusly?
> 
> Yes, except for the bugs you introduced. :)

Eeek! :)

>> +void color_parse_mem(const char *value, int len, const char *var, char *dst)
>> +{
>>  	const char *ptr = value;
>>  	int attr = -1;
>>  	int fg = -2;
> 
> What's missing in the context here (because it wasn't changed) is:
> 
>> 	if (!strcasecmp(value, "reset")) {
>> 		strcpy(dst, "\033[m");
>> 		return;
>> 	}
> 
> which doesn't work, since our string is actually something like
> "reset)\0" or even "reset)some totally unrelated string". So we would
> need a "memcasecmp" here.

	if (!strncasecmp(value, "reset", len)) {

> And then in the error case, we call:
> 
>> die("bad color value '%s' for variable '%s'", value, var);
> 
> which is also bogus.

   die("bad color value '%.*s' for variable '%s', len, value, var);

> I don't know if this is really even worth it. The timing difference is
> pretty minimal:
> 
>   $ time ./git log --pretty=tformat:'%Credfoo%Creset' >/dev/null
>   real    0m0.673s
>   user    0m0.652s
>   sys     0m0.016s
>   $ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
>   real    0m0.692s
>   user    0m0.660s
>   sys     0m0.032s
> 
> That's about 1 microsecond per commit.

Hmm, not too much overhead, agreed, but it would still be nice to avoid it.

René

Re: [PATCH 1/2] color: make it easier for non-config to parse color specs

[Jeff King]

On Sun, Jan 18, 2009 at 06:45:00PM +0100, René Scharfe wrote:

> > need a "memcasecmp" here.
> 
> 	if (!strncasecmp(value, "reset", len)) {

Thanks. I knew there must be some stock function to do this, but for
some reason I just could not think of strncasecmp.

>    die("bad color value '%.*s' for variable '%s', len, value, var);

Except that we've been shortening "len" during the course of the
function, so it is generally 0 at this point.

> >   $ time ./git log --pretty=tformat:'%Credfoo%Creset' >/dev/null
> >   real    0m0.673s
> >   user    0m0.652s
> >   sys     0m0.016s
> >   $ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
> >   real    0m0.692s
> >   user    0m0.660s
> >   sys     0m0.032s
> > 
> > That's about 1 microsecond per commit.
> 
> Hmm, not too much overhead, agreed, but it would still be nice to avoid it.

Here are the numbers with your fixes:

  $ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
  real    0m0.677s
  user    0m0.668s
  sys     0m0.008s

which puts the difference well into the noise region (I actually did get
one run with your patch that was just as fast as the original).

Here is an updated version of patch 1/2, squashing in your
color_mem_parse update, your follow-on fix, and a fix for the die().

-- >8 --
color: make it easier for non-config to parse color specs

We have very featureful color-parsing routines which are
used for color.diff.* and other options. Let's make it
easier to use those routines from other parts of the code.

This patch converts color_parse to color_parse_mem, taking a
length-bounded string instead of a NUL-terminated one. We
keep color_parse as a wrapper which takes a normal string.
Thanks to René Scharfe for rewriting the color_parse
implementation.

This also changes the error string for an invalid color not
to mention the word "config", since it is not always
appropriate (and when it is, the context is obvious since
the offending config variable is given).

Finally, while we are in the area, we clean up the parameter
names in the declaration of color_parse; the var and value
parameters were reversed from the actual implementation.
---
 color.c |   31 +++++++++++++++++++++----------
 color.h |    3 ++-
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/color.c b/color.c
index fc0b72a..915d7a9 100644
--- a/color.c
+++ b/color.c
@@ -41,29 +41,40 @@ static int parse_attr(const char *name, int len)
 
 void color_parse(const char *value, const char *var, char *dst)
 {
+	color_parse_mem(value, strlen(value), var, dst);
+}
+
+void color_parse_mem(const char *value, int value_len, const char *var,
+		char *dst)
+{
 	const char *ptr = value;
+	int len = value_len;
 	int attr = -1;
 	int fg = -2;
 	int bg = -2;
 
-	if (!strcasecmp(value, "reset")) {
+	if (!strncasecmp(value, "reset", len)) {
 		strcpy(dst, "\033[m");
 		return;
 	}
 
 	/* [fg [bg]] [attr] */
-	while (*ptr) {
+	while (len > 0) {
 		const char *word = ptr;
-		int val, len = 0;
+		int val, wordlen = 0;
 
-		while (word[len] && !isspace(word[len]))
-			len++;
+		while (len > 0 && !isspace(word[wordlen])) {
+			wordlen++;
+			len--;
+		}
 
-		ptr = word + len;
-		while (*ptr && isspace(*ptr))
+		ptr = word + wordlen;
+		while (len > 0 && isspace(*ptr)) {
 			ptr++;
+			len--;
+		}
 
-		val = parse_color(word, len);
+		val = parse_color(word, wordlen);
 		if (val >= -1) {
 			if (fg == -2) {
 				fg = val;
@@ -75,7 +86,7 @@ void color_parse(const char *value, const char *var, char *dst)
 			}
 			goto bad;
 		}
-		val = parse_attr(word, len);
+		val = parse_attr(word, wordlen);
 		if (val < 0 || attr != -1)
 			goto bad;
 		attr = val;
@@ -115,7 +126,7 @@ void color_parse(const char *value, const char *var, char *dst)
 	*dst = 0;
 	return;
 bad:
-	die("bad config value '%s' for variable '%s'", value, var);
+	die("bad color value '%.*s' for variable '%s'", value_len, value, var);
 }
 
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
diff --git a/color.h b/color.h
index 6cf5c88..7066099 100644
--- a/color.h
+++ b/color.h
@@ -16,7 +16,8 @@ extern int git_use_color_default;
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
-void color_parse(const char *var, const char *value, char *dst);
+void color_parse(const char *value, const char *var, char *dst);
+void color_parse_mem(const char *value, int len, const char *var, char *dst);
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 
-- 
1.6.1.266.g3b9d0.dirty

[PATCH 2/2] expand --pretty=format color options

[Jeff King]

Currently, the only colors available to --pretty=format
users are red, green, and blue. Rather than expand it with a
few new colors, this patch makes the usual config color
syntax available, including more colors, backgrounds, and
attributes.

Because colors are no longer bounded to a single word (e.g.,
%Cred), this uses a more advanced syntax that features a
beginning and end delimiter (but the old syntax still
works). So you can now do:

  git log --pretty=tformat:'%C(yellow)%h%C(reset) %s'

to emulate --pretty=oneline, or even

  git log --pretty=tformat:'%C(cyan magenta bold)%s%C(reset)'

if you want to relive the awesomeness of 4-color CGA.

Signed-off-by: Jeff King <peff@peff.net>
---
René: I saw you mention an end goal of implementing the other --pretty
formats in terms of --pretty=format:. So maybe this will help just a
little.

 Documentation/pretty-formats.txt |    1 +
 pretty.c                         |   12 ++++++++++++
 t/t6006-rev-list-format.sh       |    9 ++++++++-
 3 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 0a8a948..3d87d3e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -124,6 +124,7 @@ The placeholders are:
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
 - '%Creset': reset color
+- '%C(...)': color specification, as described in color.branch.* config option
 - '%m': left, right or boundary mark
 - '%n': newline
 - '%x00': print a byte from a hex code
diff --git a/pretty.c b/pretty.c
index 343dca5..b1b8620 100644
--- a/pretty.c
+++ b/pretty.c
@@ -6,6 +6,7 @@
 #include "string-list.h"
 #include "mailmap.h"
 #include "log-tree.h"
+#include "color.h"
 
 static char *user_format;
 
@@ -554,6 +555,17 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
 	case 'C':
+		if (placeholder[1] == '(') {
+			const char *end = strchr(placeholder + 2, ')');
+			char color[COLOR_MAXLEN];
+			if (!end)
+				return 0;
+			color_parse_mem(placeholder + 2,
+					end - (placeholder + 2),
+					"--pretty format", color);
+			strbuf_addstr(sb, color);
+			return end - placeholder + 1;
+		}
 		if (!prefixcmp(placeholder + 1, "red")) {
 			strbuf_addstr(sb, "\033[31m");
 			return 4;
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 86bf7e1..59d1f62 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -14,7 +14,7 @@ touch foo && git add foo && git commit -m "added foo" &&
 test_format() {
 	cat >expect.$1
 	test_expect_success "format $1" "
-git rev-list --pretty=format:$2 master >output.$1 &&
+git rev-list --pretty=format:'$2' master >output.$1 &&
 test_cmp expect.$1 output.$1
 "
 }
@@ -101,6 +101,13 @@ commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
 ^[[31mfoo^[[32mbar^[[34mbaz^[[mxyzzy
 EOF
 
+test_format advanced-colors '%C(red yellow bold)foo%C(reset)' <<'EOF'
+commit 131a310eb913d107dd3c09a65d1651175898735d
+^[[1;31;43mfoo^[[m
+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
+^[[1;31;43mfoo^[[m
+EOF
+
 cat >commit-msg <<'EOF'
 Test printing of complex bodies
 
-- 
1.6.1.238.g32268.dirty

Re: [PATCH 2/2] expand --pretty=format color options

[René Scharfe]

Jeff King schrieb:
> Currently, the only colors available to --pretty=format
> users are red, green, and blue. Rather than expand it with a
> few new colors, this patch makes the usual config color
> syntax available, including more colors, backgrounds, and
> attributes.
> 
> Because colors are no longer bounded to a single word (e.g.,
> %Cred), this uses a more advanced syntax that features a
> beginning and end delimiter (but the old syntax still
> works). So you can now do:
> 
>   git log --pretty=tformat:'%C(yellow)%h%C(reset) %s'
> 
> to emulate --pretty=oneline, or even
> 
>   git log --pretty=tformat:'%C(cyan magenta bold)%s%C(reset)'
> 
> if you want to relive the awesomeness of 4-color CGA.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> René: I saw you mention an end goal of implementing the other --pretty
> formats in terms of --pretty=format:. So maybe this will help just a
> little.

Nice!

Another step would be for --pretty=format to respect the color config,
i.e. it shouldn't print any colour codes if colouring is turned off or
if set to auto while writing to file or pipe.

René

Re: [PATCH 2/2] expand --pretty=format color options

[Jeff King]

On Sun, Jan 18, 2009 at 06:13:21PM +0100, René Scharfe wrote:

> Another step would be for --pretty=format to respect the color config,
> i.e. it shouldn't print any colour codes if colouring is turned off or
> if set to auto while writing to file or pipe.

That makes sense to me. In theory, we could offer an "always use this
color" and a "conditionally use this color" substitution. But I don't
really see why anyone would want the "always use this color" one (they
could just say --color, then).

-Peff

Re: [PATCH 2/2] expand --pretty=format color options

[Jeff King]

On Sun, Jan 18, 2009 at 12:37:53PM -0500, Jeff King wrote:

> On Sun, Jan 18, 2009 at 06:13:21PM +0100, René Scharfe wrote:
> > Another step would be for --pretty=format to respect the color config,
> > i.e. it shouldn't print any colour codes if colouring is turned off or
> > if set to auto while writing to file or pipe.
> 
> That makes sense to me. In theory, we could offer an "always use this
> color" and a "conditionally use this color" substitution. But I don't
> really see why anyone would want the "always use this color" one (they
> could just say --color, then).

Here is a patch that seems to work. It predicates pretty format colors
on diff colors, which is the same way the yellow commit header works in
log-tree. I don't know if it makes more sense to introduce yet another
color config option.

And I say "seems to work" because I remember there being some trickery
with color flags sometimes not getting set properly. However, since this
is the same flag as the yellow commit header, and called around the same
time, I think it should be fine.

One final note: if you are writing "generic" format strings, you
probably don't want to say "yellow". You want to say "whatever the user
has configured for color.diff.commit". I don't know if %C should be
overloaded for that or not (i.e., %C(commit) would be a valid color).

-- >8 --
respect color settings for --pretty=format colors

Previously, we would unconditionally print any colors the
user put into the --pretty=format specifier, regardless of
the setting of color configuration. This is not a problem if
the user writes a custom string each time git-log is
invoked. However, it makes use in scripts unnecessarily
complex, since they would have to change the format string
based on user preferences (and whether output is going to a
tty).

This patch will ignore any colors in --pretty=format if diff
colors are not turned on (they will still be parsed and
treated as placeholders, but no color will be output). This
matches the color.diff.commit colorization, which depends on
diff coloring even though it is not technically part of a
diff.
---
 pretty.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/pretty.c b/pretty.c
index b1b8620..fe606c5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -563,20 +563,25 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 			color_parse_mem(placeholder + 2,
 					end - (placeholder + 2),
 					"--pretty format", color);
-			strbuf_addstr(sb, color);
+			if (diff_use_color_default)
+				strbuf_addstr(sb, color);
 			return end - placeholder + 1;
 		}
 		if (!prefixcmp(placeholder + 1, "red")) {
-			strbuf_addstr(sb, "\033[31m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[31m");
 			return 4;
 		} else if (!prefixcmp(placeholder + 1, "green")) {
-			strbuf_addstr(sb, "\033[32m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[32m");
 			return 6;
 		} else if (!prefixcmp(placeholder + 1, "blue")) {
-			strbuf_addstr(sb, "\033[34m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[34m");
 			return 5;
 		} else if (!prefixcmp(placeholder + 1, "reset")) {
-			strbuf_addstr(sb, "\033[m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[m");
 			return 6;
 		} else
 			return 0;
-- 
1.6.1.151.g5a7da.dirty

Re: [PATCH 2/2] expand --pretty=format color options

[Jeff King]

On Sun, Jan 18, 2009 at 02:43:28PM -0500, Jeff King wrote:

> Here is a patch that seems to work. It predicates pretty format colors
> on diff colors, which is the same way the yellow commit header works in
> log-tree. I don't know if it makes more sense to introduce yet another
> color config option.
> 
> And I say "seems to work" because I remember there being some trickery
> with color flags sometimes not getting set properly. However, since this
> is the same flag as the yellow commit header, and called around the same
> time, I think it should be fine.

Hrm. OK, it doesn't actually work always. It does for git-log, but not
for rev-list, which leaves diff_use_color_default as -1. I don't know if
there are any other ways you can get to this code path without having
set diff_use_color_default.

Maybe it is time to do a cleanup on the color handling, which has
provided no end of these bugs. I will have to leave that for another
day, though.

-Peff

[PATCH 1/2] handle color.ui at a central place

[Markus Heidelberg]

Jeff King, 18.01.2009:
> On Sun, Jan 18, 2009 at 02:43:28PM -0500, Jeff King wrote:
> 
> > Here is a patch that seems to work. It predicates pretty format colors
> > on diff colors, which is the same way the yellow commit header works in
> > log-tree. I don't know if it makes more sense to introduce yet another
> > color config option.
> > 
> > And I say "seems to work" because I remember there being some trickery
> > with color flags sometimes not getting set properly. However, since this
> > is the same flag as the yellow commit header, and called around the same
> > time, I think it should be fine.
> 
> Hrm. OK, it doesn't actually work always. It does for git-log, but not
> for rev-list, which leaves diff_use_color_default as -1. I don't know if
> there are any other ways you can get to this code path without having
> set diff_use_color_default.
> 
> Maybe it is time to do a cleanup on the color handling, which has
> provided no end of these bugs. I will have to leave that for another
> day, though.

Not sure, if you it has something to do with the following, but I had
this in my tree for some days now, waiting for the 2 commits mentioned
in the log message to graduate to master, which happend just an hour or
so ago.

And a good opportunity to test the 8< scissors :)

-- 8< --
The color.ui variable had to be evaluated in the commands that use
colors. This could lead to missing colors, if it was forgotten for
certain git commands. Centralizing the handling of color.ui for branch,
diff and status color avoids this problem and also reduces code
duplication.

See commit 3f4b609 (git-commit: color status output when color.ui is set,
2009-01-08) and commit 38920dd (git-status -v: color diff output when
color.ui is set, 2009-01-08) for fixes of these bugs.

There is a fourth variable color.interactive, but this is currently only
used in git-add-interactive.perl, so there is no need for handling this
as well, so far.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 builtin-branch.c |    4 ----
 builtin-commit.c |    9 ---------
 builtin-diff.c   |    3 ---
 builtin-log.c    |   12 ------------
 builtin-merge.c  |    4 ----
 color.c          |   16 ++++++++++++++++
 color.h          |    2 ++
 config.c         |    9 +++++++--
 8 files changed, 25 insertions(+), 34 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 02fa38f..6aa329b 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -30,7 +30,6 @@ static const char * const builtin_branch_usage[] = {
 static const char *head;
 static unsigned char head_sha1[20];
 
-static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */
 	"",		/* PLAIN (normal) */
@@ -553,9 +552,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	git_config(git_branch_config, NULL);
 
-	if (branch_use_color == -1)
-		branch_use_color = git_use_color_default;
-
 	track = git_branch_track;
 
 	head = resolve_ref("HEAD", head_sha1, 0, NULL);
diff --git a/builtin-commit.c b/builtin-commit.c
index 2f0b00a..5cac034 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -862,12 +862,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	git_config(git_status_config, NULL);
 
-	if (wt_status_use_color == -1)
-		wt_status_use_color = git_use_color_default;
-
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	argc = parse_and_validate_options(argc, argv, builtin_status_usage, prefix);
 
 	index_file = prepare_index(argc, argv, prefix);
@@ -947,9 +941,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	git_config(git_commit_config, NULL);
 
-	if (wt_status_use_color == -1)
-		wt_status_use_color = git_use_color_default;
-
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage, prefix);
 
 	index_file = prepare_index(argc, argv, prefix);
diff --git a/builtin-diff.c b/builtin-diff.c
index d75d69b..d8645cf 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -279,9 +279,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_diff_ui_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 
 	/* If this is a no-index diff, just run it and exit there. */
diff --git a/builtin-log.c b/builtin-log.c
index c7aa48e..84027d4 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -236,9 +236,6 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.simplify_history = 0;
@@ -303,9 +300,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.combine_merges = 1;
@@ -373,9 +367,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	init_reflog_walk(&rev.reflog_info);
 	rev.abbrev_commit = 1;
@@ -406,9 +397,6 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	cmd_log_init(argc, argv, prefix, &rev);
diff --git a/builtin-merge.c b/builtin-merge.c
index cf86975..8e726da 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -872,10 +872,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	git_config(git_merge_config, NULL);
 
-	/* for color.ui */
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	argc = parse_options(argc, argv, builtin_merge_options,
 			builtin_merge_usage, 0);
 	if (verbosity < 0)
diff --git a/color.c b/color.c
index fc0b72a..7a8bf6e 100644
--- a/color.c
+++ b/color.c
@@ -1,9 +1,12 @@
 #include "cache.h"
 #include "color.h"
+#include "diff.h"
+#include "wt-status.h"
 
 #define COLOR_RESET "\033[m"
 
 int git_use_color_default = 0;
+int branch_use_color = -1;
 
 static int parse_color(const char *name, int len)
 {
@@ -155,6 +158,19 @@ int git_color_default_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+void git_finish_color_config(void)
+{
+	if (git_use_color_default) {
+		/* Enable colors, if undefined in the config files */
+		if (branch_use_color == -1)
+			branch_use_color = git_use_color_default;
+		if (diff_use_color_default == -1)
+			diff_use_color_default = git_use_color_default;
+		if (wt_status_use_color == -1)
+			wt_status_use_color = git_use_color_default;
+	}
+}
+
 static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 		va_list args, const char *trail)
 {
diff --git a/color.h b/color.h
index 6cf5c88..6924848 100644
--- a/color.h
+++ b/color.h
@@ -9,12 +9,14 @@
  */
 extern int git_use_color_default;
 
+extern int branch_use_color;
 
 /*
  * Use this instead of git_default_config if you need the value of color.ui.
  */
 int git_color_default_config(const char *var, const char *value, void *cb);
 
+void git_finish_color_config(void);
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
 void color_parse(const char *var, const char *value, char *dst);
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
diff --git a/config.c b/config.c
index 790405a..e35afbc 100644
--- a/config.c
+++ b/config.c
@@ -7,6 +7,7 @@
  */
 #include "cache.h"
 #include "exec_cmd.h"
+#include "color.h"
 
 #define MAXNAME (256)
 
@@ -637,8 +638,10 @@ int git_config(config_fn_t fn, void *data)
 	const char *home = NULL;
 
 	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
-	if (config_exclusive_filename)
-		return git_config_from_file(fn, config_exclusive_filename, data);
+	if (config_exclusive_filename) {
+		ret += git_config_from_file(fn, config_exclusive_filename, data);
+		goto finish;
+	}
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK))
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
@@ -654,6 +657,8 @@ int git_config(config_fn_t fn, void *data)
 	repo_config = git_pathdup("config");
 	ret += git_config_from_file(fn, repo_config, data);
 	free(repo_config);
+ finish:
+	git_finish_color_config();
 	return ret;
 }
 
-- 
1.6.1.208.g3a5f4

[PATCH 2/2] move the color variables to color.c

[Markus Heidelberg]

To be consistent with where the branch color variable is located now,
move the variables for diff and status color to the same place.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 color.c     |    4 ++--
 color.h     |    2 ++
 diff.c      |    1 -
 diff.h      |    1 -
 wt-status.c |    1 -
 wt-status.h |    1 -
 6 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/color.c b/color.c
index 7a8bf6e..b23c39c 100644
--- a/color.c
+++ b/color.c
@@ -1,12 +1,12 @@
 #include "cache.h"
 #include "color.h"
-#include "diff.h"
-#include "wt-status.h"
 
 #define COLOR_RESET "\033[m"
 
 int git_use_color_default = 0;
 int branch_use_color = -1;
+int diff_use_color_default = -1;
+int wt_status_use_color = -1;
 
 static int parse_color(const char *name, int len)
 {
diff --git a/color.h b/color.h
index 6924848..3b47d99 100644
--- a/color.h
+++ b/color.h
@@ -10,6 +10,8 @@
 extern int git_use_color_default;
 
 extern int branch_use_color;
+extern int diff_use_color_default;
+extern int wt_status_use_color;
 
 /*
  * Use this instead of git_default_config if you need the value of color.ui.
diff --git a/diff.c b/diff.c
index d235482..4bd068c 100644
--- a/diff.c
+++ b/diff.c
@@ -22,7 +22,6 @@
 static int diff_detect_rename_default;
 static int diff_rename_limit_default = 200;
 static int diff_suppress_blank_empty;
-int diff_use_color_default = -1;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
diff --git a/diff.h b/diff.h
index 4d5a327..f2c9984 100644
--- a/diff.h
+++ b/diff.h
@@ -188,7 +188,6 @@ extern void diff_unmerge(struct diff_options *,
 
 extern int git_diff_basic_config(const char *var, const char *value, void *cb);
 extern int git_diff_ui_config(const char *var, const char *value, void *cb);
-extern int diff_use_color_default;
 extern void diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int);
 extern int diff_setup_done(struct diff_options *);
diff --git a/wt-status.c b/wt-status.c
index 96ff2f8..5c3742b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -12,7 +12,6 @@
 #include "remote.h"
 
 int wt_status_relative_paths = 1;
-int wt_status_use_color = -1;
 int wt_status_submodule_summary;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
diff --git a/wt-status.h b/wt-status.h
index 78add09..6258fd0 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -36,7 +36,6 @@ struct wt_status {
 };
 
 int git_status_config(const char *var, const char *value, void *cb);
-extern int wt_status_use_color;
 extern int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
-- 
1.6.1.208.g3a5f4

Re: [PATCH 1/2] handle color.ui at a central place

[Jeff King]

On Sun, Jan 18, 2009 at 09:37:15PM +0100, Markus Heidelberg wrote:

> Not sure, if you it has something to do with the following, but I had
> this in my tree for some days now, waiting for the 2 commits mentioned
> in the log message to graduate to master, which happend just an hour or
> so ago.

I think this is probably an improvement, but I had in mind something a
little more drastic. Right now we keep munging one variable that is our
current idea of "should we do color" based on multiple config values.
Then you end up with (best case) this "finalize color config", which is
a bit ugly, or (worst case) bugs where the value hasn't always been
properly initialized (or finalized).

So I think it makes more sense to record each config value, and then
check a _function_ that does the right thing. I.e., you end up with
something like:

  if (use_color(COLOR_DIFF)) /* or COLOR_BRANCH, etc */
    ...

  int use_color(enum color_type t)
  {
    enum color_preference preference;
    preference = color_config[t];
    if (preference == COLOR_UNKNOWN)
      preference = color_config[COLOR_UI];
    if (preference == COLOR_UNKNOWN)
      preference = /* some sane default, possibly command-dependent */
    if (preference == COLOR_AUTO)
      return pager_in_use() || isatty(1);
    if (preference == COLOR_ALWAYS)
      return 1;
    if (preference == COLOR_NEVER)
      return 0;
  }

which very clearly expresses the policy being used (and you probably
want to memo-ize either that whole thing, or at least the isatty check
to avoid making repeated system calls).

-Peff

Re: [PATCH 1/2] handle color.ui at a central place

[Markus Heidelberg]

Jeff King, 20.01.2009:
> On Sun, Jan 18, 2009 at 09:37:15PM +0100, Markus Heidelberg wrote:
> 
> > Not sure, if you it has something to do with the following, but I had
> > this in my tree for some days now, waiting for the 2 commits mentioned
> > in the log message to graduate to master, which happend just an hour or
> > so ago.
> 
> I think this is probably an improvement, but I had in mind something a
> little more drastic. Right now we keep munging one variable that is our
> current idea of "should we do color" based on multiple config values.
> Then you end up with (best case) this "finalize color config", which is
> a bit ugly, or (worst case) bugs where the value hasn't always been

Yes, I didn't find it to be that great either.

> properly initialized (or finalized).
> 
> So I think it makes more sense to record each config value, and then
> check a _function_ that does the right thing. I.e., you end up with
> something like:
>
> [example code snipped]

That would probably be better.

Markus

Re: [PATCH 1/2] handle color.ui at a central place

[Jeff King]

On Wed, Jan 21, 2009 at 11:35:24PM +0100, Markus Heidelberg wrote:

> > properly initialized (or finalized).
> > 
> > So I think it makes more sense to record each config value, and then
> > check a _function_ that does the right thing. I.e., you end up with
> > something like:
> >
> > [example code snipped]
> 
> That would probably be better.

Do you want to work on it, or should it go into the "yeah, right, one
day" section of my todo list?

-Peff

Re: [PATCH 1/2] handle color.ui at a central place

[Markus Heidelberg]

Jeff King, 22.01.2009:
> On Wed, Jan 21, 2009 at 11:35:24PM +0100, Markus Heidelberg wrote:
> 
> > > properly initialized (or finalized).
> > > 
> > > So I think it makes more sense to record each config value, and then
> > > check a _function_ that does the right thing. I.e., you end up with
> > > something like:
> > >
> > > [example code snipped]
> > 
> > That would probably be better.
> 
> Do you want to work on it, or should it go into the "yeah, right, one
> day" section of my todo list?

Yes, feel free to enlarge your todo list :)
There are some other things that I want to work on before, so better
don't count on me for this. But maybe I'll come up to it, before your
todo list pointer reaches this item, who knows.

Markus

Re: [PATCH 1/2] handle color.ui at a central place

[Junio C Hamano]

Markus Heidelberg <markus.heidelberg@web.de> writes:

> Jeff King, 22.01.2009:
> ...
>> > ...
>> > That would probably be better.
>> 
>> Do you want to work on it, or should it go into the "yeah, right, one
>> day" section of my todo list?
>
> Yes, feel free to enlarge your todo list :)
> There are some other things that I want to work on before, so better
> don't count on me for this. But maybe I'll come up to it, before your
> todo list pointer reaches this item, who knows.

Whatever.

I merged and pushed out these two patches but they seem to break
format-patch big time if you have ui.color set to auto.

I will be reverting them out of 'next'.  Grumble.

Re: [PATCH 1/2] handle color.ui at a central place

[Markus Heidelberg]

Junio C Hamano, 23.01.2009:
> Markus Heidelberg <markus.heidelberg@web.de> writes:
> 
> > Jeff King, 22.01.2009:
> > ...
> >> > ...
> >> > That would probably be better.
> >> 
> >> Do you want to work on it, or should it go into the "yeah, right, one
> >> day" section of my todo list?
> >
> > Yes, feel free to enlarge your todo list :)
> > There are some other things that I want to work on before, so better
> > don't count on me for this. But maybe I'll come up to it, before your
> > todo list pointer reaches this item, who knows.
> 
> Whatever.
> 
> I merged and pushed out these two patches but they seem to break
> format-patch big time if you have ui.color set to auto.
> 
> I will be reverting them out of 'next'.  Grumble.

Damn, sorry for this.

I looked at the code and found this in git_format_config():

	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
		return 0;

Which of course didn't handle color.ui, but that wasn't necessary before
the central color.ui handling from my patch.

So with the following diff it works:

-	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
+	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")
+				       || !strcmp(var, "color.ui")) {

Compared to f3aafa4 (Disable color detection during format-patch,
2006-07-09) and a159ca0 (Allow subcommand.color and color.subcommand
color configuration, 2006-12-13), which introduced !strcmp(var,
"diff.color") resp. !strcmp(var, "color.diff") at this place.

Or with this, which however would be a similar thing to what I tried to
remove from the code.

        git_config(git_format_config, NULL);
+       diff_use_color_default = 0;

format-patch is perhaps the only place where the commit has broken
things, because I didn't find other places, where color config options
were set, but not the corresponding variables. So it seems as if only
format-patch needed code like this to turn off the colors.

Markus

Re: [PATCH 1/2] handle color.ui at a central place

[Johannes Schindelin]

Hi,

On Sat, 24 Jan 2009, Markus Heidelberg wrote:

> format-patch is perhaps the only place where the commit has broken 
> things, because I didn't find other places, where color config options 
> were set, but not the corresponding variables. So it seems as if only 
> format-patch needed code like this to turn off the colors.

So you want to add a test case when you resubmit your patch...

Ciao,
Dscho

Re: [PATCH 1/2] handle color.ui at a central place

[Markus Heidelberg]

Johannes Schindelin, 24.01.2009:
> Hi,
> 
> On Sat, 24 Jan 2009, Markus Heidelberg wrote:
> 
> > format-patch is perhaps the only place where the commit has broken 
> > things, because I didn't find other places, where color config options 
> > were set
(and evaluated)
> > , but not the corresponding variables. So it seems as if only 
> > format-patch needed code like this to turn off the colors.
> 
> So you want to add a test case when you resubmit your patch...

I'm not sure, whether it should be resubmitted at all. As Jeff pointed
out, there should be a better way to clean up the color (and color.ui)
handling.

Markus

Re: [PATCH 1/2] handle color.ui at a central place

[Junio C Hamano]

Markus Heidelberg <markus.heidelberg@web.de> writes:

> I looked at the code and found this in git_format_config():
>
> 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
> 		return 0;
>
> Which of course didn't handle color.ui, but that wasn't necessary before
> the central color.ui handling from my patch.

Centralized handling is never a goal in itself.  The goal should be to
make it easier for various codepaths to use color settings correctly,
without having to have many special case workarounds.  Centralized
handling, if designed right, could be a good way to achieve that goal.

> So with the following diff it works:
>
> -	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
> +	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")
> +				       || !strcmp(var, "color.ui")) {

Why should format-patch need to even worry about protecting itself from
"color.ui" to begin with?

If your patch is making color handling saner, I would expect that
format-patch can *lose* the existing "ignore diff.color or color.diff"
workaround as a result of that.  If you need to add even *more* workaround
code like that, there's something wrong, don't you think?

> format-patch is perhaps the only place where the commit has broken
> things, because I didn't find other places,...

You did not find the breakage in format-patch either to begin with; so
your not finding does not give us much confidence that there is no other
breakage, does it?

Grumble...

Re: [PATCH 1/2] handle color.ui at a central place

[Jeff King]

On Sat, Jan 24, 2009 at 10:36:24AM -0800, Junio C Hamano wrote:

> Why should format-patch need to even worry about protecting itself from
> "color.ui" to begin with?

Agreed. In the "should I use color" function I proposed, there should be
a big fat "are_we_a_porcelain_that_will_allow_any_color_at_all" flag
at the top, which will make it totally clear how to make sure color is
off.

> You did not find the breakage in format-patch either to begin with; so
> your not finding does not give us much confidence that there is no other
> breakage, does it?
> 
> Grumble...

Sadly, this is an area that is not covered very well in the tests
(partially, I think, because it is "just" output which we tend to
neglect, and partially because the isatty() stuff is hard to test with
our harness). So I don't think it's _entirely_ Markus' fault.

-Peff

Re: [PATCH 1/2] handle color.ui at a central place

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Jan 24, 2009 at 10:36:24AM -0800, Junio C Hamano wrote:
> ...
>> You did not find the breakage in format-patch either to begin with; so
>> your not finding does not give us much confidence that there is no other
>> breakage, does it?
>> 
>> Grumble...
>
> Sadly, this is an area that is not covered very well in the tests
> (partially, I think, because it is "just" output which we tend to
> neglect, and partially because the isatty() stuff is hard to test with
> our harness). So I don't think it's _entirely_ Markus' fault.

Oh, don't get me wrong.  I am not interested in finding whose fault it
was.  I was just stating the fact that one person not finding a breakage
does not mean much as an assurance.

It is actually not trivial to test this breakage in our test suite.
Before committing 9383af1 (Revert previous two commits, 2009-01-23), I
spent about 20 minutes trying to come up with a test to expose the
breakage in an acceptable way.  A test that assumes that it is run with a
controlling terminal is relatively easy to write, but I couldn't come up
with a test that would have triggered even when the tests were run without
a tty (for gory details, see git_config_colorbool() and how stdout_is_tty
is used).

Here is the "relatively easy" but an unacceptable one.

diff --git c/t/t4014-format-patch.sh w/t/t4014-format-patch.sh
index 9d99dc2..609946a 100755
--- c/t/t4014-format-patch.sh
+++ w/t/t4014-format-patch.sh
@@ -255,4 +255,10 @@ test_expect_success 'format-patch respects -U' '
 
 '
 
+test_expect_success 'format-patch is colorless even with color.ui = auto' '
+	git config color.ui auto &&
+	TERM=ansi git format-patch -1 >/dev/tty &&
+	grep "^+5$" 0001-foo.patch
+'
+
 test_done

Points that makes the above patch unacceptable are:

 (1) It hardcodes 0001-foo.patch.  You could try doing these:

     (1.1) patchname=$( ... git format-patch -1) && grep ... <"$patchname"
     (1.2) git format-patch -1 --stdout >patchfile && grep ... <patchfile

     but they won't work, because "color.ui = auto" will not color unless
     the standard output is a tty, and TERM is better than "dumb".

 (2) It would not trigger if /dev/tty cannot be opened for writing.

     

Re: [PATCH 1/2] handle color.ui at a central place

[Jeff King]

On Sat, Jan 24, 2009 at 12:26:50PM -0800, Junio C Hamano wrote:

> > Sadly, this is an area that is not covered very well in the tests
> > (partially, I think, because it is "just" output which we tend to
> > neglect, and partially because the isatty() stuff is hard to test with
> > our harness). So I don't think it's _entirely_ Markus' fault.
> 
> Oh, don't get me wrong.  I am not interested in finding whose fault it
> was.  I was just stating the fact that one person not finding a breakage
> does not mean much as an assurance.

OK. I was just trying to encourage Markus to keep trying. ;)

> +test_expect_success 'format-patch is colorless even with color.ui = auto' '
> +	git config color.ui auto &&
> +	TERM=ansi git format-patch -1 >/dev/tty &&
> +	grep "^+5$" 0001-foo.patch
> +'
> +

Actually, could this not just be "format-patch is colorless even with
color.ui = always"? I.e., shouldn't format-patch _always_ not have
color, no matter what (and we assume that "always" is a superset of
"auto"). And then it is also much easier to test.

Of course that doesn't fix the fact that it would _also_ be nice to test
"auto" functionality in general. Here you use the fact that format-patch
sends output to a file. But I don't think there is a good way to test
that "git log --color=auto" works as expected.

-Peff

Re: [PATCH 1/2] handle color.ui at a central place

[Markus Heidelberg]

Junio C Hamano, 24.01.2009:
> Markus Heidelberg <markus.heidelberg@web.de> writes:
> 
> > So with the following diff it works:
> >
> > -	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
> > +	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")
> > +				       || !strcmp(var, "color.ui")) {
> 
> Why should format-patch need to even worry about protecting itself from
> "color.ui" to begin with?

That's the reason, why color handling needs another rework than my
patch, which only was originated from the color.ui git_use_color_default
workarounds. Call it shortsighted, if you want.

> If your patch is making color handling saner, I would expect that
> format-patch can *lose* the existing "ignore diff.color or color.diff"
> workaround as a result of that.  If you need to add even *more* workaround
> code like that, there's something wrong, don't you think?

That's the reason, why it doesn't make sense to continue work on my
patch base.

> > format-patch is perhaps the only place where the commit has broken
> > things, because I didn't find other places,...
> 
> You did not find the breakage in format-patch either to begin with; so
> your not finding does not give us much confidence that there is no other
> breakage, does it?

Of course not.

> Grumble...

Why grumble? That was just a personal suspicion. I didn't say: "I think
this is the only breakage place, please reapply".

Re: [PATCH 2/2] expand --pretty=format color options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hrm. OK, it doesn't actually work always. It does for git-log, but not
> for rev-list, which leaves diff_use_color_default as -1. I don't know if
> there are any other ways you can get to this code path without having
> set diff_use_color_default.

Yuck, no matter what you do please don't contaminate plumbing with the UI
color options.

> Maybe it is time to do a cleanup on the color handling, which has
> provided no end of these bugs. I will have to leave that for another
> day, though.

Re: [PATCH 2/2] expand --pretty=format color options

[Jeff King]

On Mon, Jan 19, 2009 at 03:10:56PM -0800, Junio C Hamano wrote:

> > Hrm. OK, it doesn't actually work always. It does for git-log, but not
> > for rev-list, which leaves diff_use_color_default as -1. I don't know if
> > there are any other ways you can get to this code path without having
> > set diff_use_color_default.
> 
> Yuck, no matter what you do please don't contaminate plumbing with the UI
> color options.

Of course. But the problem is that rev-list is _already_ contaminated by
--pretty=format:%Cred. Or do you mean, you really want rev-list to
unconditionally output color in such a case?

-Peff

Re: [PATCH 2/2] expand --pretty=format color options

[Johannes Schindelin]

Hi,

On Mon, 19 Jan 2009, Jeff King wrote:

> On Mon, Jan 19, 2009 at 03:10:56PM -0800, Junio C Hamano wrote:
> 
> > > Hrm. OK, it doesn't actually work always. It does for git-log, but 
> > > not for rev-list, which leaves diff_use_color_default as -1. I don't 
> > > know if there are any other ways you can get to this code path 
> > > without having set diff_use_color_default.
> > 
> > Yuck, no matter what you do please don't contaminate plumbing with the 
> > UI color options.
> 
> Of course. But the problem is that rev-list is _already_ contaminated by 
> --pretty=format:%Cred. Or do you mean, you really want rev-list to 
> unconditionally output color in such a case?

No, rev-list is not contaminated with UI color options.  %Cred _always_ 
outputs the color, even when the user turned off color explicitely, using 
--no-color.

Ciao,
Dscho

Re: [PATCH 2/2] expand --pretty=format color options

[Johannes Schindelin]

Hi,

On Tue, 20 Jan 2009, Johannes Schindelin wrote:

> On Mon, 19 Jan 2009, Jeff King wrote:
> 
> > On Mon, Jan 19, 2009 at 03:10:56PM -0800, Junio C Hamano wrote:
> > 
> > > > Hrm. OK, it doesn't actually work always. It does for git-log, but 
> > > > not for rev-list, which leaves diff_use_color_default as -1. I 
> > > > don't know if there are any other ways you can get to this code 
> > > > path without having set diff_use_color_default.
> > > 
> > > Yuck, no matter what you do please don't contaminate plumbing with 
> > > the UI color options.
> > 
> > Of course. But the problem is that rev-list is _already_ contaminated 
> > by --pretty=format:%Cred. Or do you mean, you really want rev-list to 
> > unconditionally output color in such a case?
> 
> No, rev-list is not contaminated with UI color options.  %Cred _always_ 
> outputs the color, even when the user turned off color explicitely, 
> using --no-color.

BTW I would find it very logical for rev-list not to output any color at 
all when %C(yellow) is specified, as your code respects the diff UI 
options, which are implicitly turned off for rev-list (as rev-list is no 
UI), just like the coloring of "commit <name>" is implicitly turned off 
for rev-list.

Ciao,
Dscho

Re: [PATCH 2/2] expand --pretty=format color options

[Jeff King]

On Tue, Jan 20, 2009 at 11:36:08AM +0100, Johannes Schindelin wrote:

> > > Of course. But the problem is that rev-list is _already_ contaminated 
> > > by --pretty=format:%Cred. Or do you mean, you really want rev-list to 
> > > unconditionally output color in such a case?
> > 
> > No, rev-list is not contaminated with UI color options.  %Cred _always_ 
> > outputs the color, even when the user turned off color explicitely, 
> > using --no-color.
> 
> BTW I would find it very logical for rev-list not to output any color at 
> all when %C(yellow) is specified, as your code respects the diff UI 
> options, which are implicitly turned off for rev-list (as rev-list is no 
> UI), just like the coloring of "commit <name>" is implicitly turned off 
> for rev-list.

Now I'm confused. Should color in --pretty=format always be on, or
should it respect color settings? You seem to be advocating both sides
in the two paragraphs.

The behavior I would propose it along the lines of:

 - plumbing _always_ has color off

 - porcelain respects color.* config, --color, etc

-Peff

Re: [PATCH 2/2] expand --pretty=format color options

[Johannes Schindelin]

Hi,

On Tue, 20 Jan 2009, Jeff King wrote:

> On Tue, Jan 20, 2009 at 11:36:08AM +0100, Johannes Schindelin wrote:
> 
> > > > Of course. But the problem is that rev-list is _already_ contaminated 
> > > > by --pretty=format:%Cred. Or do you mean, you really want rev-list to 
> > > > unconditionally output color in such a case?
> > > 
> > > No, rev-list is not contaminated with UI color options.  %Cred _always_ 
> > > outputs the color, even when the user turned off color explicitely, 
> > > using --no-color.
> > 
> > BTW I would find it very logical for rev-list not to output any color at 
> > all when %C(yellow) is specified, as your code respects the diff UI 
> > options, which are implicitly turned off for rev-list (as rev-list is no 
> > UI), just like the coloring of "commit <name>" is implicitly turned off 
> > for rev-list.
> 
> Now I'm confused. Should color in --pretty=format always be on, or
> should it respect color settings? You seem to be advocating both sides
> in the two paragraphs.

No, I am just very bad at relating my thoughts.

What I tried to say is "plumbing does not, and should not, change behavior 
depending on diff.color".

With %Cred, I was just lazy, and did not make a check if diff.color is 
true, which I regret now.

But then, its behavior still does not depend on diff.color when using 
plumbing.

It does not even depend on it when using porcelain :-)

> The behavior I would propose it along the lines of:
> 
>  - plumbing _always_ has color off
> 
>  - porcelain respects color.* config, --color, etc

Right, that'd be the sane behavior, even for %Cred.

Ciao,
Dscho

Re: [PATCH 2/2] expand --pretty=format color options

[Jeff King]

On Tue, Jan 20, 2009 at 03:58:29PM +0100, Johannes Schindelin wrote:

> > The behavior I would propose it along the lines of:
> > 
> >  - plumbing _always_ has color off
> > 
> >  - porcelain respects color.* config, --color, etc
> 
> Right, that'd be the sane behavior, even for %Cred.

OK, good. I think our disagreement was just confused wording on both
sides, then.

-Peff

Cyellow, was Re: [a way-too-long line]

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Jeff King wrote:

> True. I was going to suggest %Cyellow, but for some reason only red, 
> green, and blue are implemented. How silly. Patch series to follow.

http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=af6ee188b3533e9c8eb665066ed0be32d58be875

Ciao,
Dscho

Re: Cyellow, was Re: [a way-too-long line]

[Jeff King]

On Sat, Jan 17, 2009 at 04:39:10PM +0100, Johannes Schindelin wrote:

> > True. I was going to suggest %Cyellow, but for some reason only red, 
> > green, and blue are implemented. How silly. Patch series to follow.
> 
> http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=af6ee188b3533e9c8eb665066ed0be32d58be875

:) Yes, that does work, but please see the series I just posted:

  http://article.gmane.org/gmane.comp.version-control.git/106062

which I think is a better solution.

-Peff

Re: Cyellow, was Re: [a way-too-long line]

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Jeff King wrote:

> On Sat, Jan 17, 2009 at 04:39:10PM +0100, Johannes Schindelin wrote:
> 
> > > True. I was going to suggest %Cyellow, but for some reason only red, 
> > > green, and blue are implemented. How silly. Patch series to follow.
> > 
> > http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=af6ee188b3533e9c8eb665066ed0be32d58be875
> 
> :) Yes, that does work, but please see the series I just posted:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/106062
> 
> which I think is a better solution.

Definitely.

Mail from the list seems to be lagging these days... Sigh.

Ciao,
Dscho