[PATCH] log --format: teach %C(auto,black) to paint it black only on terminals
Traditionally, %C(color attr) always emitted the ANSI color
sequence; it was up to the scripts that wanted to conditionally
color their output to omit %C(...) specifier when they do not want
colors.
Optionally allow auto, to be prefixed to the color, so that the
output is colored iff it goes to the terminal.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* This time with minimum test and documentation.
Documentation/pretty-formats.txt | 4 +++-
pretty.c | 13 ++---
t/t6006-rev-list-format.sh | 30 ++
3 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d9edded..2af017c 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -144,7 +144,9 @@ 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
+- '%C(...)': color specification, as described in color.branch.* config option;
+ adding `auto,` at the beginning will emit color only when the
+ output is going to a terminal
- '%m': left, right or boundary mark
- '%n': newline
- '%%': a raw '%'
diff --git a/pretty.c b/pretty.c
index dba6828..b9fd972 100644
--- a/pretty.c
+++ b/pretty.c
@@ -960,12 +960,19 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
switch (placeholder[0]) {
case 'C':
if (placeholder[1] == '(') {
- const char *end = strchr(placeholder + 2, ')');
+ const char *begin = placeholder + 2;
+ const char *end = strchr(begin, ')');
char color[COLOR_MAXLEN];
+
if (!end)
return 0;
- color_parse_mem(placeholder + 2,
- end - (placeholder + 2),
+ if (!memcmp(begin, auto,, 5)) {
+ if (!want_color(-1))
+ return end - placeholder + 1;
+ begin += 5;
+ }
+ color_parse_mem(begin,
+ end - begin,
--pretty format, color);
strbuf_addstr(sb, color);
return end - placeholder + 1;
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index f94f0c4..bfcc1c6 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -11,12 +11,12 @@ touch foo git add foo git commit -m added foo
'
# usage: test_format name format_string expected_output
-test_format() {
+test_format () {
cat expect.$1
test_expect_success format $1
-git rev-list --pretty=format:'$2' master output.$1
-test_cmp expect.$1 output.$1
-
+ git rev-list --pretty=format:'$2'${3:+ $3} master output.$1
+ test_cmp expect.$1 output.$1
+
}
test_format percent %%h 'EOF'
@@ -124,6 +124,28 @@ commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
[1;31;43mfoo[m
EOF
+test_format advanced-colors-auto \
+ '%C(auto,red yellow bold)foo%C(auto,reset)' --color 'EOF'
+commit 131a310eb913d107dd3c09a65d1651175898735d
+foo
+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
+foo
+EOF
+
+# %C(auto,...) should trump --color=always
+#
+# NEEDSWORK: --color=never should also be tested but we need to run a
+# similar test under pseudo-terminal with test_terminal which is too
+# much hassle for its worth.
+
+test_format advanced-colors-forced \
+ '%C(auto,red yellow bold)foo%C(auto,reset)' --color=always 'EOF'
+commit 131a310eb913d107dd3c09a65d1651175898735d
+foo
+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
+foo
+EOF
+
cat commit-msg 'EOF'
Test printing of complex bodies
--
1.8.1.rc2.126.ge9b7782
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log --format: teach %C(auto,black) to paint it black only on terminals
On Mon, Dec 17, 2012 at 3:40 PM, Junio C Hamano gits...@pobox.com wrote:
Traditionally, %C(color attr) always emitted the ANSI color
sequence; it was up to the scripts that wanted to conditionally
color their output to omit %C(...) specifier when they do not want
colors.
Optionally allow auto, to be prefixed to the color, so that the
output is colored iff it goes to the terminal.
I see prefix is clearly documented. Still it feels a bit unnatural
that %C(red,auto) won't work. But we can make that case work later if
somebody cares enough.
if (!end)
return 0;
- color_parse_mem(placeholder + 2,
- end - (placeholder + 2),
+ if (!memcmp(begin, auto,, 5)) {
+ if (!want_color(-1))
+ return end - placeholder + 1;
This want_color() checks color.ui and only when color.ui = auto, it
bothers to check if the output is tty. I think the document should say
that auto, (or maybe another name because it's not really auto)
respects color.ui.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log --format: teach %C(auto,black) to paint it black only on terminals
Jeff King p...@peff.net writes:
On Mon, Dec 17, 2012 at 06:44:10PM +0700, Nguyen Thai Ngoc Duy wrote:
if (!end)
return 0;
- color_parse_mem(placeholder + 2,
- end - (placeholder + 2),
+ if (!memcmp(begin, auto,, 5)) {
+ if (!want_color(-1))
+ return end - placeholder + 1;
This want_color() checks color.ui and only when color.ui = auto, it
bothers to check if the output is tty. I think the document should say
that auto, (or maybe another name because it's not really auto)
respects color.ui.
Yeah, that should definitely be documented. I wonder if it should
actually respect color.diff, which is what log usually uses (albeit
mostly for the diff itself, we have always used it for the graph and for
the commit header of each entry).
I actually do not like this patch very much. The original motive
behind this auto thing was to relieve the script writers from
the burden of having to write:
if tty -s
then
warn='%C(red)'
reset='%C(reset)'
else
warn= reset=
fi
fmt=${warn}WARNING: boo${reset} %s
and instead let them write:
fmt=%C(auto,red)WARNING: boo%C(auto,reset) %s
but between the two, there is no $cmd.color configuration involved
in the first place. I am not sure what $cmd.color configuration
should take effect---perhaps for a git frotz script, we should
allow the script writer to honor frotz.color=(auto,never,always)
configuration, not just ui.color variable.
Also the patch as posted deliberately omits support to honor command
line override --color=(auto,never,always), but it may be more
natural to expect
git show --format='%C(auto,red)%s%C(auto,reset)' --color=never
to defeat the auto, part the script writer wrote.
Now, such a script would be run by its end users as
$ git frotz --color=never
It has to do its own option parsing before running the underlying
git show to decide if it passes --color=never from the command
line for that to happen.
But at that point, we are back to the square one. Such a script
would be doing something like:
if cmdline_has_color_flag
then
use_color=... that flag ...
elif git config --get-colorbool frotz.color
then
use_color=--color=always
else
use_color=--color=never
fi
in its early part to decide $use_color, to be used in the call it
makes to git show later on:
git show --format=$fmt $use_color
Adding the logic to decide if %C(...) should be added to $fmt no
longer is an additional burden to the script writer, making the
whole %C(auto,red) machinery of little use.
So...
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log --format: teach %C(auto,black) to paint it black only on terminals
On Mon, Dec 17, 2012 at 11:34:48AM -0800, Junio C Hamano wrote:
Yeah, that should definitely be documented. I wonder if it should
actually respect color.diff, which is what log usually uses (albeit
mostly for the diff itself, we have always used it for the graph and for
the commit header of each entry).
I actually do not like this patch very much. The original motive
behind this auto thing was to relieve the script writers from
the burden of having to write:
if tty -s
then
warn='%C(red)'
reset='%C(reset)'
else
warn= reset=
fi
fmt=${warn}WARNING: boo${reset} %s
and instead let them write:
fmt=%C(auto,red)WARNING: boo%C(auto,reset) %s
but between the two, there is no $cmd.color configuration involved
in the first place. I am not sure what $cmd.color configuration
should take effect---perhaps for a git frotz script, we should
allow the script writer to honor frotz.color=(auto,never,always)
configuration, not just ui.color variable.
Since this is by definition just talking about the log traversal, I
think it makes sense to respect the log traversal option by default
(which is confusingly color.diff, of course, but that is a separate
issue). That means that scripts which just wrap a regular traversal will
do what the user is accustomed to.
If git frotz wants to have a separate color.frotz option to override
that, then they would need to implement that themselves either with or
without your patch. I do not think its presence makes things any harder.
Also the patch as posted deliberately omits support to honor command
line override --color=(auto,never,always), but it may be more
natural to expect
git show --format='%C(auto,red)%s%C(auto,reset)' --color=never
to defeat the auto, part the script writer wrote.
Now, such a script would be run by its end users as
$ git frotz --color=never
It has to do its own option parsing before running the underlying
git show to decide if it passes --color=never from the command
line for that to happen.
Yeah, _if_ it does not just pass its options directly to git-log. Which
I think a reasonable number of scripts do, as well as pretty.* aliases
(which are not really scripts, but have the same relationship with
respect to this feature). For example, git stash list does not use
color in its output, but could be improved to do so after your patch.
So no, I do not think you can cover every conceivable case. But having
git-log respect --color and the usual color.* variables for this feature
seems like the only sane default. It makes the easy cases just work, and
the hard cases are not any worse off (and they may even be better off,
since the script can manipulate --color instead of rewriting their
format strings, but that is a minor difference).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log --format: teach %C(auto,black) to paint it black only on terminals
Jeff King p...@peff.net writes:
If git frotz wants to have a separate color.frotz option to override
that, then they would need to implement that themselves either with or
without your patch. I do not think its presence makes things any harder.
That _was_ (but no longer is) exactly my point. Eh, rather, its
absense does not make things any harder.
So no, I do not think you can cover every conceivable case. But having
git-log respect --color and the usual color.* variables for this feature
seems like the only sane default. It makes the easy cases just work, and
the hard cases are not any worse off (and they may even be better off,
since the script can manipulate --color instead of rewriting their
format strings, but that is a minor difference).
OK, care to reroll the one with your patch in the other message
squashed in, possibly with fixes to the test (the result should now
honor --color={always,never}, I think)?
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log --format: teach %C(auto,black) to paint it black only on terminals
On Mon, Dec 17, 2012 at 12:03:40PM -0800, Junio C Hamano wrote:
So no, I do not think you can cover every conceivable case. But having
git-log respect --color and the usual color.* variables for this feature
seems like the only sane default. It makes the easy cases just work, and
the hard cases are not any worse off (and they may even be better off,
since the script can manipulate --color instead of rewriting their
format strings, but that is a minor difference).
OK, care to reroll the one with your patch in the other message
squashed in, possibly with fixes to the test (the result should now
honor --color={always,never}, I think)?
Here it is. It was easier to just skip using test_format, so it did not
need to be touched. I broke its cleanups out into a separate patch.
[1/2]: t6006: clean up whitespace
[2/2]: log --format: teach %C(auto,black) to respect color config
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
