Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color
Junio C Hamano gits...@pobox.com wrote: $ git show -s --format='%CredAnd%CyellowAreNotTheSameColor' Ouch, this is quite a disaster. It would have been much saner if we started from %(color:yellow), %(subject), etc., i.e. have a single long-hand magic introducer %(...), and added a set of often-used short-hands like %s. I am not opposed to unify the internal implementations and the external interfaces of pretty, for-each-ref and friends, but modelling the external UI after the mnemonic only with ad hoc additions mess the pretty-format uses is a huge mistake. Okay, I'm convinced; I'll rework the series to do %(color:...) and resubmit shortly. 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 1/3] for-each-ref: introduce %C(...) for color
Junio C Hamano wrote: If %(authordate) is I want to see the author date here, and %(authordate:short) is I want to see the author date here in the short form, you would expect I want colored output in green to be spelled as %(color:green), or something, perhaps? Last time, we almost managed a unification: tokens like %authordate in f-e-r correspond to tokens like %ae in pretty-formats; %C(...) is different in that it doesn't actually output anything, but changes the color of tokens following it. While I'm not opposed to %(color:...), I would prefer a color syntax that is different from other-token syntax, like in pretty-formats. -- 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 1/3] for-each-ref: introduce %C(...) for color
Ramkumar Ramachandra artag...@gmail.com writes: ... %C(...) is different in that it doesn't actually output anything, but changes the color of tokens following it. While I'm not opposed to %(color:...), I would prefer a color syntax that is different from other-token syntax, like in pretty-formats. You may prefer it, but I do not see why users prefer to memorize that a magic that consumes no display output columns uses a syntax different from all the other magic introducers that follows %(name of the magic with string after colon to give more specifics to the magic) syntax. In all honesty, the %XY mnemonic syntax in pretty-format is a syntactic disaster. It is perfectly OK to have a set of often used shorthand, but because we started without a consistent long-hand, we ended up with %Cred and %C(yellow), leading us to a nonsense like this (try it yourself and weep): $ git show -s --format='%CredAnd%CyellowAreNotTheSameColor' It would have been much saner if we started from %(color:yellow), %(subject), etc., i.e. have a single long-hand magic introducer %(...), and added a set of often-used short-hands like %s. I am not opposed to unify the internal implementations and the external interfaces of pretty, for-each-ref and friends, but modelling the external UI after the mnemonic only with ad hoc additions mess the pretty-format uses is a huge mistake. -- 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 1/3] for-each-ref: introduce %C(...) for color
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: ... users of for-each-ref format will be _more_ familiar with formats used by for-each-ref, and it would make a lot more sense to keep the syntactic resemblance between existing features to show magic things in for-each-ref and the new feature to show color (which is merely one new magic to the vocabulary in the context of for-each-ref), no? Okay, so what do you suggest in place of %C(...)? If %(authordate) is I want to see the author date here, and %(authordate:short) is I want to see the author date here in the short form, you would expect I want colored output in green to be spelled as %(color:green), or something, perhaps? -- 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 1/3] for-each-ref: introduce %C(...) for color
Junio C Hamano wrote: ... users of for-each-ref format will be _more_ familiar with formats used by for-each-ref, and it would make a lot more sense to keep the syntactic resemblance between existing features to show magic things in for-each-ref and the new feature to show color (which is merely one new magic to the vocabulary in the context of for-each-ref), no? Okay, so what do you suggest in place of %C(...)? -- 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 1/3] for-each-ref: introduce %C(...) for color
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: This patch is about for-each-ref and your series does not seem to aim to unify it in any way with pretty-formats, so I would have expected an enhancement in line with the former, not the latter. While I might never attempt a unification again, there's no harm in getting the formats to resemble each other in part. Yes, but... it's likely that users of f-e-r format will be familiar with pretty-formats. ... users of for-each-ref format will be _more_ familiar with formats used by for-each-ref, and it would make a lot more sense to keep the syntactic resemblance between existing features to show magic things in for-each-ref and the new feature to show color (which is merely one new magic to the vocabulary in the context of for-each-ref), no? -- 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 1/3] for-each-ref: introduce %C(...) for color
Junio C Hamano wrote: This patch is about for-each-ref and your series does not seem to aim to unify it in any way with pretty-formats, so I would have expected an enhancement in line with the former, not the latter. While I might never attempt a unification again, there's no harm in getting the formats to resemble each other in part; it's likely that users of f-e-r format will be familiar with pretty-formats. -- 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 1/3] for-each-ref: introduce %C(...) for color
Junio C Hamano wrote: Ramkumar Ramachandra artag...@gmail.com writes: Enhance 'git for-each-ref' with color formatting options. You can now use the following format in for-each-ref: %C(green)%(refname:short)%C(reset) So far, every magic for-each-ref takes were of form %(...); was there a reason why this had to be %C(...), not %(color=blah), or something more in-line with the existing other magic? It is in-line with the color specification in pretty-formats. Would you strongly prefer something else? -- 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 1/3] for-each-ref: introduce %C(...) for color
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Ramkumar Ramachandra artag...@gmail.com writes: Enhance 'git for-each-ref' with color formatting options. You can now use the following format in for-each-ref: %C(green)%(refname:short)%C(reset) So far, every magic for-each-ref takes were of form %(...); was there a reason why this had to be %C(...), not %(color=blah), or something more in-line with the existing other magic? It is in-line with the color specification in pretty-formats. Would you strongly prefer something else? This patch is about for-each-ref and your series does not seem to aim to unify it in any way with pretty-formats, so I would have expected an enhancement in line with the former, not the latter. -- 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 1/3] for-each-ref: introduce %C(...) for color
Ramkumar Ramachandra artag...@gmail.com writes:
Enhance 'git for-each-ref' with color formatting options. You can now
use the following format in for-each-ref:
%C(green)%(refname:short)%C(reset)
So far, every magic for-each-ref takes were of form %(...); was
there a reason why this had to be %C(...), not %(color=blah), or
something more in-line with the existing other magic?
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
Documentation/git-for-each-ref.txt | 4 +++-
builtin/for-each-ref.c | 23 +++
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index f2e08d1..6fa4464 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -45,7 +45,9 @@ OPTIONS
It also interpolates `%%` to `%`, and `%xx` where `xx`
are hex digits interpolates to character with hex code
`xx`; for example `%00` interpolates to `\0` (NUL),
- `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
+ `%09` to `\t` (TAB) and `%0a` to `\n` (LF). Additionally,
+ colors can be specified using `%C(...)`, with names
+ described in color.branch.*.
pattern...::
If one or more patterns are given, only refs are shown that
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1d4083c..6da2903 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@
#include quote.h
#include parse-options.h
#include remote.h
+#include color.h
/* Quoting styles */
#define QUOTE_NONE 0
@@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
while (*cp) {
if (*cp == '%') {
/*
+ * %C( is the start of a color;
* %( is the start of an atom;
* %% is a quoted per-cent.
*/
- if (cp[1] == '(')
+ if (cp[1] == 'C' cp[2] == '(')
+ return cp;
+ else if (cp[1] == '(')
return cp;
else if (cp[1] == '%')
cp++; /* skip over two % */
@@ -180,8 +184,11 @@ static int verify_format(const char *format)
const char *ep = strchr(sp, ')');
if (!ep)
return error(malformed format string %s, sp);
- /* sp points at %( and ep points at the closing ) */
- parse_atom(sp + 2, ep);
+ /* Ignore color specifications: %C(
+ * sp points at %( and ep points at the closing )
+ */
+ if (prefixcmp(sp, %C())
+ parse_atom(sp + 2, ep);
cp = ep + 1;
}
return 0;
@@ -933,12 +940,20 @@ static void emit(const char *cp, const char *ep)
static void show_ref(struct refinfo *info, const char *format, int
quote_style)
{
const char *cp, *sp, *ep;
+ char color[COLOR_MAXLEN] = ;
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
ep = strchr(sp, ')');
if (cp sp)
emit(cp, sp);
- print_value(info, parse_atom(sp + 2, ep), quote_style);
+
+ /* Do we have a color specification? */
+ if (!prefixcmp(sp, %C())
+ color_parse_mem(sp + 3, ep - sp - 3, --format, color);
+ else {
+ printf(%s, color);
+ print_value(info, parse_atom(sp + 2, ep), quote_style);
+ }
}
if (*cp) {
sp = cp + strlen(cp);
--
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 1/3] for-each-ref: introduce %C(...) for color
On Fri, Sep 27, 2013 at 8:10 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
Enhance 'git for-each-ref' with color formatting options. You can now
use the following format in for-each-ref:
%C(green)%(refname:short)%C(reset)
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
Documentation/git-for-each-ref.txt | 4 +++-
builtin/for-each-ref.c | 23 +++
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index f2e08d1..078a116 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -45,7 +45,9 @@ OPTIONS
It also interpolates `%%` to `%`, and `%xx` where `xx`
are hex digits interpolates to character with hex code
`xx`; for example `%00` interpolates to `\0` (NUL),
- `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
+ `%09` to `\t` (TAB) and `%0a` to `\n` (LF). Additionally,
+ colors can be specified using `%C(colorname)`. Use
+ `%C(reset)` to reset the color.
Reduce the color explanation here and refer to the config page.
Something like pretty-formats does:
'%C(...)': color specification, as described in color.branch.*
config option;
pattern...::
If one or more patterns are given, only refs are shown that
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1d4083c..a1ca186 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@
#include quote.h
#include parse-options.h
#include remote.h
+#include color.h
/* Quoting styles */
#define QUOTE_NONE 0
@@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
while (*cp) {
if (*cp == '%') {
/*
+* %C( is the start of a color;
* %( is the start of an atom;
* %% is a quoted per-cent.
*/
- if (cp[1] == '(')
+ if (cp[1] == 'C' cp[2] == '(')
+ return cp;
+ else if (cp[1] == '(')
return cp;
else if (cp[1] == '%')
cp++; /* skip over two % */
@@ -180,8 +184,11 @@ static int verify_format(const char *format)
const char *ep = strchr(sp, ')');
if (!ep)
return error(malformed format string %s, sp);
- /* sp points at %( and ep points at the closing ) */
- parse_atom(sp + 2, ep);
+ /* Ignore color specifications: %C(
+* sp points at %( and ep points at the closing )
+*/
+ if (prefixcmp(sp, %C())
+ parse_atom(sp + 2, ep);
cp = ep + 1;
}
return 0;
@@ -933,12 +940,20 @@ static void emit(const char *cp, const char *ep)
static void show_ref(struct refinfo *info, const char *format, int
quote_style)
{
const char *cp, *sp, *ep;
+ char color[COLOR_MAXLEN];
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
ep = strchr(sp, ')');
if (cp sp)
emit(cp, sp);
- print_value(info, parse_atom(sp + 2, ep), quote_style);
+
+ /* Do we have a color specification? */
+ if (!prefixcmp(sp, %C())
+ color_parse_mem(sp + 3, ep - sp - 3, --format,
color);
+ else {
+ printf(%s, color);
'color' used uninitialized here?
+ print_value(info, parse_atom(sp + 2, ep),
quote_style);
+ }
}
if (*cp) {
sp = cp + strlen(cp);
--
1.8.4.478.g55109e3
--
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
--
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 1/3] for-each-ref: introduce %C(...) for color
On Fri, May 24, 2013 at 10:19 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Since 'git branch' misses important options like --sort, --count, and --format that are present in 'git for-each-ref'. Until we are in a position to fix 'git branch', let us enhance the 'git for-each-ref' format so it can atleast colorize output. s/atleast/at least/ You can use the following format in for-each-ref: %C(green)%(refname:short)%C(reset) To display refs in green. Future patches will attempt to extend the format even more to get useful output. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com -- 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 1/3] for-each-ref: introduce %C(...) for color
Antoine Pelisse wrote: Is it not possible for color to be used uninitialized here ? My compiler didn't complain; what am I missing? Doesn't the declaration char color[COLOR_MAXLEN]; initialize an empty string? More importantly, aren't there numerous instances of this in the codebase? -- 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 1/3] for-each-ref: introduce %C(...) for color
David Aguilar wrote: Can you please also update Documentation/? Yeah, will do in the re-roll. Duy is bringing in pretty-formats. We'll probably need a separate document called pretty-ref-formats or some such thing. -- 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 1/3] for-each-ref: introduce %C(...) for color
On Sat, May 25, 2013 at 05:20:29PM +0530, Ramkumar Ramachandra wrote: Antoine Pelisse wrote: Is it not possible for color to be used uninitialized here ? My compiler didn't complain; what am I missing? Doesn't the declaration char color[COLOR_MAXLEN]; initialize an empty string? Why would it? The variable's begin allocated on the stack and the C standard only zero-initializes variables with static storage duration; Section 6.7.9 of the C11 standard says: If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. I suspect the compiler doesn't complain because there is a path through the function that initializes color before reading it (if we hit the if branch in the loop before the else branch) and the compile assumes that there is something in the function's contract that guarantees that we follow this path. But I don't think that's correct so you do need to initialize color to the empty string. More importantly, aren't there numerous instances of this in the codebase? Care to point at one? I had a quick look and all places I inspected are either static or write to the array before reading it. -- 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 1/3] for-each-ref: introduce %C(...) for color
On Sat, May 25, 2013 at 1:50 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Antoine Pelisse wrote: Is it not possible for color to be used uninitialized here ? My compiler didn't complain; what am I missing? Doesn't the declaration char color[COLOR_MAXLEN]; initialize an empty string? As John said, this is allocated on the stack. What do you want it to be initialized to ? Filled with zeros ? That's overkill because only the first char needs to be zeroed. The compiler can't know what to do and it lets you to take the best action. More importantly, aren't there numerous instances of this in the codebase? I think Valgrind would be mad at us if there was many instances of this. By the way Ramkumar, could you check if Valgrind complains with your patch ? -- 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 1/3] for-each-ref: introduce %C(...) for color
John Keeping wrote: Section 6.7.9 of the C11 standard says: If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. Ah, thanks. I'll initialize it to an empty string. More importantly, aren't there numerous instances of this in the codebase? Care to point at one? I had a quick look and all places I inspected are either static or write to the array before reading it. I'll run some Valgrind tests to see what it yields. -- 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 1/3] for-each-ref: introduce %C(...) for color
On Fri, May 24, 2013 at 4:19 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
@@ -928,12 +936,22 @@ static void emit(const char *cp, const char *ep)
static void show_ref(struct refinfo *info, const char *format, int
quote_style)
{
const char *cp, *sp, *ep;
+ char color[COLOR_MAXLEN];
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
ep = strchr(sp, ')');
if (cp sp)
emit(cp, sp);
- print_value(info, parse_atom(sp + 2, ep), quote_style);
+
+ /* Do we have a color specification? */
+ if (!prefixcmp(sp, %C())
+ color_parse_mem(sp + 3,
+ ep - sp - 3,
+ --format , color);
+ else {
+ printf(%s, color);
Is it not possible for color to be used uninitialized here ?
+ print_value(info, parse_atom(sp + 2, ep),
quote_style);
+ }
}
if (*cp) {
sp = cp + strlen(cp);
--
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 1/3] for-each-ref: introduce %C(...) for color
On Fri, May 24, 2013 at 7:19 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Since 'git branch' misses important options like --sort, --count, and --format that are present in 'git for-each-ref'. Until we are in a position to fix 'git branch', let us enhance the 'git for-each-ref' format so it can atleast colorize output. You can use the following format in for-each-ref: %C(green)%(refname:short)%C(reset) To display refs in green. Future patches will attempt to extend the format even more to get useful output. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- builtin/for-each-ref.c | 26 ++ 1 file changed, 22 insertions(+), 4 deletions(-) Can you please also update Documentation/? -- David -- 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
