Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-08-01 Thread Karthik Nayak 
On Thu, Jul 30, 2015 at 9:47 PM, Junio C Hamano gits...@pobox.com wrote:
 Jacob Keller jacob.kel...@gmail.com writes:

 On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
 matthieu@grenoble-inp.fr wrote:
 Eric Sunshine sunsh...@sunshineco.com writes:

 Also, please explain here and in the commit message why this highly
 specialized colorizer ('colornext'), is needed even though a more
 general purpose one ('color') is already available.

 It is needed in the current form to allow
 %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
 and not the [].

 But I now think that this would be more elegantly solved by Junio's
 %(if) %(endif) idea:

   %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)

 (I added spaces for clarity)

 I agree, this style seems a lot more elegant and expressive while much
 easier to understand. Same for doing this to the alignment atoms and
 such as it solves the same problem(s) there.

 Do you mean something like these?

 %(align:left,20) branch %(refname:short) %(end)
 %(align:middle,20) branch %(refname:short) %(end)
 %(align:right,20) branch %(refname:short) %(end)

 to replace and enhance %(padright:20)?

 I can't speak to how easy it is to implement tho.

 Perhaps it would go like this:

  * Instead of always emitting to the standard output, emit() and
print_value() will gain an option to append into a strbuf that is
passed as argument.  Alternatively, appending to strbuf could be
made the only output channel for them; show_ref_array_item() can
prepare an empty strbuf, call them repeatedly to fill it, and
then print the resulting strbuf itself.

  * Things like %(if) and %(align) would do the following:

(1) Push the currently active strbuf it got from the calling
show_ref_array_item() to the formatting state;

(2) Create a new strbuf and arrange so that further output would
be diverted to this new one; and

(3) Push the fact that the diverted output will be processed by
them (i.e. %(if), %(align), etc.) when the diversion finishes
to the formatting state.

  * When %(end) is seen, the currently active strbuf (i.e. the new
one created in (2) above for diversion) holds the output made
since the previously seen %(if), %(align), etc.  The formatting
state knows what processing needs to be performed on that from
(3) above.

- Pop the strbuf where the output of the entire %(if)...%(end)
  construct needs to go from the formatting state;

- Have the processing popped from (3) above, e.g. %(if:atom) or
  %(align:left,20), do whatever they need to do on the diverted
  output, and emit their result.

 Both %(if) and the hypotherical %(align) can use this same
 diversion mechanism.  And the above would properly nest,
 e.g.

 %(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)


This actually looks neat and good, I'll try implementing this :)

-- 
Regards,
Karthik Nayak
--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-30 Thread Junio C Hamano 
Jacob Keller jacob.kel...@gmail.com writes:

 On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
 matthieu@grenoble-inp.fr wrote:
 Eric Sunshine sunsh...@sunshineco.com writes:

 Also, please explain here and in the commit message why this highly
 specialized colorizer ('colornext'), is needed even though a more
 general purpose one ('color') is already available.

 It is needed in the current form to allow
 %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
 and not the [].

 But I now think that this would be more elegantly solved by Junio's
 %(if) %(endif) idea:

   %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)

 (I added spaces for clarity)

 I agree, this style seems a lot more elegant and expressive while much
 easier to understand. Same for doing this to the alignment atoms and
 such as it solves the same problem(s) there.

Do you mean something like these?

%(align:left,20) branch %(refname:short) %(end)
%(align:middle,20) branch %(refname:short) %(end)
%(align:right,20) branch %(refname:short) %(end)

to replace and enhance %(padright:20)?

 I can't speak to how easy it is to implement tho.

Perhaps it would go like this:

 * Instead of always emitting to the standard output, emit() and
   print_value() will gain an option to append into a strbuf that is
   passed as argument.  Alternatively, appending to strbuf could be
   made the only output channel for them; show_ref_array_item() can
   prepare an empty strbuf, call them repeatedly to fill it, and
   then print the resulting strbuf itself.

 * Things like %(if) and %(align) would do the following:

   (1) Push the currently active strbuf it got from the calling
   show_ref_array_item() to the formatting state;

   (2) Create a new strbuf and arrange so that further output would
   be diverted to this new one; and

   (3) Push the fact that the diverted output will be processed by
   them (i.e. %(if), %(align), etc.) when the diversion finishes
   to the formatting state.

 * When %(end) is seen, the currently active strbuf (i.e. the new
   one created in (2) above for diversion) holds the output made
   since the previously seen %(if), %(align), etc.  The formatting
   state knows what processing needs to be performed on that from
   (3) above.

   - Pop the strbuf where the output of the entire %(if)...%(end)
 construct needs to go from the formatting state;

   - Have the processing popped from (3) above, e.g. %(if:atom) or
 %(align:left,20), do whatever they need to do on the diverted
 output, and emit their result.

Both %(if) and the hypotherical %(align) can use this same
diversion mechanism.  And the above would properly nest,
e.g.

%(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)




--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-29 Thread Matthieu Moy 
Eric Sunshine sunsh...@sunshineco.com writes:

 Also, please explain here and in the commit message why this highly
 specialized colorizer ('colornext'), is needed even though a more
 general purpose one ('color') is already available.

It is needed in the current form to allow
%(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
and not the [].

But I now think that this would be more elegantly solved by Junio's
%(if) %(endif) idea:

  %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)

(I added spaces for clarity)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-29 Thread Eric Sunshine 
On Tuesday, July 28, 2015, Karthik Nayak karthik@gmail.com wrote:
 The 'colornext' atom allows us to color only the succeeding atom with
 a particular color. This resets any previous color preferences set. It
 is not compatible with `padright`. This is required for printing
 formats like `git branch -vv` where the upstream is colored in blue.

Can you explain here and in the commit message why %(colornext) is not
compatible with %(padright:)? Is it an implementation limitation, or a
syntax or semantic limitation, or what? Can the implementation be
fixed to make it compatible or does it require a redesign?

Also, please explain here and in the commit message why this highly
specialized colorizer ('colornext'), is needed even though a more
general purpose one ('color') is already available.
--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-29 Thread Jacob Keller 
On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
 Eric Sunshine sunsh...@sunshineco.com writes:

 Also, please explain here and in the commit message why this highly
 specialized colorizer ('colornext'), is needed even though a more
 general purpose one ('color') is already available.

 It is needed in the current form to allow
 %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
 and not the [].

 But I now think that this would be more elegantly solved by Junio's
 %(if) %(endif) idea:

   %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)

 (I added spaces for clarity)


I agree, this style seems a lot more elegant and expressive while much
easier to understand. Same for doing this to the alignment atoms and
such as it solves the same problem(s) there.

I can't speak to how easy it is to implement tho.

Regards,
Jake
--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-28 Thread Matthieu Moy 
Karthik Nayak karthik@gmail.com writes:

 --- a/t/t6302-for-each-ref-filter.sh
 +++ b/t/t6302-for-each-ref-filter.sh
 @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
   test_cmp expect actual
  '
  
 +get_color ()
 +{
 + git config --get-color no.such.slot $1
 +}
 +
 +cat expect EOF 
 +$(get_color green)foo1.10$(get_color reset)||
 +$(get_color green)foo1.3$(get_color reset)||
 +$(get_color green)foo1.6$(get_color reset)||
 +EOF
 +
 +test_expect_success 'check `colornext` format option' '
 + git for-each-ref --format=%(colornext:green)%(refname:short)|| | grep 
 foo actual 
 + test_cmp expect actual
 +'

This is not a very good test: you're not checking that colornext applies
to the next and only this one. Similarly to what I suggested for
padright, I'd suggest

  
--format=%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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

[RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-28 Thread Karthik Nayak 
The 'colornext' atom allows us to color only the succeeding atom with
a particular color. This resets any previous color preferences set. It
is not compatible with `padright`. This is required for printing
formats like `git branch -vv` where the upstream is colored in blue.

Mentored-by: Christian Couder christian.cou...@gmail.com
Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Karthik Nayak karthik@gmail.com
---
 Documentation/git-for-each-ref.txt |  5 +
 ref-filter.c   | 20 +++-
 ref-filter.h   |  4 +++-
 t/t6302-for-each-ref-filter.sh | 16 
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 2b60aee..9dc02aa 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,6 +133,11 @@ padright::
padding. If the `value` is greater than the atom length, then
no padding is performed.
 
+colornext::
+   Change output color only for the next atom. Followed by
+   `:colorname`.  Not compatible with `padright` and resets any
+   previous `color`, if set.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 4c5e3f8..3ab4ab9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -57,6 +57,7 @@ static struct {
{ HEAD },
{ color },
{ padright },
+   { colornext },
 };
 
 /*
@@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
v-modifier_atom = 1;
v-pad_to_right = 1;
continue;
+   } else if (starts_with(name, colornext:)) {
+   char color[COLOR_MAXLEN] = ;
+
+   if (color_parse(name + 10, color)  0)
+   die(_(unable to parse format));
+   v-s = xstrdup(color);
+   v-modifier_atom = 1;
+   v-color_next = 1;
+   continue;
} else
continue;
 
@@ -1256,7 +1266,13 @@ void ref_array_sort(struct ref_sorting *sorting, struct 
ref_array *array)
 static void apply_formatting_state(struct ref_formatting_state *state,
   struct atom_value *v, struct strbuf *value)
 {
-   if (state-pad_to_right) {
+   if (state-color_next  state-pad_to_right)
+   die(_(cannot use `colornext` and `padright` together));
+   if (state-color_next) {
+   strbuf_addf(value, %s%s%s, state-color_next, v-s, 
GIT_COLOR_RESET);
+   return;
+   }
+   else if (state-pad_to_right) {
if (!is_utf8(v-s))
strbuf_addf(value, %-*s, state-pad_to_right, v-s);
else {
@@ -1346,6 +1362,8 @@ static void emit(const char *cp, const char *ep)
 static void store_formatting_state(struct ref_formatting_state *state,
   struct atom_value *atomv)
 {
+   if (atomv-color_next)
+   state-color_next = atomv-s;
if (atomv-pad_to_right)
state-pad_to_right = atomv-ul;
 }
diff --git a/ref-filter.h b/ref-filter.h
index fcf469e..8c82fd1 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -21,12 +21,14 @@ struct atom_value {
const char *s;
unsigned long ul; /* used for sorting when not FIELD_STR */
unsigned int modifier_atom : 1, /*  atoms which act as modifiers for 
the next atom */
-   pad_to_right : 1;
+   pad_to_right : 1,
+   color_next : 1;
 };
 
 struct ref_formatting_state {
int quote_style;
unsigned int pad_to_right;
+   const char *color_next;
 };
 
 struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 68688a9..6aad069 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
test_cmp expect actual
 '
 
+get_color ()
+{
+   git config --get-color no.such.slot $1
+}
+
+cat expect EOF 
+$(get_color green)foo1.10$(get_color reset)||
+$(get_color green)foo1.3$(get_color reset)||
+$(get_color green)foo1.6$(get_color reset)||
+EOF
+
+test_expect_success 'check `colornext` format option' '
+   git for-each-ref --format=%(colornext:green)%(refname:short)|| | grep 
foo actual 
+   test_cmp expect actual
+'
+
 test_done
-- 
2.4.6

--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-28 Thread Christian Couder 
On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak karthik@gmail.com wrote:
 @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
 v-modifier_atom = 1;
 v-pad_to_right = 1;
 continue;
 +   } else if (starts_with(name, colornext:)) {
 +   char color[COLOR_MAXLEN] = ;
 +
 +   if (color_parse(name + 10, color)  0)
 +   die(_(unable to parse format));

Maybe use skip_prefix(), and die() with a more helpful message.

 +   v-s = xstrdup(color);
 +   v-modifier_atom = 1;
 +   v-color_next = 1;
 +   continue;
 } else
 continue;
--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-28 Thread Karthik Nayak 
On Tue, Jul 28, 2015 at 2:43 PM, Christian Couder
christian.cou...@gmail.com wrote:
 On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak karthik@gmail.com wrote:
 @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
 v-modifier_atom = 1;
 v-pad_to_right = 1;
 continue;
 +   } else if (starts_with(name, colornext:)) {
 +   char color[COLOR_MAXLEN] = ;
 +
 +   if (color_parse(name + 10, color)  0)
 +   die(_(unable to parse format));

 Maybe use skip_prefix(), and die() with a more helpful message.


Okay will do. Thanks!

-- 
Regards,
Karthik Nayak
--
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: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

2015-07-28 Thread Karthik Nayak 
On Tue, Jul 28, 2015 at 2:15 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
 Karthik Nayak karthik@gmail.com writes:

 --- a/t/t6302-for-each-ref-filter.sh
 +++ b/t/t6302-for-each-ref-filter.sh
 @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
   test_cmp expect actual
  '

 +get_color ()
 +{
 + git config --get-color no.such.slot $1
 +}
 +
 +cat expect EOF 
 +$(get_color green)foo1.10$(get_color reset)||
 +$(get_color green)foo1.3$(get_color reset)||
 +$(get_color green)foo1.6$(get_color reset)||
 +EOF
 +
 +test_expect_success 'check `colornext` format option' '
 + git for-each-ref --format=%(colornext:green)%(refname:short)|| | 
 grep foo actual 
 + test_cmp expect actual
 +'

 This is not a very good test: you're not checking that colornext applies
 to the next and only this one. Similarly to what I suggested for
 padright, I'd suggest

   
 --format=%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|


That was the purpose of the || but that doesn't check the color of next atom,
Thanks for the example will use that :)

-- 
Regards,
Karthik Nayak
--
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