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

[Jacob Keller <jacob.keller@gmail.com>]

On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak <karthik.188@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.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@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 = 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 char *color_next;
> +       const char *color_next, *ifexists;
>  };
>
>  struct ref_sorting {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 6aad069..29ed97b 100755
> --- 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
> +'
> +
> +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
> +'
> +
>  test_done
> --
> 2.4.6
>
> --
> 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