Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

2015-08-01 Thread Karthik Nayak 
On Wed, Jul 29, 2015 at 11:30 PM, Junio C Hamano gits...@pobox.com wrote:
 Karthik Nayak karthik@gmail.com writes:

 A handful of huh? on the design.

  - The atom says if *exists* and explanation says has a value.
How are they related?  Does an atom whose value is an empty
string has a value?  Or is ifexists meant to be used only to
ignore meaningless atom, e.g. %(*objectname) applied to a ref that
refers to an object that is not an annotated tag?

 It's meant to ignore meaningless atom. atom's whose values are empty
 strings are ignored.

 That is a self-contradicting answer.

 If you ask for %(*objectname) on a commit, that request truly is
 meaningless, as a commit is not an annotated tag that points at another
 object whose objectname is being asked for.

 But if a commit has an empty log message (you should be able to
 create such an object with commit-tree), then %(subject) would be
 an empty string.  The fact that the commit happens to have an empty
 string as its message is far from meaningless.

 Either you ignore an empty string, or you ignore meaningless one.
 Which does ifexists mean?

I meant ignore atom values which are empty, sorry for the confusion.


  - That %s looks ugly.  Are there cases where a user may want to say
%(ifexists:[%i]) or something other than 's' after that per-cent?

 Couldn't think of a better replacer, any suggestions would be welcome :)

 See below.

 Its given as example, is that misleading?

 Othewise I wouldn't be asking.

  - What, if anything, is allowed to come between %(ifexists...) and
the next atom like %(refname)?  For example, are these valid
constructs?

 . %(ifexists...)%(padright:20)%(refname)

 Doesn't work ...
 ...
  - This syntax does not seem to allow switching on an attribute to
show or not to show another, e.g. if %(*objectname) makes sense,
then show '%(padright:20)%(refname:short) %(*subject)' for it.

 Yes this doesn't do that,

 One way to do all of the above is to make it

 %(ifexists:atom:expansionString)

 That is, for example:

  %(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))

 would give you a string tag v1.0 with v1.0 painted in blue for
 refs/tags/v1.0 but nothing for refs/heads/master.

 Obviously expansionString part needs some escaping mechanism to
 allow it to include an unmatched ).

I liked your other idea of if and endif better :)

-- 
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 04/11] ref-filter: add 'ifexists' atom

2015-08-01 Thread Karthik Nayak 
On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
 Junio C Hamano gits...@pobox.com writes:

 Junio C Hamano gits...@pobox.com writes:

 Couldn't think of a better replacer, any suggestions would be welcome :)

 See below.
 ...
 One way to do all of the above is ...

 Note that is just one way, not the only or not necessarily the
 best.  It certainly is not the easiest, I think.

 %(if:atom)...%(endif)

 might be easier to implement.

 And I find it easier to read or write too. Nested parenthesis in a
 format string make them really tricky. That removes the need for
 escaping since the content of the if/endif is a format string like the
 others, it can use the same escaping rules (IIRC, %% to escape a %).


THat's a really good point. Will work on 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 04/11] ref-filter: add 'ifexists' atom

2015-08-01 Thread Jacob Keller 
On Fri, Jul 31, 2015 at 11:46 PM, Karthik Nayak karthik@gmail.com wrote:
 On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy
 matthieu@grenoble-inp.fr wrote:
 Junio C Hamano gits...@pobox.com writes:

 Junio C Hamano gits...@pobox.com writes:

 Couldn't think of a better replacer, any suggestions would be welcome :)

 See below.
 ...
 One way to do all of the above is ...

 Note that is just one way, not the only or not necessarily the
 best.  It certainly is not the easiest, I think.

 %(if:atom)...%(endif)

 might be easier to implement.

 And I find it easier to read or write too. Nested parenthesis in a
 format string make them really tricky. That removes the need for
 escaping since the content of the if/endif is a format string like the
 others, it can use the same escaping rules (IIRC, %% to escape a %).


 THat's a really good point. Will work on this :)

 --
 Regards,
 Karthik Nayak
 --


Not sure how much work it would take to extent other atoms to this
behavior, such as %(padright) and %(align) and such, that way they
could operate on literals and so forth.

Maybe not worth it as part of this GSoC project, as it may be too
complicated, but maybe something to mark as a TODO for future or
something?

The only issue being that it might mean we have to keep the old
implementation too for backwards compatibility .. Maybe it's easier to
implement that I think, or maybe it's far more challenging than I
think

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 04/11] ref-filter: add 'ifexists' atom

2015-07-29 Thread Junio C Hamano 
Matthieu Moy matthieu@grenoble-inp.fr writes:

 Junio C Hamano gits...@pobox.com writes:

 Junio C Hamano gits...@pobox.com writes:

 Couldn't think of a better replacer, any suggestions would be welcome :)

 See below.
 ...
 One way to do all of the above is ...

 Note that is just one way, not the only or not necessarily the
 best.  It certainly is not the easiest, I think.

 %(if:atom)...%(endif)

 might be easier to implement.

 And I find it easier to read or write too. Nested parenthesis in a
 format string make them really tricky. That removes the need for
 escaping since the content of the if/endif is a format string like the
 others, it can use the same escaping rules (IIRC, %% to escape a %).

Yeah, that is also a good point.
--
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 04/11] ref-filter: add 'ifexists' atom

2015-07-29 Thread Matthieu Moy 
Junio C Hamano gits...@pobox.com writes:

 Junio C Hamano gits...@pobox.com writes:

 Couldn't think of a better replacer, any suggestions would be welcome :)

 See below.
 ...
 One way to do all of the above is ...

 Note that is just one way, not the only or not necessarily the
 best.  It certainly is not the easiest, I think.

 %(if:atom)...%(endif)

 might be easier to implement.

And I find it easier to read or write too. Nested parenthesis in a
format string make them really tricky. That removes the need for
escaping since the content of the if/endif is a format string like the
others, it can use the same escaping rules (IIRC, %% to escape a %).

-- 
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 04/11] ref-filter: add 'ifexists' atom

2015-07-29 Thread Karthik Nayak 
On Tue, Jul 28, 2015 at 11:27 PM, Junio C Hamano gits...@pobox.com wrote:
 Karthik Nayak karthik@gmail.com writes:

 The 'ifexists' atom allows us to print a required format if the
 preceeding atom has a value. If the preceeding atom has no value then
 the format given is not printed. e.g. to print [refname] we can
 now use the format %(ifexists:[%s])%(refname).

 A handful of huh? on the design.

  - The atom says if *exists* and explanation says has a value.
How are they related?  Does an atom whose value is an empty
string has a value?  Or is ifexists meant to be used only to
ignore meaningless atom, e.g. %(*objectname) applied to a ref that
refers to an object that is not an annotated tag?

It's meant to ignore meaningless atom. atom's whose values are empty
strings are ignored.


  - That %s looks ugly.  Are there cases where a user may want to say
%(ifexists:[%i]) or something other than 's' after that per-cent?


Couldn't think of a better replacer, any suggestions would be welcome :)

. Is it allowed to have more than one %s there?
. Is it allowed to have no %s there?


1. yes its allowed to have multiple %s
2. yes no %s is also allowed.

  - The syntax makes the reader wonder if [] is part of the
construct, or just an example of any arbitrary string, i.e. is
%(ifexists:the %s can be part of arbitrary string) valid?


Its given as example, is that misleading?

  - If an arbitrary string is allowed, is there any quoting mechanism
to allow ) to be part of that arbitrary string?

Nope. Haven't done anything for that. I'll look into that :)


  - What, if anything, is allowed to come between %(ifexists...) and
the next atom like %(refname)?  For example, are these valid
constructs?

 . %(ifexists...)%(padright:20)%(refname)

Doesn't work with padright, maybe we could eventually support that.

 . %(ifexists...) %(refname) [%(subject)]


Not sure what this is.

  - This syntax does not seem to allow switching on an attribute to
show or not to show another, e.g. if %(*objectname) makes sense,
then show '%(padright:20)%(refname:short) %(*subject)' for it.

Yes this doesn't do that, I can say this is a pretty basic version, we could
probably work on and implement more things?
This is currently to support 'git branch -vv' where we have the upstream
in square brackets.

-- 
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 04/11] ref-filter: add 'ifexists' atom

2015-07-29 Thread Junio C Hamano 
Karthik Nayak karthik@gmail.com writes:

 A handful of huh? on the design.

  - The atom says if *exists* and explanation says has a value.
How are they related?  Does an atom whose value is an empty
string has a value?  Or is ifexists meant to be used only to
ignore meaningless atom, e.g. %(*objectname) applied to a ref that
refers to an object that is not an annotated tag?

 It's meant to ignore meaningless atom. atom's whose values are empty
 strings are ignored.

That is a self-contradicting answer.

If you ask for %(*objectname) on a commit, that request truly is
meaningless, as a commit is not an annotated tag that points at another
object whose objectname is being asked for.

But if a commit has an empty log message (you should be able to
create such an object with commit-tree), then %(subject) would be
an empty string.  The fact that the commit happens to have an empty
string as its message is far from meaningless.

Either you ignore an empty string, or you ignore meaningless one.
Which does ifexists mean?

  - That %s looks ugly.  Are there cases where a user may want to say
%(ifexists:[%i]) or something other than 's' after that per-cent?

 Couldn't think of a better replacer, any suggestions would be welcome :)

See below.

 Its given as example, is that misleading?

Othewise I wouldn't be asking.

  - What, if anything, is allowed to come between %(ifexists...) and
the next atom like %(refname)?  For example, are these valid
constructs?

 . %(ifexists...)%(padright:20)%(refname)

 Doesn't work ...
 ...
  - This syntax does not seem to allow switching on an attribute to
show or not to show another, e.g. if %(*objectname) makes sense,
then show '%(padright:20)%(refname:short) %(*subject)' for it.

 Yes this doesn't do that,

One way to do all of the above is to make it

%(ifexists:atom:expansionString)

That is, for example:

 %(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))

would give you a string tag v1.0 with v1.0 painted in blue for
refs/tags/v1.0 but nothing for refs/heads/master.

Obviously expansionString part needs some escaping mechanism to
allow it to include an unmatched ).
--
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 04/11] ref-filter: add 'ifexists' atom

2015-07-29 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

 Couldn't think of a better replacer, any suggestions would be welcome :)

 See below.
 ...
 One way to do all of the above is ...

Note that is just one way, not the only or not necessarily the
best.  It certainly is not the easiest, I think.

%(if:atom)...%(endif)

might be easier to implement.
--
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 04/11] ref-filter: add 'ifexists' atom

2015-07-28 Thread Jacob Keller 
On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak karthik@gmail.com wrote:
 The 'ifexists' atom allows us to print a required format if the
 preceeding atom has a value. If the preceeding atom has no value then

Don't you mean following atom here? since you do document it as the
next atom below you should fix the commit message as well to match.
In any respect, this is a useful formatting atom :)

%(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?

 the format given is not printed. e.g. to print [refname] we can
 now use the format %(ifexists:[%s])%(refname).

 Add documentation and test for the same.

 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 |  8 
  ref-filter.c   | 37 ++---
  ref-filter.h   |  5 +++--
  t/t6302-for-each-ref-filter.sh | 21 +
  4 files changed, 66 insertions(+), 5 deletions(-)

 diff --git a/Documentation/git-for-each-ref.txt 
 b/Documentation/git-for-each-ref.txt
 index 9dc02aa..4424020 100644
 --- a/Documentation/git-for-each-ref.txt
 +++ b/Documentation/git-for-each-ref.txt
 @@ -138,6 +138,14 @@ colornext::
 `:colorname`.  Not compatible with `padright` and resets any
 previous `color`, if set.

 +ifexists::
 +   Print required string only if the next atom specified in the
 +   '--format' option exists.
 +   e.g. --format=%(ifexists:[%s])%(symref) prints the symref
 +   like [symref] only if the ref has a symref.  This was
 +   incorporated to simulate the output of 'git branch -vv', where
 +   we need to display the upstream branch in square brackets.
 +

I suggest documenting that the atom will be placed into the contents
of ifexists via the %s indicator, as you do show an example but don't
explicitely say %s is the formatting character.

  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 3f40144..ff5a16b 100644
 --- a/ref-filter.c
 +++ b/ref-filter.c
 @@ -58,6 +58,7 @@ static struct {
 { color },
 { padright },
 { colornext },
 +   { ifexists },
  };

  /*
 @@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)
 v-modifier_atom = 1;
 v-color_next = 1;
 continue;
 +   } else if (starts_with(name, ifexists:)) {
 +   skip_prefix(name, ifexists:, v-s);
 +   if (!*v-s)
 +   die(_(no string given with 'ifexists:'));
 +   v-modifier_atom = 1;
 +   v-ifexists = 1;
 +   continue;
 } else
 continue;

 @@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct 
 ref_formatting_state *state,
  {
 if (state-color_next  state-pad_to_right)
 die(_(cannot use `colornext` and `padright` together));
 -   if (state-color_next) {
 +   if (state-pad_to_right  state-ifexists)
 +   die(_(cannot use 'align' and 'ifexists' together));
 +   if (state-color_next  !state-ifexists) {
 strbuf_addf(value, %s%s%s, state-color_next, v-s, 
 GIT_COLOR_RESET);
 return;
 -   }
 -   else if (state-pad_to_right) {
 +   } else if (state-ifexists) {
 +   const char *sp = state-ifexists;
 +
 +   while (*sp) {
 +   if (*sp != '%') {
 +   strbuf_addch(value, *sp++);
 +   continue;
 +   } else if (sp[1] == '%') {
 +   strbuf_addch(value, *sp++);
 +   continue;
 +   } else if (sp[1] == 's') {
 +   if (state-color_next)
 +   strbuf_addf(value, %s%s%s, 
 state-color_next, v-s, GIT_COLOR_RESET);
 +   else
 +   strbuf_addstr(value, v-s);
 +   sp += 2;
 +   }
 +   }
 +
 +   return;
 +   } else if (state-pad_to_right) {
 if (!is_utf8(v-s))
 strbuf_addf(value, %-*s, state-pad_to_right, v-s);
 else {
 @@ -1413,6 +1442,8 @@ static void store_formatting_state(struct 
 ref_formatting_state *state,
 state-color_next = atomv-s;
 if (atomv-pad_to_right)
 state-pad_to_right = atomv-ul;
 +   if (atomv-ifexists)
 +   state-ifexists =

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' 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
 @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
   test_cmp expect actual
  '
  
 +test_expect_success 'check `ifexists` format option' '
 + cat expect -\EOF 
 + [foo1.10]
 + [foo1.3]
 + [foo1.6]
 + EOF
 + git for-each-ref --format=%(ifexists:[%s])%(refname:short) | grep 
 foo actual 
 + test_cmp expect actual
 +'

You're testing only the positive case of ifexists, right? You need a
test for the negative case (i.e. the attribute does not exist) too.
Ideally, check that the ifexist actually applies to the right attribute,
like

--format '%(ifexist:%s)%(attribute1) %(ifexist:[%s])%(attribute2)'

and manage to get an output like

foo
 [bar]
foobar [barfoo]

 +cat expect EOF 
 +[$(get_color green)foo1.10$(get_color reset)]||foo1.10
 +[$(get_color green)foo1.3$(get_color reset)]||foo1.3
 +[$(get_color green)foo1.6$(get_color reset)]||foo1.6
 +EOF
 +
 +test_expect_success 'check `ifexists` with `colornext` format option' '
 + git for-each-ref 
 --format=%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)
  | grep foo actual 
 + test_cmp expect actual
 +'

You have a double || that is not useful. Not really harmful either, but
it may distract the reader. You may want to s/||/|/.

-- 
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 04/11] ref-filter: add 'ifexists' atom

2015-07-28 Thread Karthik Nayak 
The 'ifexists' atom allows us to print a required format if the
preceeding atom has a value. If the preceeding atom has no value then
the format given is not printed. e.g. to print [refname] we can
now use the format %(ifexists:[%s])%(refname).

Add documentation and test for the same.

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 |  8 
 ref-filter.c   | 37 ++---
 ref-filter.h   |  5 +++--
 t/t6302-for-each-ref-filter.sh | 21 +
 4 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 9dc02aa..4424020 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -138,6 +138,14 @@ colornext::
`:colorname`.  Not compatible with `padright` and resets any
previous `color`, if set.
 
+ifexists::
+   Print required string only if the next atom specified in the
+   '--format' option exists.
+   e.g. --format=%(ifexists:[%s])%(symref) prints the symref
+   like [symref] only if the ref has a symref.  This was
+   incorporated to simulate the output of 'git branch -vv', where
+   we need to display the upstream branch in square brackets.
+
 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 3f40144..ff5a16b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -58,6 +58,7 @@ static struct {
{ color },
{ padright },
{ colornext },
+   { ifexists },
 };
 
 /*
@@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)
v-modifier_atom = 1;
v-color_next = 1;
continue;
+   } else if (starts_with(name, ifexists:)) {
+   skip_prefix(name, ifexists:, v-s);
+   if (!*v-s)
+   die(_(no string given with 'ifexists:'));
+   v-modifier_atom = 1;
+   v-ifexists = 1;
+   continue;
} else
continue;
 
@@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct 
ref_formatting_state *state,
 {
if (state-color_next  state-pad_to_right)
die(_(cannot use `colornext` and `padright` together));
-   if (state-color_next) {
+   if (state-pad_to_right  state-ifexists)
+   die(_(cannot use 'align' and 'ifexists' together));
+   if (state-color_next  !state-ifexists) {
strbuf_addf(value, %s%s%s, state-color_next, v-s, 
GIT_COLOR_RESET);
return;
-   }
-   else if (state-pad_to_right) {
+   } else if (state-ifexists) {
+   const char *sp = state-ifexists;
+
+   while (*sp) {
+   if (*sp != '%') {
+   strbuf_addch(value, *sp++);
+   continue;
+   } else if (sp[1] == '%') {
+   strbuf_addch(value, *sp++);
+   continue;
+   } else if (sp[1] == 's') {
+   if (state-color_next)
+   strbuf_addf(value, %s%s%s, 
state-color_next, v-s, GIT_COLOR_RESET);
+   else
+   strbuf_addstr(value, v-s);
+   sp += 2;
+   }
+   }
+
+   return;
+   } else if (state-pad_to_right) {
if (!is_utf8(v-s))
strbuf_addf(value, %-*s, state-pad_to_right, v-s);
else {
@@ -1413,6 +1442,8 @@ static void store_formatting_state(struct 
ref_formatting_state *state,
state-color_next = atomv-s;
if (atomv-pad_to_right)
state-pad_to_right = atomv-ul;
+   if (atomv-ifexists)
+   state-ifexists = atomv-s;
 }
 
 static void reset_formatting_state(struct ref_formatting_state *state)
diff --git a/ref-filter.h b/ref-filter.h
index a021b04..7d1871d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,13 +28,14 @@ struct atom_value {
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,
-   color_next : 1;
+   color_next : 1,
+   ifexists : 1;
 };
 
 struct ref_formatting_state {
int quote_style;
unsigned int pad_to_right;
-   const

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

2015-07-28 Thread Karthik Nayak 
On Tue, Jul 28, 2015 at 1:24 PM, Jacob Keller jacob.kel...@gmail.com wrote:
 On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak karthik@gmail.com wrote:
 The 'ifexists' atom allows us to print a required format if the
 preceeding atom has a value. If the preceeding atom has no value then

 Don't you mean following atom here? since you do document it as the
 next atom below you should fix the commit message as well to match.
 In any respect, this is a useful formatting atom :)


That should have been `succeeding` atom. My bad! thanks :)

 %(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?

 I suggest documenting that the atom will be placed into the contents
 of ifexists via the %s indicator, as you do show an example but don't
 explicitely say %s is the formatting character.


Yeah! I should have explicitly mentioned that, 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 04/11] ref-filter: add 'ifexists' atom

2015-07-28 Thread Karthik Nayak 
On Tue, Jul 28, 2015 at 2:20 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
 @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
   test_cmp expect actual
  '

 +test_expect_success 'check `ifexists` format option' '
 + cat expect -\EOF 
 + [foo1.10]
 + [foo1.3]
 + [foo1.6]
 + EOF
 + git for-each-ref --format=%(ifexists:[%s])%(refname:short) | grep 
 foo actual 
 + test_cmp expect actual
 +'

 You're testing only the positive case of ifexists, right? You need a
 test for the negative case (i.e. the attribute does not exist) too.
 Ideally, check that the ifexist actually applies to the right attribute,
 like

 --format '%(ifexist:%s)%(attribute1) %(ifexist:[%s])%(attribute2)'

 and manage to get an output like

 foo
  [bar]
 foobar [barfoo]


Yes! this seems like an important test, thanks :)


 You have a double || that is not useful. Not really harmful either, but
 it may distract the reader. You may want to s/||/|/.


Will change!

-- 
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 04/11] ref-filter: add 'ifexists' atom

2015-07-28 Thread Junio C Hamano 
Karthik Nayak karthik@gmail.com writes:

 The 'ifexists' atom allows us to print a required format if the
 preceeding atom has a value. If the preceeding atom has no value then
 the format given is not printed. e.g. to print [refname] we can
 now use the format %(ifexists:[%s])%(refname).

A handful of huh? on the design.

 - The atom says if *exists* and explanation says has a value.
   How are they related?  Does an atom whose value is an empty
   string has a value?  Or is ifexists meant to be used only to
   ignore meaningless atom, e.g. %(*objectname) applied to a ref that
   refers to an object that is not an annotated tag?

 - That %s looks ugly.  Are there cases where a user may want to say
   %(ifexists:[%i]) or something other than 's' after that per-cent?

   . Is it allowed to have more than one %s there?
   . Is it allowed to have no %s there?

 - The syntax makes the reader wonder if [] is part of the
   construct, or just an example of any arbitrary string, i.e. is
   %(ifexists:the %s can be part of arbitrary string) valid?

 - If an arbitrary string is allowed, is there any quoting mechanism
   to allow ) to be part of that arbitrary string?
   
 - What, if anything, is allowed to come between %(ifexists...) and
   the next atom like %(refname)?  For example, are these valid
   constructs?

. %(ifexists...)%(padright:20)%(refname)
. %(ifexists...) %(refname) [%(subject)]

 - This syntax does not seem to allow switching on an attribute to
   show or not to show another, e.g. if %(*objectname) makes sense,
   then show '%(padright:20)%(refname:short) %(*subject)' for 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