v7, 17 partsport branch.c to use ref-filter's printing options

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Implement %(if), %(then) and %(else) atoms. Used as
> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
> format string between %(if) and %(then) expands to an empty string, or
> to only whitespaces, then the whole %(if)...%(end) expands to the string
> following %(then). Otherwise, it expands to the string following
> %(else), if any. Nesting of this construct is possible.
>
> This is in preparation for porting over `git branch -l` to use
> ref-filter APIs for printing.
>
> Add Documentation and tests regarding 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 |  40 +++++++++++
>  ref-filter.c                       | 133 +++++++++++++++++++++++++++++++++++--
>  t/t6302-for-each-ref-filter.sh     |  76 +++++++++++++++++++++
>  3 files changed, 242 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f57e69b..fed8126 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -146,6 +146,16 @@ align::
>         quoted, but if nested then only the topmost level performs
>         quoting.
>
> +if::
> +       Used as %(if)...%(then)...(%end) or
> +       %(if)...%(then)...%(else)...%(end).  If there is an atom with
> +       value or string literal after the %(if) then everything after
> +       the %(then) is printed, else if the %(else) atom is used, then
> +       everything after %(else) is printed. We ignore space when
> +       evaluating the string before %(then), this is useful when we
> +       use the %(HEAD) atom which prints either "*" or " " and we
> +       want to apply the 'if' condition only on the 'HEAD' ref.
> +
>  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.
> @@ -181,6 +191,20 @@ As a special case for the date-type fields, you may specify a format for
>  the date by adding `:` followed by date format name (see the
>  values the `--date` option to linkgit:git-rev-list[1] takes).
>
> +Some atoms like %(align) and %(if) always require a matching %(end).
> +We call them "opening atoms" and sometimes denote them as %($open).
> +
> +When a scripting language specific quoting is in effect (i.e. one of
> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
> +atoms, replacement from every %(atom) is quoted when and only when it
> +appears at the top-level (that is, when it appears outside
> +%($open)...%(end)).
> +
> +When a scripting language specific quoting is in effect, everything
> +between a top-level opening atom and its matching %(end) is evaluated
> +according to the semantics of the opening atom and its result is
> +quoted.
> +
>

>  EXAMPLES
>  --------
> @@ -268,6 +292,22 @@ eval=`git for-each-ref --shell --format="$fmt" \
>  eval "$eval"
>  ------------
>
> +
> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
> +This prefixes the current branch with a star.
> +
> +------------
> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  %(end)%(refname:short)" refs/heads/
> +------------
> +
> +
> +An example to show the usage of %(if)...%(then)...%(end).
> +This prints the authorname, if present.
> +
> +------------
> +git for-each-ref --format="%(refname)%(if)%(authorname)%(then) %(color:red)Authored by: %(authorname)%(end)"
> +------------
> +
>  SEE ALSO
>  --------
>  linkgit:git-show-ref[1]
> diff --git a/ref-filter.c b/ref-filter.c
> index d4c2931..8c183a0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -21,6 +21,12 @@ struct align {
>         unsigned int width;
>  };
>
> +struct if_then_else {
> +       unsigned int then_atom_seen : 1,
> +               else_atom_seen : 1,
> +               condition_satisfied : 1;
> +};
> +
>  /*
>   * An atom is a valid field atom listed below, possibly prefixed with
>   * a "*" to denote deref_tag().
> @@ -203,6 +209,9 @@ static struct {
>         { "color", FIELD_STR, color_atom_parser },
>         { "align", FIELD_STR, align_atom_parser },
>         { "end" },
> +       { "if" },
> +       { "then" },
> +       { "else" },
>  };
>
>  #define REF_FORMATTING_STATE_INIT  { 0, NULL }
> @@ -210,7 +219,7 @@ static struct {
>  struct ref_formatting_stack {
>         struct ref_formatting_stack *prev;
>         struct strbuf output;
> -       void (*at_end)(struct ref_formatting_stack *stack);
> +       void (*at_end)(struct ref_formatting_stack **stack);
>         void *at_end_data;
>  };
>
> @@ -343,13 +352,14 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
>         *stack = prev;
>  }
>
> -static void end_align_handler(struct ref_formatting_stack *stack)
> +static void end_align_handler(struct ref_formatting_stack **stack)
>  {

> -       struct align *align = (struct align *)stack->at_end_data;
> +       struct ref_formatting_stack *cur = *stack;
> +       struct align *align = (struct align *)cur->at_end_data;
>         struct strbuf s = STRBUF_INIT;
>
> -       strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
> -       strbuf_swap(&stack->output, &s);
> +       strbuf_utf8_align(&s, align->position, align->width, cur->output.buf);
> +       strbuf_swap(&cur->output, &s);
>         strbuf_release(&s);
>  }
>
> @@ -363,6 +373,103 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
>         new->at_end_data = &atomv->u.align;
>  }
>
> +static void if_then_else_handler(struct ref_formatting_stack **stack)
> +{
> +       struct ref_formatting_stack *cur = *stack;
> +       struct ref_formatting_stack *prev = cur->prev;
> +       struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
> +
> +       if (!if_then_else->then_atom_seen)
> +               die(_("format: %%(if) atom used without a %%(then) atom"));
> +
> +       if (if_then_else->else_atom_seen) {
> +               /*
> +                * There is an %(else) atom: we need to drop one state from the
> +                * stack, either the %(else) branch if the condition is satisfied, or
> +                * the %(then) branch if it isn't.
> +                */
> +               if (if_then_else->condition_satisfied) {
> +                       strbuf_reset(&cur->output);
> +                       pop_stack_element(&cur);

> +               /*
> +                * No %(else) atom: just drop the %(then) branch if the
> +                * condition is not satisfied.
> +                */
> +               strbuf_reset(&cur->output);

> +
> +       *stack = cur;
> +       free(if_then_else);
> +}
> +
> +static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> +       struct ref_formatting_stack *new;
> +       struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
> +
> +       push_stack_element(&state->stack);
> +       new = state->stack;
> +       new->at_end = if_then_else_handler;
> +       new->at_end_data = if_then_else;
> +}
> +

> +static int is_empty(const char *s)
> +{
> +       while (*s != '\0') {
> +               if (!isspace(*s))
> +                       return 0;
> +               s++;
> +       }
> +       return 1;
> +}
> +
> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> +       struct ref_formatting_stack *cur = state->stack;
> +       struct if_then_else *if_then_else = NULL;
> +
> +       if (cur->at_end == if_then_else_handler)
> +               if_then_else = (struct if_then_else *)cur->at_end_data;
> +       if (!if_then_else)
> +               die(_("format: %%(then) atom used without an %%(if) atom"));
> +       if (if_then_else->then_atom_seen)
> +               die(_("format: %%(then) atom used more than once"));
> +       if (if_then_else->else_atom_seen)
> +               die(_("format: %%(then) atom used after %%(else)"));
> +       if_then_else->then_atom_seen = 1;
> +       /*
> +        * If there exists non-empty string between the 'if' and
> +        * 'then' atom then the 'if' condition is satisfied.
> +        */
> +       if (cur->output.len && !is_empty(cur->output.buf))
> +               if_then_else->condition_satisfied = 1;
> +       strbuf_reset(&cur->output);
> +}

> +
> +static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> +       struct ref_formatting_stack *prev = state->stack;
> +       struct if_then_else *if_then_else = NULL;
> +
> +       if (prev->at_end == if_then_else_handler)
> +               if_then_else = (struct if_then_else *)prev->at_end_data;
> +       if (!if_then_else)
> +               die(_("format: %%(else) atom used without an %%(if) atom"));
> +       if (!if_then_else->then_atom_seen)
> +               die(_("format: %%(else) atom used without a %%(then) atom"));
> +       if (if_then_else->else_atom_seen)
> +               die(_("format: %%(else) atom used more than once"));
> +       if_then_else->else_atom_seen = 1;
> +       push_stack_element(&state->stack);
> +       state->stack->at_end_data = prev->at_end_data;
> +       state->stack->at_end = prev->at_end;
> +}

> +
>  static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>  {
>         struct ref_formatting_stack *current = state->stack;
> @@ -370,14 +477,17 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
>
>         if (!current->at_end)
>                 die(_("format: %%(end) atom used without corresponding atom"));
> -       current->at_end(current);
> +       current->at_end(&state->stack);
> +
> +       /*  Stack may have been popped within at_end(), hence reset the current pointer */
> +       current = state->stack;
>
>         /*
>          * Perform quote formatting when the stack element is that of
>          * a supporting atom. If nested then perform quote formatting
>          * only on the topmost supporting atom.
>          */
> -       if (!state->stack->prev->prev) {
> +       if (!current->prev->prev) {
>                 quote_formatting(&s, current->output.buf, state->quote_style);
>                 strbuf_swap(&current->output, &s);
>         }
> @@ -1029,6 +1139,15 @@ static void populate_value(struct ref_array_item *ref)
>                 } else if (!strcmp(name, "end")) {
>                         v->handler = end_atom_handler;
>                         continue;
> +               } else if (!strcmp(name, "if")) {
> +                       v->handler = if_atom_handler;
> +                       continue;
> +               } else if (!strcmp(name, "then")) {
> +                       v->handler = then_atom_handler;
> +                       continue;
> +               } else if (!strcmp(name, "else")) {
> +                       v->handler = else_atom_handler;
> +                       continue;
>                 } else
>                         continue;
>
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index d0ab09f..fed3013 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -327,4 +327,80 @@ test_expect_success 'reverse version sort' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'improper usage of %(if), %(then), %(else) and %(end) atoms' '
> +       test_must_fail git for-each-ref --format="%(if)" &&
> +       test_must_fail git for-each-ref --format="%(then) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(else) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(if) %(else) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(if) %(then) %(then) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(then) %(else) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(if) %(else) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(if) %(then) %(else)" &&
> +       test_must_fail git for-each-ref --format="%(if) %(else) %(then) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(if) %(then) %(else) %(else) %(end)" &&
> +       test_must_fail git for-each-ref --format="%(if) %(end)"
> +'
> +
> +test_expect_success 'check %(if)...%(then)...%(end) atoms' '
> +       git for-each-ref --format="%(refname)%(if)%(authorname)%(then) Author: %(authorname)%(end)" >actual &&
> +       cat >expect <<-\EOF &&
> +       refs/heads/master Author: A U Thor
> +       refs/heads/side Author: A U Thor
> +       refs/odd/spot Author: A U Thor
> +       refs/tags/annotated-tag
> +       refs/tags/doubly-annotated-tag
> +       refs/tags/doubly-signed-tag
> +       refs/tags/foo1.10 Author: A U Thor
> +       refs/tags/foo1.3 Author: A U Thor
> +       refs/tags/foo1.6 Author: A U Thor
> +       refs/tags/four Author: A U Thor
> +       refs/tags/one Author: A U Thor
> +       refs/tags/signed-tag
> +       refs/tags/three Author: A U Thor
> +       refs/tags/two Author: A U Thor
> +       EOF
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
> +       git for-each-ref --format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): %(refname)" >actual &&
> +       cat >expect <<-\EOF &&
> +       A U Thor: refs/heads/master
> +       A U Thor: refs/heads/side
> +       A U Thor: refs/odd/spot
> +       No author: refs/tags/annotated-tag
> +       No author: refs/tags/doubly-annotated-tag
> +       No author: refs/tags/doubly-signed-tag
> +       A U Thor: refs/tags/foo1.10
> +       A U Thor: refs/tags/foo1.3
> +       A U Thor: refs/tags/foo1.6
> +       A U Thor: refs/tags/four
> +       A U Thor: refs/tags/one
> +       No author: refs/tags/signed-tag
> +       A U Thor: refs/tags/three
> +       A U Thor: refs/tags/two
> +       EOF
> +       test_cmp expect actual
> +'
> +test_expect_success 'ignore spaces in %(if) atom usage' '
> +       git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head ref%(else)Not Head ref%(end)" >actual &&
> +       cat >expect <<-\EOF &&
> +       master: Head ref
> +       side: Not Head ref
> +       odd/spot: Not Head ref
> +       annotated-tag: Not Head ref
> +       doubly-annotated-tag: Not Head ref
> +       doubly-signed-tag: Not Head ref
> +       foo1.10: Not Head ref
> +       foo1.3: Not Head ref
> +       foo1.6: Not Head ref
> +       four: Not Head ref
> +       one: Not Head ref
> +       signed-tag: Not Head ref
> +       three: Not Head ref
> +       two: Not Head ref
> +       EOF
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> +Some atoms like %(align) and %(if) always require a matching %(end).
>> +We call them "opening atoms" and sometimes denote them as %($open).
>> +
>> +When a scripting language specific quoting is in effect (i.e. one of
>> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
>> +atoms, replacement from every %(atom) is quoted when and only when it
>> +appears at the top-level (that is, when it appears outside
>> +%($open)...%(end)).
>> +
>> +When a scripting language specific quoting is in effect, everything
>> +between a top-level opening atom and its matching %(end) is evaluated
>> +according to the semantics of the opening atom and its result is
>> +quoted.
>> +
>>
>
> Nice, I like the explanation above.
>

>>
>> +               }
>> +       } else if (!if_then_else->condition_satisfied)
>
> Minor nit. I'm not sure what standard we use here at Git, but
> traditionally, I prefer to see { } blocks on all sections even if only
> one of them needs it. (That is, only drop the braces when every
> section is one line.) It also looks weird with a comment since it
> appears as multiple lines to the reader. I think the braces improve
> readability.
>
> I don't know whether that's Git's code base standard or not, however.
> It's not really worth a re-roll unless something else would need to
> change.
>

>> Minor nit. I'm not sure what standard we use here at Git, but
>> traditionally, I prefer to see { } blocks on all sections even if only
>> one of them needs it. (That is, only drop the braces when every
>> section is one line.) It also looks weird with a comment since it
>> appears as multiple lines to the reader. I think the braces improve
>> readability.
>>
>> I don't know whether that's Git's code base standard or not, however.
>> It's not really worth a re-roll unless something else would need to
>> change.
>>
> I believe this is the syntax followed in Git, xdiff/xmerge.c:173 and so on.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> Minor nit. I'm not sure what standard we use here at Git, but
>>> traditionally, I prefer to see { } blocks on all sections even if only
>>> one of them needs it. (That is, only drop the braces when every
>>> section is one line.) It also looks weird with a comment since it
>>> appears as multiple lines to the reader. I think the braces improve
>>> readability.
>>>
>>> I don't know whether that's Git's code base standard or not, however.
>>> It's not really worth a re-roll unless something else would need to
>>> change.
>>>
>> I believe this is the syntax followed in Git, xdiff/xmerge.c:173 and so on.
>
> That is a bad example for two reasons, if you mean this part:
>
>         if ((size = file->recs[i]->size) &&
>                         file->recs[i]->ptr[size - 1] == '\n')
>                 /* Last line; ends in LF; Is it CR/LF? */
>                 return size > 1 &&
>                         file->recs[i]->ptr[size - 2] == '\r';
> *       if (!i)
>                 /* The only line has no eol */
>                 return -1;
>         /* Determine eol from second-to-last line */
>
>
> What Jacob prefers is this:
>
> ---------------------------------------------
>         if (cond)
>                 simple;
>         else if (cond)
>                 simple;
>         else
>                 simple;
> ---------------------------------------------
>         if (cond) {
>                 simple;
>         } else if (cond) {
>                 no;
>                 longer;
>                 simple;
>         } else {
>                 simple;
>         }
> ---------------------------------------------
>
> That is, as long as all arms of if/else if/.../else cascade is
> simple, {} is used nowhere in the cascade, but once even one of them
> requires {} then all others gain {}.

>> +if::
>> +       Used as %(if)...%(then)...(%end) or
>> +       %(if)...%(then)...%(else)...%(end).  If there is an atom with
>> +       value or string literal after the %(if) then everything after
>> +       the %(then) is printed, else if the %(else) atom is used, then
>> +       everything after %(else) is printed. We ignore space when
>> +       evaluating the string before %(then), this is useful when we
>> +       use the %(HEAD) atom which prints either "*" or " " and we
>> +       want to apply the 'if' condition only on the 'HEAD' ref.
>> +
>>  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.

>> +When a scripting language specific quoting is in effect (i.e. one of
>> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
>> +atoms, replacement from every %(atom) is quoted when and only when it
>> +appears at the top-level (that is, when it appears outside
>> +%($open)...%(end)).

>> +               }
>> +       } else if (!if_then_else->condition_satisfied)
>
> Minor nit. I'm not sure what standard we use here at Git, but
> traditionally, I prefer to see { } blocks on all sections even if only
> one of them needs it. (That is, only drop the braces when every
> section is one line.) It also looks weird with a comment since it
> appears as multiple lines to the reader. I think the braces improve
> readability.
>
> I don't know whether that's Git's code base standard or not, however.
> It's not really worth a re-roll unless something else would need to
> change.
>

>> +               /*
>> +                * No %(else) atom: just drop the %(then) branch if the
>> +                * condition is not satisfied.
>> +                */
>> +               strbuf_reset(&cur->output);

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Ok, so I have only one minor nit, but otherwise this looks quite good
>> to me. A few comments explaining my understanding, but only one
>> suggested
>> change which is really a minor nit and not worth re-rolling just for it.
>
> As you didn't snip parts you didn't comment, I'll use this to add my
> own for convenience ;-)
>
>>> +if::
>>> +       Used as %(if)...%(then)...(%end) or
>>> +       %(if)...%(then)...%(else)...%(end).  If there is an atom with
>>> +       value or string literal after the %(if) then everything after
>>> +       the %(then) is printed, else if the %(else) atom is used, then
>>> +       everything after %(else) is printed. We ignore space when
>>> +       evaluating the string before %(then), this is useful when we
>>> +       use the %(HEAD) atom which prints either "*" or " " and we
>>> +       want to apply the 'if' condition only on the 'HEAD' ref.
>>> +
>>>  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.
>
> I see a few instances of (%end) that were meant to be %(end).
>

> Aren't the following two paragraphs ...
>
>>> +When a scripting language specific quoting is in effect (i.e. one of
>>> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
>>> +atoms, replacement from every %(atom) is quoted when and only when it
>>> +appears at the top-level (that is, when it appears outside
>>> +%($open)...%(end)).
>
>>> +When a scripting language specific quoting is in effect, everything
>>> +between a top-level opening atom and its matching %(end) is evaluated
>>> +according to the semantics of the opening atom and its result is
>>> +quoted.
>
> ... saying the same thing?
>

>
>>> +               }
>>> +       } else if (!if_then_else->condition_satisfied)
>>
>> Minor nit. I'm not sure what standard we use here at Git, but
>> traditionally, I prefer to see { } blocks on all sections even if only
>> one of them needs it. (That is, only drop the braces when every
>> section is one line.) It also looks weird with a comment since it
>> appears as multiple lines to the reader. I think the braces improve
>> readability.
>>
>> I don't know whether that's Git's code base standard or not, however.
>> It's not really worth a re-roll unless something else would need to
>> change.
>>
>
> In principle, we mimick the kernel style of using {} block even on a
> single-liner body in if/else if/else cascade when any one of them is
> not a single-liner and requires {}.  But we often ignore that when a
> truly trivial single liner follows if() even if its else clause is a
> big block, e.g.
>
>         if (cond)
>                 single;
>         else {
>                 big;
>                 block;
>         }
>
> I agree with you that this case should just use {} for the following
> paragraph, because it is technically a single-liner, but comes with
> a big comment block and is very much easier to read with {} around
> it.
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Ensure that each 'atom_value' has a reference to its corresponding
> 'used_atom'. This let's us use values within 'used_atom' in the
> 'handler' function.
>
> Hence we can get the %(align) atom's parameters directly from the
> 'used_atom' therefore removing the necessity of passing %(align) atom's
> parameters to 'atom_value'.
>
> This also acts as a preparatory patch for the upcoming patch where we
> introduce %(if:equals=) and %(if:notequals=).
>

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  ref-filter.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 8c183a0..8392303 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -230,11 +230,9 @@ struct ref_formatting_state {
>
>  struct atom_value {
>         const char *s;
> -       union {
> -               struct align align;
> -       } u;
>         void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
>         unsigned long ul; /* used for sorting when not FIELD_STR */
> +       struct used_atom *atom;
>  };
>
>  /*
> @@ -370,7 +368,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
>         push_stack_element(&state->stack);
>         new = state->stack;
>         new->at_end = end_align_handler;
> -       new->at_end_data = &atomv->u.align;
> +       new->at_end_data = &atomv->atom->u.align;

>  }
>
>  static void if_then_else_handler(struct ref_formatting_stack **stack)
> @@ -1069,6 +1067,7 @@ static void populate_value(struct ref_array_item *ref)
>                 struct branch *branch = NULL;
>
>                 v->handler = append_atom;
> +               v->atom = atom;
>
>                 if (*name == '*') {
>                         deref = 1;
> @@ -1133,7 +1132,6 @@ static void populate_value(struct ref_array_item *ref)
>                                 v->s = " ";
>                         continue;
>                 } else if (starts_with(name, "align")) {
> -                       v->u.align = atom->u.align;
>                         v->handler = align_atom_handler;
>                         continue;
>                 } else if (!strcmp(name, "end")) {
> --
> 2.10.2
>

>>
>>  /*
>> @@ -370,7 +368,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
>>         push_stack_element(&state->stack);
>>         new = state->stack;
>>         new->at_end = end_align_handler;
>> -       new->at_end_data = &atomv->u.align;
>> +       new->at_end_data = &atomv->atom->u.align;
>
> At first, this confused me. I was like "we dropped the union, why are
> we still referencing it. But I realized that the "used_atom" struct
> actually contains the same union and we were copying it.
>
> Ok, so this looks good.
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Implement %(if:equals=<string>) wherein the if condition is only
> satisfied if the value obtained between the %(if:...) and %(then) atom
> is the same as the given '<string>'.
>
> Similarly, implement (if:notequals=<string>) wherein the if condition
> is only satisfied if the value obtained between the %(if:...) and
> %(then) atom is differnt from the given '<string>'.
>
> This is done by introducing 'if_atom_parser()' which parses the given
> %(if) atom and then stores the data in used_atom which is later passed
> on to the used_atom of the %(then) atom, so that it can do the required
> comparisons.
>

> Add tests and Documentation 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 |  3 +++
>  ref-filter.c                       | 43 +++++++++++++++++++++++++++++++++-----
>  t/t6302-for-each-ref-filter.sh     | 18 ++++++++++++++++
>  3 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index fed8126..b7b8560 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -155,6 +155,9 @@ if::
>         evaluating the string before %(then), this is useful when we
>         use the %(HEAD) atom which prints either "*" or " " and we
>         want to apply the 'if' condition only on the 'HEAD' ref.
> +       Append ":equals=<string>" or ":notequals=<string>" to compare
> +       the value between the %(if:...) and %(then) atoms with the
> +       given string.
>

>  In addition to the above, for commit and tag objects, the header
>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> diff --git a/ref-filter.c b/ref-filter.c
> index 8392303..44481c3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -22,6 +22,8 @@ struct align {
>  };
>
>  struct if_then_else {
> +       const char *if_equals,
> +               *not_equals;

>         unsigned int then_atom_seen : 1,
>                 else_atom_seen : 1,
>                 condition_satisfied : 1;
> @@ -49,6 +51,10 @@ static struct used_atom {
>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>                         unsigned int nlines;
>                 } contents;
> +               struct {
> +                       const char *if_equals,
> +                               *not_equals;

> +               } if_then_else;
>                 enum { O_FULL, O_SHORT } objectname;
>         } u;
>  } *used_atom;
> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
>         string_list_clear(&params, 0);
>  }
>
> +static void if_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +       if (!arg)
> +               return;
> +       else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
> +                ;
> +       else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
> +               ;

> +       else
> +               die(_("unrecognized %%(if) argument: %s"), arg);
> +}
> +
> +
>  static struct {
>         const char *name;
>         cmp_type cmp_type;
> @@ -209,7 +228,7 @@ static struct {
>         { "color", FIELD_STR, color_atom_parser },
>         { "align", FIELD_STR, align_atom_parser },
>         { "end" },
> -       { "if" },
> +       { "if", FIELD_STR, if_atom_parser },
>         { "then" },
>         { "else" },
>  };
> @@ -410,6 +429,9 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
>         struct ref_formatting_stack *new;
>         struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
>
> +       if_then_else->if_equals = atomv->atom->u.if_then_else.if_equals;
> +       if_then_else->not_equals = atomv->atom->u.if_then_else.not_equals;
> +
>         push_stack_element(&state->stack);
>         new = state->stack;
>         new->at_end = if_then_else_handler;
> @@ -441,10 +463,17 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
>                 die(_("format: %%(then) atom used after %%(else)"));
>         if_then_else->then_atom_seen = 1;
>         /*
> -        * If there exists non-empty string between the 'if' and
> -        * 'then' atom then the 'if' condition is satisfied.
> +        * If the 'equals' or 'notequals' attribute is used then
> +        * perform the required comparison. If not, only non-empty
> +        * strings satisfy the 'if' condition.
>          */
> -       if (cur->output.len && !is_empty(cur->output.buf))
> +       if (if_then_else->if_equals) {
> +               if (!strcmp(if_then_else->if_equals, cur->output.buf))
> +                       if_then_else->condition_satisfied = 1;

> +       } else  if (if_then_else->not_equals) {
> +               if (strcmp(if_then_else->not_equals, cur->output.buf))
> +                       if_then_else->condition_satisfied = 1;
> +       } else if (cur->output.len && !is_empty(cur->output.buf))
>                 if_then_else->condition_satisfied = 1;
>         strbuf_reset(&cur->output);
>  }
> @@ -1137,7 +1166,11 @@ static void populate_value(struct ref_array_item *ref)
>                 } else if (!strcmp(name, "end")) {
>                         v->handler = end_atom_handler;
>                         continue;
> -               } else if (!strcmp(name, "if")) {
> +               } else if (starts_with(name, "if")) {
> +                       const char *s;
> +
> +                       if (skip_prefix(name, "if:", &s))
> +                               v->s = xstrdup(s);
>                         v->handler = if_atom_handler;
>                         continue;
>                 } else if (!strcmp(name, "then")) {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index fed3013..a09a1a4 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -403,4 +403,22 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'check %(if:equals=<string>)' '
> +       git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
> +       cat >expect <<-\EOF &&
> +       Found master
> +       Not master
> +       EOF
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'check %(if:notequals=<string>)' '
> +       git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
> +       cat >expect <<-\EOF &&
> +       Found master
> +       Not master
> +       EOF
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
> Ok. How does this handle whitespace? The previous if implementation
> treated whitespace as trimming to ignore. Does this require an exact
> whitespace match? It appears by the code that strings must match
> exactly. Would it make more sense to always trim the value of
> whitespace first before comparison? Hmm.. I think we should avoid
> doing that actually.
>

>>  In addition to the above, for commit and tag objects, the header
>>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8392303..44481c3 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -22,6 +22,8 @@ struct align {
>>  };
>>
>>  struct if_then_else {
>> +       const char *if_equals,
>> +               *not_equals;
>
> Ok so we add both if_equals and not_equals values. Could we re-use the
> same string?

>
>>         unsigned int then_atom_seen : 1,
>>                 else_atom_seen : 1,
>>                 condition_satisfied : 1;
>> @@ -49,6 +51,10 @@ static struct used_atom {
>>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>                         unsigned int nlines;
>>                 } contents;
>> +               struct {
>> +                       const char *if_equals,
>> +                               *not_equals;
>
>
> Same here, why do we need both strings here stored separately? Could
> we instead store which state to check and store the string once? I'm
> not sure that really buys us any storage.
>

> On Wed, Nov 9, 2016 at 4:52 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Ok. How does this handle whitespace? The previous if implementation
>> treated whitespace as trimming to ignore. Does this require an exact
>> whitespace match? It appears by the code that strings must match
>> exactly. Would it make more sense to always trim the value of
>> whitespace first before comparison? Hmm.. I think we should avoid
>> doing that actually.
>>
>
> This does not trim whitespace what so ever and requires an exact match.
> I don't see the reason behind trimming whitespace though.
>

>> @@ -49,6 +51,10 @@ static struct used_atom {
>>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>                         unsigned int nlines;
>>                 } contents;
>> +               struct {
>> +                       const char *if_equals,
>> +                               *not_equals;
>
>
> Same here, why do we need both strings here stored separately? Could
> we instead store which state to check and store the string once? I'm
> not sure that really buys us any storage.

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>> @@ -49,6 +51,10 @@ static struct used_atom {
>>>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>>                         unsigned int nlines;
>>>                 } contents;
>>> +               struct {
>>> +                       const char *if_equals,
>>> +                               *not_equals;
>>
>>
>> Same here, why do we need both strings here stored separately? Could
>> we instead store which state to check and store the string once? I'm
>> not sure that really buys us any storage.
>
> I am not sure if storage is an issue, but I tend to agree that it
> would be semantically cleaner if this was done as a pair of <what
> operation uses this string constant?, the string constant>, and the
> former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.
>
> You could later enhance the comparison operator more easily with
> such an arrangement (e.g. if-equals-case-insensitively).

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>> @@ -49,6 +51,10 @@ static struct used_atom {
>>>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>>                         unsigned int nlines;
>>>                 } contents;
>>> +               struct {
>>> +                       const char *if_equals,
>>> +                               *not_equals;
>>
>>
>> Same here, why do we need both strings here stored separately? Could
>> we instead store which state to check and store the string once? I'm
>> not sure that really buys us any storage.
>
> I am not sure if storage is an issue, but I tend to agree that it
> would be semantically cleaner if this was done as a pair of <what
> operation uses this string constant?, the string constant>, and the
> former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.
>
> You could later enhance the comparison operator more easily with
> such an arrangement (e.g. if-equals-case-insensitively).

> From: Karthik Nayak <karthik.188@gmail.com>
> 
> Implement %(if:equals=<string>) wherein the if condition is only
> satisfied if the value obtained between the %(if:...) and %(then) atom
> is the same as the given '<string>'.
> 
> Similarly, implement (if:notequals=<string>) wherein the if condition
> is only satisfied if the value obtained between the %(if:...) and
> %(then) atom is differnt from the given '<string>'.

> 
> This is done by introducing 'if_atom_parser()' which parses the given
> %(if) atom and then stores the data in used_atom which is later passed
> on to the used_atom of the %(then) atom, so that it can do the required
> comparisons.

> 
> Add tests and Documentation 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 |  3 +++
>  ref-filter.c                       | 43 +++++++++++++++++++++++++++++++++-----
>  t/t6302-for-each-ref-filter.sh     | 18 ++++++++++++++++
>  3 files changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index fed8126..b7b8560 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -155,6 +155,9 @@ if::
>  	evaluating the string before %(then), this is useful when we
>  	use the %(HEAD) atom which prints either "*" or " " and we
>  	want to apply the 'if' condition only on the 'HEAD' ref.

> +	Append ":equals=<string>" or ":notequals=<string>" to compare
> +	the value between the %(if:...) and %(then) atoms with the
> +	given string.
>  
>  In addition to the above, for commit and tag objects, the header
>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> diff --git a/ref-filter.c b/ref-filter.c
> index 8392303..44481c3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -22,6 +22,8 @@ struct align {
>  };
>  
>  struct if_then_else {
> +	const char *if_equals,
> +		*not_equals;

>  	unsigned int then_atom_seen : 1,
>  		else_atom_seen : 1,
>  		condition_satisfied : 1;
> @@ -49,6 +51,10 @@ static struct used_atom {
>  			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>  			unsigned int nlines;
>  		} contents;
> +		struct {
> +			const char *if_equals,
> +				*not_equals;
> +		} if_then_else;

>  		enum { O_FULL, O_SHORT } objectname;
>  	} u;
>  } *used_atom;
> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
>  	string_list_clear(&params, 0);
>  }
>  
> +static void if_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +	if (!arg)
> +		return;
> +	else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
> +		 ;
> +	else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
> +		;

> +test_expect_success 'check %(if:equals=<string>)' '
> +	git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
> +	cat >expect <<-\EOF &&
> +	Found master
> +	Not master
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check %(if:notequals=<string>)' '
> +	git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
> +	cat >expect <<-\EOF &&
> +	Found master
> +	Not master
> +	EOF
> +	test_cmp expect actual
> +'

> W dniu 08.11.2016 o 21:11, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Implement %(if:equals=<string>) wherein the if condition is only
>> satisfied if the value obtained between the %(if:...) and %(then) atom
>> is the same as the given '<string>'.
>>
>> Similarly, implement (if:notequals=<string>) wherein the if condition
>> is only satisfied if the value obtained between the %(if:...) and
>> %(then) atom is differnt from the given '<string>'.
>                   ^^^^^^^^
>
> s/differnt/different/   <-- typo
>

>>
>> Add tests and Documentation 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 |  3 +++
>>  ref-filter.c                       | 43 +++++++++++++++++++++++++++++++++-----
>>  t/t6302-for-each-ref-filter.sh     | 18 ++++++++++++++++
>>  3 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index fed8126..b7b8560 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -155,6 +155,9 @@ if::
>>       evaluating the string before %(then), this is useful when we
>>       use the %(HEAD) atom which prints either "*" or " " and we
>>       want to apply the 'if' condition only on the 'HEAD' ref.
>
> So %(if) is actually %(if:notempty) ?  Just kidding.
>

>> +     Append ":equals=<string>" or ":notequals=<string>" to compare
>> +     the value between the %(if:...) and %(then) atoms with the
>> +     given string.
>>
>>  In addition to the above, for commit and tag objects, the header
>>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8392303..44481c3 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -22,6 +22,8 @@ struct align {
>>  };
>>
>>  struct if_then_else {
>> +     const char *if_equals,
>> +             *not_equals;
>
> I guess using anonymous structs from C11 here...
>
>>       unsigned int then_atom_seen : 1,
>>               else_atom_seen : 1,
>>               condition_satisfied : 1;
>> @@ -49,6 +51,10 @@ static struct used_atom {
>>                       enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>                       unsigned int nlines;
>>               } contents;
>> +             struct {
>> +                     const char *if_equals,
>> +                             *not_equals;
>> +             } if_then_else;
>
> ...to avoid code duplication there is rather out of question?
>

>>               enum { O_FULL, O_SHORT } objectname;
>>       } u;
>>  } *used_atom;
>> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
>>       string_list_clear(&params, 0);
>>  }
>>
>> +static void if_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +     if (!arg)
>> +             return;
>> +     else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
>> +              ;
>> +     else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
>> +             ;
>
> Those ';' should be perfectly aligned, isn't it?
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(objectname:short=<length>) which would print the
> abbreviated unique objectname of given length. When no length is
> specified, the length is 'DEFAULT_ABBREV'. The minimum length is
> 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided
> object name is unique.
>

> Add tests and documentation for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Helped-by: Jacob Keller <jacob.keller@gmail.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  4 ++++
>  ref-filter.c                       | 25 +++++++++++++++++++------
>  t/t6300-for-each-ref.sh            | 10 ++++++++++
>  3 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index b7b8560..92184c4 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -107,6 +107,10 @@ objectsize::
>  objectname::
>         The object name (aka SHA-1).
>         For a non-ambiguous abbreviation of the object name append `:short`.
> +       For an abbreviation of the object name with desired length append
> +       `:short=<length>`, where the minimum length is MINIMUM_ABBREV. The
> +       length may be exceeded to ensure unique object names.
> +
>
>  upstream::
>         The name of a local ref which can be considered ``upstream''
> diff --git a/ref-filter.c b/ref-filter.c
> index 44481c3..fe4ea2b 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -55,7 +55,10 @@ static struct used_atom {
>                         const char *if_equals,
>                                 *not_equals;
>                 } if_then_else;
> -               enum { O_FULL, O_SHORT } objectname;
> +               struct {
> +                       enum { O_FULL, O_LENGTH, O_SHORT } option;
> +                       unsigned int length;
> +               } objectname;
>         } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
> @@ -118,10 +121,17 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)
>  static void objectname_atom_parser(struct used_atom *atom, const char *arg)
>  {
>         if (!arg)
> -               atom->u.objectname = O_FULL;
> +               atom->u.objectname.option = O_FULL;
>         else if (!strcmp(arg, "short"))
> -               atom->u.objectname = O_SHORT;
> -       else
> +               atom->u.objectname.option = O_SHORT;
> +       else if (skip_prefix(arg, "short=", &arg)) {
> +               atom->u.objectname.option = O_LENGTH;
> +               if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
> +                   atom->u.objectname.length == 0)
> +                       die(_("positive value expected objectname:short=%s"), arg);
> +               if (atom->u.objectname.length < MINIMUM_ABBREV)
> +                       atom->u.objectname.length = MINIMUM_ABBREV;

> +       } else
>                 die(_("unrecognized %%(objectname) argument: %s"), arg);
>  }
>
> @@ -591,12 +601,15 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
>                            struct atom_value *v, struct used_atom *atom)
>  {
>         if (starts_with(name, "objectname")) {
> -               if (atom->u.objectname == O_SHORT) {
> +               if (atom->u.objectname.option == O_SHORT) {
>                         v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>                         return 1;

> -               } else if (atom->u.objectname == O_FULL) {
> +               } else if (atom->u.objectname.option == O_FULL) {
>                         v->s = xstrdup(sha1_to_hex(sha1));
>                         return 1;
> +               } else if (atom->u.objectname.option == O_LENGTH) {
> +                       v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
> +                       return 1;
>                 } else
>                         die("BUG: unknown %%(objectname) option");
>         }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 19a2823..2be0a3f 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -60,6 +60,8 @@ test_atom head objecttype commit
>  test_atom head objectsize 171
>  test_atom head objectname $(git rev-parse refs/heads/master)
>  test_atom head objectname:short $(git rev-parse --short refs/heads/master)
> +test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
> +test_atom head objectname:short=10 $(git rev-parse --short=10 refs/heads/master)
>  test_atom head tree $(git rev-parse refs/heads/master^{tree})
>  test_atom head parent ''
>  test_atom head numparent 0
> @@ -99,6 +101,8 @@ test_atom tag objecttype tag
>  test_atom tag objectsize 154
>  test_atom tag objectname $(git rev-parse refs/tags/testtag)
>  test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)
> +test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
> +test_atom head objectname:short=10 $(git rev-parse --short=10 refs/heads/master)
>  test_atom tag tree ''
>  test_atom tag parent ''
>  test_atom tag numparent ''
> @@ -164,6 +168,12 @@ test_expect_success 'Check invalid format specifiers are errors' '
>         test_must_fail git for-each-ref --format="%(authordate:INVALID)" refs/heads
>  '
>
> +test_expect_success 'arguments to %(objectname:short=) must be positive integers' '
> +       test_must_fail git for-each-ref --format="%(objectname:short=0)" &&
> +       test_must_fail git for-each-ref --format="%(objectname:short=-1)" &&
> +       test_must_fail git for-each-ref --format="%(objectname:short=foo)"
> +'
> +
>  test_date () {
>         f=$1 &&
>         committer_date=$2 &&
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(objectname:short=<length>) which would print the
>> abbreviated unique objectname of given length. When no length is
>> specified, the length is 'DEFAULT_ABBREV'. The minimum length is
>> 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided
>> object name is unique.
>>
>
> Ok this makes sense. It may be annoying that the length might go
> beyond the size that we wanted, but I think it's better than printing
> a non-unique short abbreviation.
>
> I have one suggested change, which is to drop O_LENGTH and have
> O_SHORT store the length always, setting it to DEFAULT_ABBREV when no
> length provided. This allows you to drop some code. I don't think it's
> actually worth a re-roll by itself since the current code is correct.
>
> Thanks,
> Jake
>

> On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Add support for %(objectname:short=<length>) which would print the
>>> abbreviated unique objectname of given length. When no length is
>>> specified, the length is 'DEFAULT_ABBREV'. The minimum length is
>>> 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided
>>> object name is unique.
>>>
>>
>> Ok this makes sense. It may be annoying that the length might go
>> beyond the size that we wanted, but I think it's better than printing
>> a non-unique short abbreviation.
>>
>> I have one suggested change, which is to drop O_LENGTH and have
>> O_SHORT store the length always, setting it to DEFAULT_ABBREV when no
>> length provided. This allows you to drop some code. I don't think it's
>> actually worth a re-roll by itself since the current code is correct.
>>
>> Thanks,
>> Jake
>>
>
> That does make sense, It would also not error out when we use
> %(objectname:short=) and
> not specify the length. Idk, if that's desirable or not. But it does
> make the code a little more
> confusing to read at the same time.
>

> So since its a small change, I'd be okay going either ways with this.
>
> --
> Regards,
> Karthik Nayak

> On Thu, Nov 10, 2016 at 9:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>
>> That does make sense, It would also not error out when we use
>> %(objectname:short=) and
>> not specify the length. Idk, if that's desirable or not. But it does
>> make the code a little more
>> confusing to read at the same time.
>>
>
> I am not sure that would be the case. If you see "objectname:short"
> you trreat this as if they had passed "objectname:short=<default
> abbrev>" but if you see "objectname:short=" you die, no?
>

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>       else if (!strcmp(arg, "short"))
>> -             atom->u.objectname = O_SHORT;
>> -     else
>> +             atom->u.objectname.option = O_SHORT;
>> +     else if (skip_prefix(arg, "short=", &arg)) {
>> +             atom->u.objectname.option = O_LENGTH;
>> +             if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
>> +                 atom->u.objectname.length == 0)
>> +                     die(_("positive value expected objectname:short=%s"), arg);
>> +             if (atom->u.objectname.length < MINIMUM_ABBREV)
>> +                     atom->u.objectname.length = MINIMUM_ABBREV;
>> +     } else
>>               die(_("unrecognized %%(objectname) argument: %s"), arg);
>>  }
>
> Users who want to use the default-abbrev, i.e. the autoscaling one
> introduced recently, must use "short", not "short=-1", with this
> code (especially with the "must be at least MINIMUM_ABBREV" logic),
> but I do not think it is a problem, so I think this is good.
>

>  	else if (!strcmp(arg, "short"))
> -		atom->u.objectname = O_SHORT;
> -	else
> +		atom->u.objectname.option = O_SHORT;
> +	else if (skip_prefix(arg, "short=", &arg)) {
> +		atom->u.objectname.option = O_LENGTH;
> +		if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
> +		    atom->u.objectname.length == 0)
> +			die(_("positive value expected objectname:short=%s"), arg);
> +		if (atom->u.objectname.length < MINIMUM_ABBREV)
> +			atom->u.objectname.length = MINIMUM_ABBREV;
> +	} else
>  		die(_("unrecognized %%(objectname) argument: %s"), arg);
>  }

> @@ -591,12 +601,15 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
>  			   struct atom_value *v, struct used_atom *atom)
>  {
>  	if (starts_with(name, "objectname")) {
> -		if (atom->u.objectname == O_SHORT) {
> +		if (atom->u.objectname.option == O_SHORT) {
>  			v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>  			return 1;
> -		} else if (atom->u.objectname == O_FULL) {
> +		} else if (atom->u.objectname.option == O_FULL) {
>  			v->s = xstrdup(sha1_to_hex(sha1));
>  			return 1;
> +		} else if (atom->u.objectname.option == O_LENGTH) {
> +			v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
> +			return 1;

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Move the implementation of get_head_description() from branch.c to
> ref-filter.  This gives a description of the HEAD ref if called. This
> is used as the refname for the HEAD ref whenever the
> FILTER_REFS_DETACHED_HEAD option is used. Make it public because we
> need it to calculate the length of the HEAD refs description in
> branch.c:calc_maxwidth() when we port branch.c to use ref-filter
> APIs.
>

>
> -               if (starts_with(name, "refname"))
> +               if (starts_with(name, "refname")) {
>                         refname = ref->refname;
> -               else if (starts_with(name, "symref"))
> +                       if (ref->kind & FILTER_REFS_DETACHED_HEAD)
> +                               refname = get_head_description();

> +               } else if (starts_with(name, "symref"))
>                         refname = ref->symref ? ref->symref : "";
>                 else if (starts_with(name, "upstream")) {
>                         const char *branch_name;
> diff --git a/ref-filter.h b/ref-filter.h
> index 14d435e..4aea594 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -106,5 +106,7 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
>  struct ref_sorting *ref_default_sorting(void);
>  /*  Function to parse --merged and --no-merged options */
>  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
> +/*  Get the current HEAD's description */
> +char *get_head_description(void);
>
>  #endif /*  REF_FILTER_H  */
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
>>
>> -               if (starts_with(name, "refname"))
>> +               if (starts_with(name, "refname")) {
>>                         refname = ref->refname;
>> -               else if (starts_with(name, "symref"))
>> +                       if (ref->kind & FILTER_REFS_DETACHED_HEAD)
>> +                               refname = get_head_description();
>
> Since this (I think?) changes behavior of refname would it make sense
> to add a test for this?
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> To allow column display, we will need to first render the output in a
> string list to allow print_columns() to compute the proper size of
> each column before starting the actual output. Introduce the function
> format_ref_array_item() that does the formatting of a ref_array_item
> to an strbuf.
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(upstream:track,nobracket) which will print the
> tracking information without the brackets (i.e. "ahead N, behind M").
> This is needed when we port branch.c to use ref-filter's printing APIs.
>

> Add test and documentation 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                       | 67 +++++++++++++++++++++++++-------------
>  t/t6300-for-each-ref.sh            |  2 ++
>  3 files changed, 51 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index fd365eb..3953431 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -118,9 +118,11 @@ upstream::
>         `refname` above.  Additionally respects `:track` to show
>         "[ahead N, behind M]" and `:trackshort` to show the terse
>         version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> -       or "=" (in sync).  Has no effect if the ref does not have
> -       tracking information associated with it. `:track` also prints
> -       "[gone]" whenever unknown upstream ref is encountered.
> +       or "=" (in sync). `:track` also prints "[gone]" whenever
> +       unknown upstream ref is encountered. Append `:track,nobracket`
> +       to show tracking information without brackets (i.e "ahead N,
> +       behind M").  Has no effect if the ref does not have tracking
> +       information associated with it.
>

>  push::
>         The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index 6d51b80..4d7e414 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -46,8 +46,10 @@ static struct used_atom {
>         union {
>                 char color[COLOR_MAXLEN];
>                 struct align align;
> -               enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
> -                       remote_ref;
> +               struct {
> +                       enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
> +                       unsigned int nobracket: 1;
> +               } remote_ref;
>                 struct {
>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>                         unsigned int nlines;
> @@ -75,16 +77,33 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>
>  static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>  {
> -       if (!arg)
> -               atom->u.remote_ref = RR_NORMAL;
> -       else if (!strcmp(arg, "short"))
> -               atom->u.remote_ref = RR_SHORTEN;
> -       else if (!strcmp(arg, "track"))
> -               atom->u.remote_ref = RR_TRACK;
> -       else if (!strcmp(arg, "trackshort"))
> -               atom->u.remote_ref = RR_TRACKSHORT;
> -       else
> -               die(_("unrecognized format: %%(%s)"), atom->name);
> +       struct string_list params = STRING_LIST_INIT_DUP;
> +       int i;
> +
> +       if (!arg) {
> +               atom->u.remote_ref.option = RR_NORMAL;
> +               return;
> +       }
> +
> +       atom->u.remote_ref.nobracket = 0;
> +       string_list_split(&params, arg, ',', -1);
> +
> +       for (i = 0; i < params.nr; i++) {
> +               const char *s = params.items[i].string;
> +
> +               if (!strcmp(s, "short"))
> +                       atom->u.remote_ref.option = RR_SHORTEN;
> +               else if (!strcmp(s, "track"))

> +                       atom->u.remote_ref.option = RR_TRACK;
> +               else if (!strcmp(s, "trackshort"))
> +                       atom->u.remote_ref.option = RR_TRACKSHORT;
> +               else if (!strcmp(s, "nobracket"))
> +                       atom->u.remote_ref.nobracket = 1;
> +               else
> +                       die(_("unrecognized format: %%(%s)"), atom->name);
> +       }
> +
> +       string_list_clear(&params, 0);
>  }
>
>  static void body_atom_parser(struct used_atom *atom, const char *arg)
> @@ -1045,25 +1064,27 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>                                     struct branch *branch, const char **s)
>  {
>         int num_ours, num_theirs;
> -       if (atom->u.remote_ref == RR_SHORTEN)
> +       if (atom->u.remote_ref.option == RR_SHORTEN)
>                 *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> -       else if (atom->u.remote_ref == RR_TRACK) {
> +       else if (atom->u.remote_ref.option == RR_TRACK) {
>                 if (stat_tracking_info(branch, &num_ours,
>                                        &num_theirs, NULL)) {
> -                       *s = "[gone]";
> -                       return;
> -               }
> -
> -               if (!num_ours && !num_theirs)
> +                       *s = xstrdup("gone");
> +               } else if (!num_ours && !num_theirs)
>                         *s = "";
>                 else if (!num_ours)
> -                       *s = xstrfmt("[behind %d]", num_theirs);
> +                       *s = xstrfmt("behind %d", num_theirs);
>                 else if (!num_theirs)
> -                       *s = xstrfmt("[ahead %d]", num_ours);
> +                       *s = xstrfmt("ahead %d", num_ours);
>                 else
> -                       *s = xstrfmt("[ahead %d, behind %d]",
> +                       *s = xstrfmt("ahead %d, behind %d",
>                                      num_ours, num_theirs);
> -       } else if (atom->u.remote_ref == RR_TRACKSHORT) {
> +               if (!atom->u.remote_ref.nobracket && *s[0]) {
> +                       const char *to_free = *s;
> +                       *s = xstrfmt("[%s]", *s);
> +                       free((void *)to_free);
> +               }
> +       } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
>                 if (stat_tracking_info(branch, &num_ours,
>                                        &num_theirs, NULL))
>                         return;
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index a92b36f..2c5f177 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -372,6 +372,8 @@ test_expect_success 'setup for upstream:track[short]' '
>
>  test_atom head upstream:track '[ahead 1]'
>  test_atom head upstream:trackshort '>'
> +test_atom head upstream:track,nobracket 'ahead 1'
> +test_atom head upstream:nobracket,track 'ahead 1'
>  test_atom head push:track '[ahead 1]'
>  test_atom head push:trackshort '>'
>
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>> This is needed when we port branch.c to use ref-filter's printing APIs.
>>
>
> Makes sense. Seems a bit weird that we have the brackets normally
> rather than adding them as an option, but I think this is ok. We don't
> want to change all previous uses in this case.
>
> My only suggestion here would be to add code so that the options die()
> when we use nobracket along with trackshort or without track. This
> ensures that the nobracket option only applies to track mode?
>

>> Add test and documentation 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                       | 67 +++++++++++++++++++++++++-------------
>>  t/t6300-for-each-ref.sh            |  2 ++
>>  3 files changed, 51 insertions(+), 26 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index fd365eb..3953431 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -118,9 +118,11 @@ upstream::
>>         `refname` above.  Additionally respects `:track` to show
>>         "[ahead N, behind M]" and `:trackshort` to show the terse
>>         version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> -       or "=" (in sync).  Has no effect if the ref does not have
>> -       tracking information associated with it. `:track` also prints
>> -       "[gone]" whenever unknown upstream ref is encountered.
>> +       or "=" (in sync). `:track` also prints "[gone]" whenever
>> +       unknown upstream ref is encountered. Append `:track,nobracket`
>> +       to show tracking information without brackets (i.e "ahead N,
>> +       behind M").  Has no effect if the ref does not have tracking
>> +       information associated with it.
>>
>
> Ok so my comment on the previous patch is fixed here, the new wording
> makes it much more clear that [gone] is not the same thing as no
> information. So I don't think we should bother changing the previous
> patch in the series. This might want to document that nobracket works
> even without track, even if it doesn't actually do anything? Or make
> the code more strict in that we die() if the values are put together
> that make no sense?
>

>>  push::
>>         The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6d51b80..4d7e414 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -46,8 +46,10 @@ static struct used_atom {
>>         union {
>>                 char color[COLOR_MAXLEN];
>>                 struct align align;
>> -               enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>> -                       remote_ref;
>> +               struct {
>> +                       enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
>> +                       unsigned int nobracket: 1;
>> +               } remote_ref;
>>                 struct {
>>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>                         unsigned int nlines;
>> @@ -75,16 +77,33 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>>
>>  static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>>  {
>> -       if (!arg)
>> -               atom->u.remote_ref = RR_NORMAL;
>> -       else if (!strcmp(arg, "short"))
>> -               atom->u.remote_ref = RR_SHORTEN;
>> -       else if (!strcmp(arg, "track"))
>> -               atom->u.remote_ref = RR_TRACK;
>> -       else if (!strcmp(arg, "trackshort"))
>> -               atom->u.remote_ref = RR_TRACKSHORT;
>> -       else
>> -               die(_("unrecognized format: %%(%s)"), atom->name);
>> +       struct string_list params = STRING_LIST_INIT_DUP;
>> +       int i;
>> +
>> +       if (!arg) {
>> +               atom->u.remote_ref.option = RR_NORMAL;
>> +               return;
>> +       }
>> +
>> +       atom->u.remote_ref.nobracket = 0;
>> +       string_list_split(&params, arg, ',', -1);
>> +
>> +       for (i = 0; i < params.nr; i++) {
>> +               const char *s = params.items[i].string;
>> +
>> +               if (!strcmp(s, "short"))
>> +                       atom->u.remote_ref.option = RR_SHORTEN;
>> +               else if (!strcmp(s, "track"))
>
> Should we add die()s here to disallow setting the remote_ref option
> multiple times? Otherwise, we should document that the last one wins?
> Not sure it's really a big deal here, but we could do it to ensure
> consistency for options which don't make sense together?
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Borrowing from branch.c's implementation print "[gone]" whenever an
> unknown upstream ref is encountered instead of just ignoring it.
>

> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Helped-by : Jacob Keller <jacob.keller@gmail.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt | 3 ++-
>  ref-filter.c                       | 4 +++-
>  t/t6300-for-each-ref.sh            | 2 +-
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 92184c4..fd365eb 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -119,7 +119,8 @@ upstream::
>         "[ahead N, behind M]" and `:trackshort` to show the terse
>         version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>         or "=" (in sync).  Has no effect if the ref does not have
> -       tracking information associated with it.
> +       tracking information associated with it. `:track` also prints
> +       "[gone]" whenever unknown upstream ref is encountered.
>

>  push::
>         The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index b8b8a95..6d51b80 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1049,8 +1049,10 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>                 *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
>         else if (atom->u.remote_ref == RR_TRACK) {
>                 if (stat_tracking_info(branch, &num_ours,
> -                                      &num_theirs, NULL))
> +                                      &num_theirs, NULL)) {
> +                       *s = "[gone]";
>                         return;
> +               }
>
>                 if (!num_ours && !num_theirs)
>                         *s = "";
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2be0a3f..a92b36f 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -382,7 +382,7 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
>
>  test_expect_success 'Check that :track[short] works when upstream is invalid' '
>         cat >expected <<-\EOF &&
> -
> +       [gone]
>
>         EOF
>         test_when_finished "git config branch.master.merge refs/heads/master" &&
> --
> 2.10.2
>

>> Make changes to t/t6300-for-each-ref.sh and
>> Documentation/git-for-each-ref.txt to reflect this change.
>>
>
> This will change behavior if people were expecting it to remain
> silent, but I think this could be considered a bug.
>

>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Helped-by : Jacob Keller <jacob.keller@gmail.com>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  Documentation/git-for-each-ref.txt | 3 ++-
>>  ref-filter.c                       | 4 +++-
>>  t/t6300-for-each-ref.sh            | 2 +-
>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 92184c4..fd365eb 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -119,7 +119,8 @@ upstream::
>>         "[ahead N, behind M]" and `:trackshort` to show the terse
>>         version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>>         or "=" (in sync).  Has no effect if the ref does not have
>> -       tracking information associated with it.
>> +       tracking information associated with it. `:track` also prints
>> +       "[gone]" whenever unknown upstream ref is encountered.
>>
>
> I think this is poorly worded. If I understand, "has no effect if the
> ref does not have tracking information" so in that case we still print
> nothing, right? but otherwise we print [gone] only when the upstream
> ref no longer actually exists locally? I wonder if there is a better
> wording for this? I don't have one. Any suggestions to avoid confusing
> these two cases?
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> The "%(symref)" atom doesn't work when used with the ':short' modifier
> because we strictly match only 'symref' for setting the 'need_symref'
> indicator. Fix this by using comparing with valid_atom rather than used_atom.
>

> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  ref-filter.c            |  2 +-
>  t/t6300-for-each-ref.sh | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 4d7e414..5666814 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -338,7 +338,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>                 valid_atom[i].parser(&used_atom[at], arg);
>         if (*atom == '*')
>                 need_tagged = 1;
> -       if (!strcmp(used_atom[at].name, "symref"))
> +       if (!strcmp(valid_atom[i].name, "symref"))
>                 need_symref = 1;
>         return at;
>  }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2c5f177..b06ea1c 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -38,6 +38,7 @@ test_atom() {
>         case "$1" in
>                 head) ref=refs/heads/master ;;
>                  tag) ref=refs/tags/testtag ;;
> +                sym) ref=refs/heads/sym ;;
>                    *) ref=$1 ;;
>         esac
>         printf '%s\n' "$3" >expected
> @@ -565,4 +566,27 @@ test_expect_success 'Verify sort with multiple keys' '
>                 refs/tags/bogo refs/tags/master > actual &&
>         test_cmp expected actual
>  '
> +
> +test_expect_success 'Add symbolic ref for the following tests' '
> +       git symbolic-ref refs/heads/sym refs/heads/master
> +'
> +
> +cat >expected <<EOF
> +refs/heads/master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref) atom' '
> +       git for-each-ref --format="%(symref)" refs/heads/sym > actual &&
> +       test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +heads/master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:short) atom' '
> +       git for-each-ref --format="%(symref:short)" refs/heads/sym > actual &&
> +       test_cmp expected actual
> +'
> +
>  test_done
> --
> 2.10.2
>

> +test_expect_success 'Add symbolic ref for the following tests' '
> +	git symbolic-ref refs/heads/sym refs/heads/master
> +'
> +
> +cat >expected <<EOF
> +refs/heads/master
> +EOF

> +	test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +heads/master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:short) atom' '
> +	git for-each-ref --format="%(symref:short)" refs/heads/sym > actual &&
> +	test_cmp expected actual
> +'

> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
> [...]
>
>> +test_expect_success 'Add symbolic ref for the following tests' '
>> +     git symbolic-ref refs/heads/sym refs/heads/master
>> +'
>> +
>> +cat >expected <<EOF
>> +refs/heads/master
>> +EOF
>
> This should be inside the relevant test, not outside.  In other
> patches in this series you are putting setup together with the
> rest of test, by using "cat >expected <<-\EOF".
>

>> +
>> +test_expect_success 'Verify usage of %(symref) atom' '
>> +     git for-each-ref --format="%(symref)" refs/heads/sym > actual &&
>
> This should be spelled " >actual", rather than " > actual"; there
> should be no space between redirection and file name.
>
>> +     test_cmp expected actual
>> +'
>> +
>> +cat >expected <<EOF
>> +heads/master
>> +EOF
>> +
>> +test_expect_success 'Verify usage of %(symref:short) atom' '
>> +     git for-each-ref --format="%(symref:short)" refs/heads/sym > actual &&
>> +     test_cmp expected actual
>> +'
>
> Same here.
>

> ground for parsing them. This would allow us to share implementations of
> the atom modifiers between these atoms.
> 
> Introduce refname_atom_parser_internal() to act as a common parsing
> function for ref printing atoms. This would eventually be used to
> introduce refname_atom_parser() and symref_atom_parser() and also be
> internally used in remote_ref_atom_parser().
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---

> +static void refname_atom_parser_internal(struct refname_atom *atom,
> +					 const char *arg, const char *name)
> +{
> +	if (!arg)
> +		atom->option = R_NORMAL;
> +	else if (!strcmp(arg, "short"))
> +		atom->option = R_SHORT;
> +	else if (skip_prefix(arg, "strip=", &arg)) {
> +		atom->option = R_STRIP;
> +		if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
> +			die(_("positive value expected refname:strip=%s"), arg);
> +	} 	else

> +		die(_("unrecognized %%(%s) argument: %s"), name, arg);
> +}
> +
>  static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>  {
>  	struct string_list params = STRING_LIST_INIT_DUP;
> 

> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Since there are multiple atoms which print refs ('%(refname)',
>> '%(symref)', '%(push)', '%upstream'), it makes sense to have a common
>
> Minor typo; it should be: "%(upstream)"
>

>> ground for parsing them. This would allow us to share implementations of
>> the atom modifiers between these atoms.
>>
>> Introduce refname_atom_parser_internal() to act as a common parsing
>> function for ref printing atoms. This would eventually be used to
>> introduce refname_atom_parser() and symref_atom_parser() and also be
>> internally used in remote_ref_atom_parser().
>>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
> [...]
>
>> +static void refname_atom_parser_internal(struct refname_atom *atom,
>> +                                      const char *arg, const char *name)
>> +{
>> +     if (!arg)
>> +             atom->option = R_NORMAL;
>> +     else if (!strcmp(arg, "short"))
>> +             atom->option = R_SHORT;
>> +     else if (skip_prefix(arg, "strip=", &arg)) {
>> +             atom->option = R_STRIP;
>> +             if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
>> +                     die(_("positive value expected refname:strip=%s"), arg);
>> +     }       else
>           ^^^^^^
>
> It looks like you have spurious tab here.
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Using refname_atom_parser_internal(), introduce symref_atom_parser() and
> refname_atom_parser() which will parse the atoms %(symref) and
> %(refname) respectively. Store the parsed information into the
> 'used_atom' structure based on the modifiers used along with the atoms.
>
> Now the '%(symref)' atom supports the ':strip' atom modifier. Update the
> Documentation and tests to reflect this.
>

> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  5 +++
>  ref-filter.c                       | 78 ++++++++++++++++++++++----------------
>  t/t6300-for-each-ref.sh            |  9 +++++
>  3 files changed, 59 insertions(+), 33 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 3953431..a669a32 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -166,6 +166,11 @@ if::
>         the value between the %(if:...) and %(then) atoms with the
>         given string.
>
> +symref::
> +       The ref which the given symbolic ref refers to. If not a
> +       symbolic ref, nothing is printed. Respects the `:short` and
> +       `:strip` options in the same way as `refname` above.
> +
>  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 aad537d..f1d27b5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -176,6 +176,16 @@ static void objectname_atom_parser(struct used_atom *atom, const char *arg)
>                 die(_("unrecognized %%(objectname) argument: %s"), arg);
>  }
>
> +static void symref_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +       return refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
> +}
> +
> +static void refname_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +       return refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
> +}
> +

>  static align_type parse_align_position(const char *s)
>  {
>         if (!strcmp(s, "right"))
> @@ -244,7 +254,7 @@ static struct {
>         cmp_type cmp_type;
>         void (*parser)(struct used_atom *atom, const char *arg);
>  } valid_atom[] = {
> -       { "refname" },
> +       { "refname" , FIELD_STR, refname_atom_parser },
>         { "objecttype" },
>         { "objectsize", FIELD_ULONG },
>         { "objectname", FIELD_STR, objectname_atom_parser },
> @@ -273,7 +283,7 @@ static struct {
>         { "contents", FIELD_STR, contents_atom_parser },
>         { "upstream", FIELD_STR, remote_ref_atom_parser },
>         { "push", FIELD_STR, remote_ref_atom_parser },
> -       { "symref" },
> +       { "symref", FIELD_STR, symref_atom_parser },
>         { "flag" },
>         { "HEAD" },
>         { "color", FIELD_STR, color_atom_parser },
> @@ -1058,21 +1068,16 @@ static inline char *copy_advance(char *dst, const char *src)
>         return dst;
>  }
>
> -static const char *strip_ref_components(const char *refname, const char *nr_arg)
> +static const char *strip_ref_components(const char *refname, unsigned int len)
>  {
> -       char *end;
> -       long nr = strtol(nr_arg, &end, 10);
> -       long remaining = nr;
> +       long remaining = len;
>         const char *start = refname;
>
> -       if (nr < 1 || *end != '\0')
> -               die(_(":strip= requires a positive integer argument"));
> -
>         while (remaining) {
>                 switch (*start++) {
>                 case '\0':
> -                       die(_("ref '%s' does not have %ld components to :strip"),
> -                           refname, nr);
> +                       die(_("ref '%s' does not have %ud components to :strip"),
> +                           refname, len);
>                 case '/':
>                         remaining--;
>                         break;
> @@ -1081,6 +1086,16 @@ static const char *strip_ref_components(const char *refname, const char *nr_arg)
>         return start;
>  }
>
> +static const char *show_ref(struct refname_atom *atom, const char *refname)
> +{
> +       if (atom->option == R_SHORT)
> +               return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> +       else if (atom->option == R_STRIP)
> +               return strip_ref_components(refname, atom->strip);
> +       else
> +               return refname;
> +}
> +
>  static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>                                     struct branch *branch, const char **s)
>  {
> @@ -1153,6 +1168,21 @@ char *get_head_description(void)
>         return strbuf_detach(&desc, NULL);
>  }
>
> +static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref)
> +{
> +       if (!ref->symref)
> +               return "";
> +       else
> +               return show_ref(&atom->u.refname, ref->symref);
> +}
> +
> +static const char *get_refname(struct used_atom *atom, struct ref_array_item *ref)
> +{
> +       if (ref->kind & FILTER_REFS_DETACHED_HEAD)
> +               return get_head_description();
> +       return show_ref(&atom->u.refname, ref->refname);
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -1181,7 +1211,6 @@ static void populate_value(struct ref_array_item *ref)
>                 struct atom_value *v = &ref->value[i];
>                 int deref = 0;
>                 const char *refname;
> -               const char *formatp;
>                 struct branch *branch = NULL;
>
>                 v->handler = append_atom;
> @@ -1192,12 +1221,10 @@ static void populate_value(struct ref_array_item *ref)
>                         name++;
>                 }
>
> -               if (starts_with(name, "refname")) {
> -                       refname = ref->refname;
> -                       if (ref->kind & FILTER_REFS_DETACHED_HEAD)
> -                               refname = get_head_description();
> -               } else if (starts_with(name, "symref"))
> -                       refname = ref->symref ? ref->symref : "";
> +               if (starts_with(name, "refname"))
> +                       refname = get_refname(atom, ref);
> +               else if (starts_with(name, "symref"))
> +                       refname = get_symref(atom, ref);
>                 else if (starts_with(name, "upstream")) {
>                         const char *branch_name;
>                         /* only local branches may have an upstream */
> @@ -1273,21 +1300,6 @@ static void populate_value(struct ref_array_item *ref)
>                 } else
>                         continue;
>
> -               formatp = strchr(name, ':');
> -               if (formatp) {
> -                       const char *arg;
> -
> -                       formatp++;
> -                       if (!strcmp(formatp, "short"))
> -                               refname = shorten_unambiguous_ref(refname,
> -                                                     warn_ambiguous_refs);
> -                       else if (skip_prefix(formatp, "strip=", &arg))
> -                               refname = strip_ref_components(refname, arg);
> -                       else
> -                               die(_("unknown %.*s format %s"),
> -                                   (int)(formatp - name), name, formatp);
> -               }
> -
>                 if (!deref)
>                         v->s = refname;
>                 else
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index b06ea1c..3d28234 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -589,4 +589,13 @@ test_expect_success 'Verify usage of %(symref:short) atom' '
>         test_cmp expected actual
>  '
>
> +cat >expected <<EOF
> +master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:strip) atom' '
> +       git for-each-ref --format="%(symref:strip=2)" refs/heads/sym > actual &&
> +       test_cmp expected actual
> +'
> +
>  test_done
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>
> What's the reasoning for using separate functions here if they are
> exactly identical except for name? Do we intend to add separate
> options for this? I don't really have a problem with separate
> functions here since it helps avoid confusion but they are identical
> otherwise...
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Use the recently introduced refname_atom_parser_internal() within
> remote_ref_atom_parser(), this provides a common base for all the ref
> printing atoms, allowing %(upstream) and %(push) to also use the
> ':strip' option.
>
> The atoms '%(push)' and '%(upstream)' will retain the ':track' and
> ':trackshort' atom modifiers to themselves as they have no meaning in
> context to the '%(refname)' and '%(symref)' atoms.
>
> Update the documentation and tests to reflect the same.
>

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt | 27 ++++++++++++++-------------
>  ref-filter.c                       | 26 +++++++++++++++-----------
>  t/t6300-for-each-ref.sh            |  2 ++
>  3 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index a669a32..600b703 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -114,21 +114,22 @@ objectname::
>
>  upstream::
>         The name of a local ref which can be considered ``upstream''
> -       from the displayed ref. Respects `:short` in the same way as
> -       `refname` above.  Additionally respects `:track` to show
> -       "[ahead N, behind M]" and `:trackshort` to show the terse
> -       version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> -       or "=" (in sync). `:track` also prints "[gone]" whenever
> -       unknown upstream ref is encountered. Append `:track,nobracket`
> -       to show tracking information without brackets (i.e "ahead N,
> -       behind M").  Has no effect if the ref does not have tracking
> -       information associated with it.
> +       from the displayed ref. Respects `:short` and `:strip` in the
> +       same way as `refname` above.  Additionally respects `:track`
> +       to show "[ahead N, behind M]" and `:trackshort` to show the
> +       terse version: ">" (ahead), "<" (behind), "<>" (ahead and
> +       behind), or "=" (in sync). `:track` also prints "[gone]"
> +       whenever unknown upstream ref is encountered. Append
> +       `:track,nobracket` to show tracking information without
> +       brackets (i.e "ahead N, behind M").  Has no effect if the ref
> +       does not have tracking information associated with it.
>
>  push::
> -       The name of a local ref which represents the `@{push}` location
> -       for the displayed ref. Respects `:short`, `:track`, and
> -       `:trackshort` options as `upstream` does. Produces an empty
> -       string if no `@{push}` ref is configured.
> +       The name of a local ref which represents the `@{push}`
> +       location for the displayed ref. Respects `:short`, `:strip`,
> +       `:track`, and `:trackshort` options as `upstream`
> +       does. Produces an empty string if no `@{push}` ref is
> +       configured.
>
>  HEAD::
>         '*' if HEAD matches current ref (the checked out branch), ' '
> diff --git a/ref-filter.c b/ref-filter.c
> index f1d27b5..7d3d3a6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -52,7 +52,8 @@ static struct used_atom {
>                 char color[COLOR_MAXLEN];
>                 struct align align;
>                 struct {
> -                       enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
> +                       enum { RR_REF, RR_TRACK, RR_TRACKSHORT } option;
> +                       struct refname_atom refname;
>                         unsigned int nobracket: 1;
>                 } remote_ref;
>                 struct {
> @@ -102,7 +103,9 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>         int i;
>
>         if (!arg) {
> -               atom->u.remote_ref.option = RR_NORMAL;
> +               atom->u.remote_ref.option = RR_REF;
> +               refname_atom_parser_internal(&atom->u.remote_ref.refname,
> +                                            arg, atom->name);
>                 return;
>         }
>
> @@ -112,16 +115,17 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>         for (i = 0; i < params.nr; i++) {
>                 const char *s = params.items[i].string;
>
> -               if (!strcmp(s, "short"))
> -                       atom->u.remote_ref.option = RR_SHORTEN;
> -               else if (!strcmp(s, "track"))
> +               if (!strcmp(s, "track"))
>                         atom->u.remote_ref.option = RR_TRACK;
>                 else if (!strcmp(s, "trackshort"))
>                         atom->u.remote_ref.option = RR_TRACKSHORT;
>                 else if (!strcmp(s, "nobracket"))
>                         atom->u.remote_ref.nobracket = 1;
> -               else
> -                       die(_("unrecognized format: %%(%s)"), atom->name);
> +               else {
> +                       atom->u.remote_ref.option = RR_REF;
> +                       refname_atom_parser_internal(&atom->u.remote_ref.refname,
> +                                                    arg, atom->name);
> +               }
>         }
>
>         string_list_clear(&params, 0);
> @@ -1100,8 +1104,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>                                     struct branch *branch, const char **s)
>  {
>         int num_ours, num_theirs;
> -       if (atom->u.remote_ref.option == RR_SHORTEN)
> -               *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> +       if (atom->u.remote_ref.option == RR_REF)
> +               *s = show_ref(&atom->u.remote_ref.refname, refname);
>         else if (atom->u.remote_ref.option == RR_TRACK) {
>                 if (stat_tracking_info(branch, &num_ours,
>                                        &num_theirs, NULL)) {
> @@ -1133,8 +1137,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>                         *s = ">";
>                 else
>                         *s = "<>";
> -       } else /* RR_NORMAL */
> -               *s = refname;
> +       } else
> +               die("BUG: unhandled RR_* enum");
>  }
>
>  char *get_head_description(void)
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 3d28234..7ca0a12 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -55,8 +55,10 @@ test_atom head refname:strip=1 heads/master
>  test_atom head refname:strip=2 master
>  test_atom head upstream refs/remotes/origin/master
>  test_atom head upstream:short origin/master
> +test_atom head upstream:strip=2 origin/master
>  test_atom head push refs/remotes/myfork/master
>  test_atom head push:short myfork/master
> +test_atom head push:strip=1 remotes/myfork/master
>  test_atom head objecttype commit
>  test_atom head objectsize 171
>  test_atom head objectname $(git rev-parse refs/heads/master)
> --
> 2.10.2
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
> the atom %(upstream:track) can be translated if needed. This is needed
> as we port branch.c to use ref-filter's printing API's.
>

> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> 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>
> ---
>  ref-filter.c | 28 ++++++++++++++++++++++++----
>  ref-filter.h |  2 ++
>  2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index b47b900..944671a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -15,6 +15,26 @@
>  #include "version.h"
>  #include "wt-status.h"
>
> +static struct ref_msg {
> +       const char *gone;
> +       const char *ahead;
> +       const char *behind;
> +       const char *ahead_behind;
> +} msgs = {
> +       "gone",
> +       "ahead %d",
> +       "behind %d",
> +       "ahead %d, behind %d"
> +};
> +
> +void setup_ref_filter_porcelain_msg(void)
> +{
> +       msgs.gone = _("gone");
> +       msgs.ahead = _("ahead %d");
> +       msgs.behind = _("behind %d");
> +       msgs.ahead_behind = _("ahead %d, behind %d");
> +}
> +
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
>  struct align {
> @@ -1130,15 +1150,15 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>         else if (atom->u.remote_ref.option == RR_TRACK) {
>                 if (stat_tracking_info(branch, &num_ours,
>                                        &num_theirs, NULL)) {
> -                       *s = xstrdup("gone");
> +                       *s = xstrdup(msgs.gone);
>                 } else if (!num_ours && !num_theirs)
>                         *s = "";
>                 else if (!num_ours)
> -                       *s = xstrfmt("behind %d", num_theirs);
> +                       *s = xstrfmt(msgs.behind, num_theirs);
>                 else if (!num_theirs)
> -                       *s = xstrfmt("ahead %d", num_ours);
> +                       *s = xstrfmt(msgs.ahead, num_ours);
>                 else
> -                       *s = xstrfmt("ahead %d, behind %d",
> +                       *s = xstrfmt(msgs.ahead_behind,
>                                      num_ours, num_theirs);
>                 if (!atom->u.remote_ref.nobracket && *s[0]) {
>                         const char *to_free = *s;
> diff --git a/ref-filter.h b/ref-filter.h
> index 0014b92..da17145 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -111,5 +111,7 @@ struct ref_sorting *ref_default_sorting(void);
>  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
>  /*  Get the current HEAD's description */
>  char *get_head_description(void);
> +/*  Set up translated strings in the output. */
> +void setup_ref_filter_porcelain_msg(void);
>
>  #endif /*  REF_FILTER_H  */
> --
> 2.10.2
>

> From: Karthik Nayak <karthik.188@gmail.com>
> 
> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
> the atom %(upstream:track) can be translated if needed. This is needed
> as we port branch.c to use ref-filter's printing API's.
> 
> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> 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>
> ---
>  ref-filter.c | 28 ++++++++++++++++++++++++----
>  ref-filter.h |  2 ++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/ref-filter.c b/ref-filter.c
> index b47b900..944671a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -15,6 +15,26 @@
>  #include "version.h"
>  #include "wt-status.h"
>  
> +static struct ref_msg {
> +	const char *gone;
> +	const char *ahead;
> +	const char *behind;
> +	const char *ahead_behind;
> +} msgs = {
> +	"gone",
> +	"ahead %d",
> +	"behind %d",
> +	"ahead %d, behind %d"
> +};
> +
> +void setup_ref_filter_porcelain_msg(void)
> +{
> +	msgs.gone = _("gone");
> +	msgs.ahead = _("ahead %d");
> +	msgs.behind = _("behind %d");
> +	msgs.ahead_behind = _("ahead %d, behind %d");
> +}

> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
>> the atom %(upstream:track) can be translated if needed. This is needed
>> as we port branch.c to use ref-filter's printing API's.
>>
>> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> 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>
>> ---
>>  ref-filter.c | 28 ++++++++++++++++++++++++----
>>  ref-filter.h |  2 ++
>>  2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index b47b900..944671a 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -15,6 +15,26 @@
>>  #include "version.h"
>>  #include "wt-status.h"
>>
>> +static struct ref_msg {
>> +     const char *gone;
>> +     const char *ahead;
>> +     const char *behind;
>> +     const char *ahead_behind;
>> +} msgs = {
>> +     "gone",
>> +     "ahead %d",
>> +     "behind %d",
>> +     "ahead %d, behind %d"
>> +};
>> +
>> +void setup_ref_filter_porcelain_msg(void)
>> +{
>> +     msgs.gone = _("gone");
>> +     msgs.ahead = _("ahead %d");
>> +     msgs.behind = _("behind %d");
>> +     msgs.ahead_behind = _("ahead %d, behind %d");
>> +}
>
> Do I understand it correctly that this mechanism is here to avoid
> repeated calls into gettext, as those messages would get repeated
> over and over; otherwise one would use foo = N_("...") and _(foo),
> isn't it?
>
> I wonder if there is some way to avoid duplication here, but I don't
> see anything easy and safe (e.g. against running setup_*() twice).
>

> cc'in Matthieu since he wrote the patch.
>
> On Sat, Nov 19, 2016 at 4:16 AM, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
>>> the atom %(upstream:track) can be translated if needed. This is needed
>>> as we port branch.c to use ref-filter's printing API's.
>>>
>>> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>>> 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>
>>> ---
>>>  ref-filter.c | 28 ++++++++++++++++++++++++----
>>>  ref-filter.h |  2 ++
>>>  2 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> index b47b900..944671a 100644
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -15,6 +15,26 @@
>>>  #include "version.h"
>>>  #include "wt-status.h"
>>>
>>> +static struct ref_msg {
>>> +     const char *gone;
>>> +     const char *ahead;
>>> +     const char *behind;
>>> +     const char *ahead_behind;
>>> +} msgs = {
>>> +     "gone",
>>> +     "ahead %d",
>>> +     "behind %d",
>>> +     "ahead %d, behind %d"
>>> +};
>>> +
>>> +void setup_ref_filter_porcelain_msg(void)
>>> +{
>>> +     msgs.gone = _("gone");
>>> +     msgs.ahead = _("ahead %d");
>>> +     msgs.behind = _("behind %d");
>>> +     msgs.ahead_behind = _("ahead %d, behind %d");
>>> +}
>>
>> Do I understand it correctly that this mechanism is here to avoid
>> repeated calls into gettext, as those messages would get repeated
>> over and over; otherwise one would use foo = N_("...") and _(foo),
>> isn't it?

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> cc'in Matthieu since he wrote the patch.
>>
>> On Sat, Nov 19, 2016 at 4:16 AM, Jakub Narębski <jnareb@gmail.com> wrote:
>>> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>>
>>>> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
>>>> the atom %(upstream:track) can be translated if needed. This is needed
>>>> as we port branch.c to use ref-filter's printing API's.
>>>>
>>>> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>>>> 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>
>>>> ---
>>>>  ref-filter.c | 28 ++++++++++++++++++++++++----
>>>>  ref-filter.h |  2 ++
>>>>  2 files changed, 26 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/ref-filter.c b/ref-filter.c
>>>> index b47b900..944671a 100644
>>>> --- a/ref-filter.c
>>>> +++ b/ref-filter.c
>>>> @@ -15,6 +15,26 @@
>>>>  #include "version.h"
>>>>  #include "wt-status.h"
>>>>
>>>> +static struct ref_msg {
>>>> +     const char *gone;
>>>> +     const char *ahead;
>>>> +     const char *behind;
>>>> +     const char *ahead_behind;
>>>> +} msgs = {
>>>> +     "gone",
>>>> +     "ahead %d",
>>>> +     "behind %d",
>>>> +     "ahead %d, behind %d"
>>>> +};
>>>> +
>>>> +void setup_ref_filter_porcelain_msg(void)
>>>> +{
>>>> +     msgs.gone = _("gone");
>>>> +     msgs.ahead = _("ahead %d");
>>>> +     msgs.behind = _("behind %d");
>>>> +     msgs.ahead_behind = _("ahead %d, behind %d");
>>>> +}
>>>
>>> Do I understand it correctly that this mechanism is here to avoid
>>> repeated calls into gettext, as those messages would get repeated
>>> over and over; otherwise one would use foo = N_("...") and _(foo),
>>> isn't it?
>
> That's not the primary goal. The primary goal is to keep untranslated,
> and immutable messages in plumbing commands. We may decide one day that
> _("gone") is not the best message for the end user and replace it with,
> say, _("vanished"), but the "gone" has to remain the same forever and
> regardless of the user's config for scripts using it.
>
> We could have written
>
>   in_porcelain ? _("gone") : "gone"
>
> here and there in the code, but having a single place where we set all
> the messages seems simpler. Call setup_ref_filter_porcelain_msg() and
> get the porcelain messages, don't call it and keep the plumbing
> messages.
>
> Note that it's not the first place in the code where we do this, see
> setup_unpack_trees_porcelain in unpack-trees.c for another instance.
>
> Karthik: the commit message could be improved, for example:
>
> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
> the atom %(upstream:track) can be translated if needed. By default, keep
> the messages untranslated, which is the right behavior for plumbing
> commands. This is needed as we port branch.c to use ref-filter's
> printing API's.
>
> Why not a comment right below "} msgs = {" saying e.g.:
>
>         /* Untranslated plumbing messages: */
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add the options `:dir` and `:base` to all ref printing ('%(refname)',
> '%(symref)', '%(push)' and '%(upstream)') atoms. The `:dir` option gives
> the directory (the part after $GIT_DIR/) of the ref without the
> refname. The `:base` option gives the base directory of the given
> ref (i.e. the directory following $GIT_DIR/refs/).
>

> Add tests and documentation for the same.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt | 34 +++++++++++++++++++---------------
>  ref-filter.c                       | 29 +++++++++++++++++++++++++----
>  t/t6300-for-each-ref.sh            | 24 ++++++++++++++++++++++++
>  3 files changed, 68 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 600b703..f4ad297 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -96,7 +96,9 @@ refname::
>         slash-separated path components from the front of the refname
>         (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>         `<N>` must be a positive integer.  If a displayed ref has fewer
> -       components than `<N>`, the command aborts with an error.
> +       components than `<N>`, the command aborts with an error. For the base
> +       directory of the ref (i.e. foo in refs/foo/bar/boz) append
> +       `:base`. For the entire directory path append `:dir`.
>
>  objecttype::
>         The type of the object (`blob`, `tree`, `commit`, `tag`).
> @@ -114,22 +116,23 @@ objectname::
>
>  upstream::
>         The name of a local ref which can be considered ``upstream''
> -       from the displayed ref. Respects `:short` and `:strip` in the
> -       same way as `refname` above.  Additionally respects `:track`
> -       to show "[ahead N, behind M]" and `:trackshort` to show the
> -       terse version: ">" (ahead), "<" (behind), "<>" (ahead and
> -       behind), or "=" (in sync). `:track` also prints "[gone]"
> -       whenever unknown upstream ref is encountered. Append
> -       `:track,nobracket` to show tracking information without
> -       brackets (i.e "ahead N, behind M").  Has no effect if the ref
> -       does not have tracking information associated with it.
> +       from the displayed ref. Respects `:short`, `:strip`, `:base`
> +       and `:dir` in the same way as `refname` above.  Additionally
> +       respects `:track` to show "[ahead N, behind M]" and
> +       `:trackshort` to show the terse version: ">" (ahead), "<"
> +       (behind), "<>" (ahead and behind), or "=" (in sync). `:track`
> +       also prints "[gone]" whenever unknown upstream ref is
> +       encountered. Append `:track,nobracket` to show tracking
> +       information without brackets (i.e "ahead N, behind M").  Has
> +       no effect if the ref does not have tracking information
> +       associated with it.
>
>  push::
>         The name of a local ref which represents the `@{push}`
>         location for the displayed ref. Respects `:short`, `:strip`,
> -       `:track`, and `:trackshort` options as `upstream`
> -       does. Produces an empty string if no `@{push}` ref is
> -       configured.
> +       `:track`, `:trackshort`, `:base` and `:dir` options as
> +       `upstream` does. Produces an empty string if no `@{push}` ref
> +       is configured.
>

>  HEAD::
>         '*' if HEAD matches current ref (the checked out branch), ' '
> @@ -169,8 +172,9 @@ if::
>
>  symref::
>         The ref which the given symbolic ref refers to. If not a
> -       symbolic ref, nothing is printed. Respects the `:short` and
> -       `:strip` options in the same way as `refname` above.
> +       symbolic ref, nothing is printed. Respects the `:short`,
> +       `:strip`, `:base` and `:dir` options in the same way as
> +       `refname` above.
>
>  In addition to the above, for commit and tag objects, the header
>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> diff --git a/ref-filter.c b/ref-filter.c
> index 7d3d3a6..b47b900 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -31,7 +31,7 @@ struct if_then_else {
>  };
>
>  struct refname_atom {
> -       enum { R_NORMAL, R_SHORT, R_STRIP } option;
> +       enum { R_BASE, R_DIR, R_NORMAL, R_SHORT, R_STRIP } option;
>         unsigned int strip;
>  };
>
> @@ -93,7 +93,11 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
>                 atom->option = R_STRIP;
>                 if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
>                         die(_("positive value expected refname:strip=%s"), arg);
> -       }       else
> +       } else if (!strcmp(arg, "dir"))
> +               atom->option = R_DIR;
> +       else if (!strcmp(arg, "base"))
> +               atom->option = R_BASE;
> +       else
>                 die(_("unrecognized %%(%s) argument: %s"), name, arg);
>  }
>
> @@ -252,7 +256,6 @@ static void if_atom_parser(struct used_atom *atom, const char *arg)
>                 die(_("unrecognized %%(if) argument: %s"), arg);
>  }
>
> -
>  static struct {
>         const char *name;
>         cmp_type cmp_type;
> @@ -1096,7 +1099,25 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
>                 return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
>         else if (atom->option == R_STRIP)
>                 return strip_ref_components(refname, atom->strip);
> -       else
> +       else if (atom->option == R_BASE) {
> +               const char *sp, *ep;
> +
> +               if (skip_prefix(refname, "refs/", &sp)) {
> +                       ep = strchr(sp, '/');
> +                       if (!ep)
> +                               return "";
> +                       return xstrndup(sp, ep - sp);
> +               }
> +               return "";
> +       } else if (atom->option == R_DIR) {
> +               const char *sp, *ep;
> +
> +               sp = refname;
> +               ep = strrchr(sp, '/');
> +               if (!ep)
> +                       return "";
> +               return xstrndup(sp, ep - sp);
> +       } else
>                 return refname;
>  }
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7ca0a12..8ff6568 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -53,12 +53,18 @@ test_atom head refname refs/heads/master
>  test_atom head refname:short master
>  test_atom head refname:strip=1 heads/master
>  test_atom head refname:strip=2 master
> +test_atom head refname:dir refs/heads
> +test_atom head refname:base heads
>  test_atom head upstream refs/remotes/origin/master
>  test_atom head upstream:short origin/master
>  test_atom head upstream:strip=2 origin/master
> +test_atom head upstream:dir refs/remotes/origin
> +test_atom head upstream:base remotes
>  test_atom head push refs/remotes/myfork/master
>  test_atom head push:short myfork/master
>  test_atom head push:strip=1 remotes/myfork/master
> +test_atom head push:dir refs/remotes/myfork
> +test_atom head push:base remotes
>  test_atom head objecttype commit
>  test_atom head objectsize 171
>  test_atom head objectname $(git rev-parse refs/heads/master)
> @@ -600,4 +606,22 @@ test_expect_success 'Verify usage of %(symref:strip) atom' '
>         test_cmp expected actual
>  '
>
> +cat >expected <<EOF
> +refs/heads
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:dir) atom' '
> +       git for-each-ref --format="%(symref:dir)" refs/heads/sym > actual &&
> +       test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +heads
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:base) atom' '
> +       git for-each-ref --format="%(symref:base)" refs/heads/sym > actual &&
> +       test_cmp expected actual
> +'
> +
>  test_done
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>> Add tests and documentation for the same.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>>  Documentation/git-for-each-ref.txt | 34 +++++++++++++++++++---------------
>>  ref-filter.c                       | 29 +++++++++++++++++++++++++----
>>  t/t6300-for-each-ref.sh            | 24 ++++++++++++++++++++++++
>>  3 files changed, 68 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 600b703..f4ad297 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -96,7 +96,9 @@ refname::
>>         slash-separated path components from the front of the refname
>>         (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>>         `<N>` must be a positive integer.  If a displayed ref has fewer
>> -       components than `<N>`, the command aborts with an error.
>> +       components than `<N>`, the command aborts with an error. For the base
>> +       directory of the ref (i.e. foo in refs/foo/bar/boz) append
>> +       `:base`. For the entire directory path append `:dir`.
>>
>>  objecttype::
>>         The type of the object (`blob`, `tree`, `commit`, `tag`).
>> @@ -114,22 +116,23 @@ objectname::
>>
>>  upstream::
>>         The name of a local ref which can be considered ``upstream''
>> -       from the displayed ref. Respects `:short` and `:strip` in the
>> -       same way as `refname` above.  Additionally respects `:track`
>> -       to show "[ahead N, behind M]" and `:trackshort` to show the
>> -       terse version: ">" (ahead), "<" (behind), "<>" (ahead and
>> -       behind), or "=" (in sync). `:track` also prints "[gone]"
>> -       whenever unknown upstream ref is encountered. Append
>> -       `:track,nobracket` to show tracking information without
>> -       brackets (i.e "ahead N, behind M").  Has no effect if the ref
>> -       does not have tracking information associated with it.
>> +       from the displayed ref. Respects `:short`, `:strip`, `:base`
>> +       and `:dir` in the same way as `refname` above.  Additionally
>> +       respects `:track` to show "[ahead N, behind M]" and
>> +       `:trackshort` to show the terse version: ">" (ahead), "<"
>> +       (behind), "<>" (ahead and behind), or "=" (in sync). `:track`
>> +       also prints "[gone]" whenever unknown upstream ref is
>> +       encountered. Append `:track,nobracket` to show tracking
>> +       information without brackets (i.e "ahead N, behind M").  Has
>> +       no effect if the ref does not have tracking information
>> +       associated with it.
>>
>>  push::
>>         The name of a local ref which represents the `@{push}`
>>         location for the displayed ref. Respects `:short`, `:strip`,
>> -       `:track`, and `:trackshort` options as `upstream`
>> -       does. Produces an empty string if no `@{push}` ref is
>> -       configured.
>> +       `:track`, `:trackshort`, `:base` and `:dir` options as
>> +       `upstream` does. Produces an empty string if no `@{push}` ref
>> +       is configured.
>>
>
> At this point would it make more sense to document the extra values
> here in one block separately? For example, the upstream atom is
> getting pretty complex with all those options. Additionally, some of
> the options can be combined, like nobracket, but others cannot be
> comined so It may be worth documenting how and when those combinations
> work?
>

>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>> index 600b703..f4ad297 100644
>>> --- a/Documentation/git-for-each-ref.txt
>>> +++ b/Documentation/git-for-each-ref.txt
>>> @@ -96,7 +96,9 @@ refname::
>>>         slash-separated path components from the front of the refname
>>>         (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>>>         `<N>` must be a positive integer.  If a displayed ref has fewer
>>> -       components than `<N>`, the command aborts with an error.
>>> +       components than `<N>`, the command aborts with an error. For the base
>>> +       directory of the ref (i.e. foo in refs/foo/bar/boz) append
>>> +       `:base`. For the entire directory path append `:dir`.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>>> index 600b703..f4ad297 100644
>>>> --- a/Documentation/git-for-each-ref.txt
>>>> +++ b/Documentation/git-for-each-ref.txt
>>>> @@ -96,7 +96,9 @@ refname::
>>>>         slash-separated path components from the front of the refname
>>>>         (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>>>>         `<N>` must be a positive integer.  If a displayed ref has fewer
>>>> -       components than `<N>`, the command aborts with an error.
>>>> +       components than `<N>`, the command aborts with an error. For the base
>>>> +       directory of the ref (i.e. foo in refs/foo/bar/boz) append
>>>> +       `:base`. For the entire directory path append `:dir`.
>
> Sorry that I missed this so far and I do not know how many recent
> rerolls had them like this, but I am not sure about these :base and
> :dir suffixes.  From their names I think readers would expect that
> they do rough equivalents to basename() and dirname() applied to the
> refname, but the example contradicts with that intuition.  The
> result of applying basename() to 'refs/boo/bar/boz' would be 'boz'
> and not 'foo' as the example says.
>

> So assuming that :base and :dir are unrelated to basename() and
> dirname():
>
>  - I think calling these :base and :dir may be misleading
>
>  - More importantly, what do these do?  I do not think of a good
>    description that generalizes "base of refs/foo/bar/boz is foo" to
>    explain your :base.

>>  - More importantly, what do these do?  I do not think of a good
>>    description that generalizes "base of refs/foo/bar/boz is foo" to
>>    explain your :base.
>
> $ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
> refs/heads/master                  refs/heads
> refs/heads/ref-filter                refs/heads
> refs/remotes/junio/va/i18n     refs/remotes/junio/va
>
> $ ./git for-each-ref  refs/heads --format
> "%(align:left,30)%(refname)%(end) %(refname:base)"
> refs/heads/master                 heads
> refs/heads/ref-filter                heads
> refs/remotes/junio/va/i18n     remotes
>
> I guess this should clear it up.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>  - More importantly, what do these do?  I do not think of a good
>>>    description that generalizes "base of refs/foo/bar/boz is foo" to
>>>    explain your :base.
>>
>> $ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
>> refs/heads/master                  refs/heads
>> refs/heads/ref-filter                refs/heads
>> refs/remotes/junio/va/i18n     refs/remotes/junio/va
>>
>> $ ./git for-each-ref  refs/heads --format
>> "%(align:left,30)%(refname)%(end) %(refname:base)"
>> refs/heads/master                 heads
>> refs/heads/ref-filter                heads
>> refs/remotes/junio/va/i18n     remotes
>>
>> I guess this should clear it up.
>
> Hmph.
>
> I would guess from these examples that :dir is an equivalent to
> dirname().  But it is unclear how :base is defined.  Is it the path
> component that comes immediately after "refs/" that appears at the
> beginning?

> On Tue, Nov 15, 2016 at 1:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>>>  - More importantly, what do these do?  I do not think of a good
>>>>    description that generalizes "base of refs/foo/bar/boz is foo" to
>>>>    explain your :base.
>>>
>>> $ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
>>> refs/heads/master                  refs/heads
>>> refs/heads/ref-filter                refs/heads
>>> refs/remotes/junio/va/i18n     refs/remotes/junio/va
>>>
>>> $ ./git for-each-ref  refs/heads --format
>>> "%(align:left,30)%(refname)%(end) %(refname:base)"
>>> refs/heads/master                 heads
>>> refs/heads/ref-filter                heads
>>> refs/remotes/junio/va/i18n     remotes
>>>
>>> I guess this should clear it up.
>>
>> Hmph.
>>
>> I would guess from these examples that :dir is an equivalent to
>> dirname().  But it is unclear how :base is defined.  Is it the path
>> component that comes immediately after "refs/" that appears at the
>> beginning?
>
> ':dir' is equivalent to dirname(). Yup, base is the folder right after 'refs/'.
> For local branches it is 'heads' for remotes it is 'remotes'. This is useful
> when we want to make decisions based on the type of branch we're dealing
> with (using along with %(if)...%(end)).
>
> --
> Regards,
> Karthik Nayak

> On Mon, Nov 14, 2016 at 10:48 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Tue, Nov 15, 2016 at 1:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>>>  - More importantly, what do these do?  I do not think of a good
>>>>>    description that generalizes "base of refs/foo/bar/boz is foo" to
>>>>>    explain your :base.
>>>>
>>>> $ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
>>>> refs/heads/master                  refs/heads
>>>> refs/heads/ref-filter                refs/heads
>>>> refs/remotes/junio/va/i18n     refs/remotes/junio/va
>>>>
>>>> $ ./git for-each-ref  refs/heads --format
>>>> "%(align:left,30)%(refname)%(end) %(refname:base)"
>>>> refs/heads/master                 heads
>>>> refs/heads/ref-filter                heads
>>>> refs/remotes/junio/va/i18n     remotes
>>>>
>>>> I guess this should clear it up.
>>>
>>> Hmph.
>>>
>>> I would guess from these examples that :dir is an equivalent to
>>> dirname().  But it is unclear how :base is defined.  Is it the path
>>> component that comes immediately after "refs/" that appears at the
>>> beginning?
>>
>> ':dir' is equivalent to dirname(). Yup, base is the folder right after 'refs/'.
>> For local branches it is 'heads' for remotes it is 'remotes'. This is useful
>> when we want to make decisions based on the type of branch we're dealing
>> with (using along with %(if)...%(end)).
>>
>> --
>> Regards,
>> Karthik Nayak
>
>
> dirname makes sense. What about implementing a reverse variant of
> strip, which you could perform stripping of right-most components and
> instead of stripping by a number, strip "to" a number, ie: keep the
> left N most components, and then you could use something like
>
> "keep=2" to keep "refs/heads"
>
> ?
>
> I think that would be more general purpose than basename, and less confusing?
>
> Thanks,
> Jake

> dirname makes sense. What about implementing a reverse variant of
> strip, which you could perform stripping of right-most components and
> instead of stripping by a number, strip "to" a number, ie: keep the
> left N most components, and then you could use something like
> ...
> I think that would be more general purpose than basename, and less confusing?

>I think you are going in the right direction.  I had a similar
>thought but built around a different axis.  I.e. if strip=1 strips
>one from the left, perhaps we want to have rstrip=1 that strips one
>from the right, and also strip=-1 to mean strip everything except
>one from the left and so on?.  I think this and your keep (and
>perhaps you'll have rkeep for completeness) have the same expressive
>power.  I do not offhand have a preference one over the other.

>
>Somehow it sounds a bit strange to me to treat 'remotes' as the same
>class of token as 'heads' and 'tags' (I'd expect 'heads' and
>'remotes/origin' would be at the same level in end-user's mind), but
>that is probably an unrelated tangent.  The reason this series wants
>to introduce :base must be to emulate an existing feature, so that
>existing feature is a concrete counter-example that argues against
>my "it sounds a bit strange" reaction.

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> dirname makes sense. What about implementing a reverse variant of
>> strip, which you could perform stripping of right-most components and
>> instead of stripping by a number, strip "to" a number, ie: keep the
>> left N most components, and then you could use something like
>> ...
>> I think that would be more general purpose than basename, and less confusing?
>
> I think you are going in the right direction.  I had a similar
> thought but built around a different axis.  I.e. if strip=1 strips
> one from the left, perhaps we want to have rstrip=1 that strips one
> from the right, and also strip=-1 to mean strip everything except
> one from the left and so on?.  I think this and your keep (and
> perhaps you'll have rkeep for completeness) have the same expressive
> power.  I do not offhand have a preference one over the other.
>

> On November 15, 2016 9:42:03 AM PST, Junio C Hamano <gitster@pobox.com> wrote:
>>Somehow it sounds a bit strange to me to treat 'remotes' as the same
>>class of token as 'heads' and 'tags' (I'd expect 'heads' and
>>'remotes/origin' would be at the same level in end-user's mind), but
>>that is probably an unrelated tangent.  The reason this series wants
>>to introduce :base must be to emulate an existing feature, so that
>>existing feature is a concrete counter-example that argues against
>>my "it sounds a bit strange" reaction.
>
> It may be a bit strange indeed. What is the requirement for this?
>
> I think implementing a strip and rstrip ( if necessary ) with negative numbers would be most ideal.

> On Tue, Nov 15, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>> ...
>> I think you are going in the right direction.  I had a similar
>> thought but built around a different axis.  I.e. if strip=1 strips
>> one from the left, perhaps we want to have rstrip=1 that strips one
>> from the right, and also strip=-1 to mean strip everything except
>> one from the left and so on?
> ...

> If we do implement strip with negative numbers, it definitely
> would be neat, but to get the desired feature which I've mentioned
> below, we'd need to call strip twice, i.e
> to get remotes from /refs/foo/abc/xyz we'd need to do
> strip=1,strip=-1, which could be
> done but ...

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Nov 15, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jacob Keller <jacob.keller@gmail.com> writes:
>>> ...
>>> I think you are going in the right direction.  I had a similar
>>> thought but built around a different axis.  I.e. if strip=1 strips
>>> one from the left, perhaps we want to have rstrip=1 that strips one
>>> from the right, and also strip=-1 to mean strip everything except
>>> one from the left and so on?
>> ...
>
>> If we do implement strip with negative numbers, it definitely
>> would be neat, but to get the desired feature which I've mentioned
>> below, we'd need to call strip twice, i.e
>> to get remotes from /refs/foo/abc/xyz we'd need to do
>> strip=1,strip=-1, which could be
>> done but ...
>
> ... would be unnecessary if this is the only use case:
>
>> strbuf_addf(&fmt,
>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>> local.buf, remote.buf);
>
> You can "strip to leave only 2 components" and compare the result
> with refs/remotes instead, no?
>

> Hey,
>
> On Fri, Nov 18, 2016 at 12:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> On Tue, Nov 15, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Jacob Keller <jacob.keller@gmail.com> writes:
>>>> ...
>>>> I think you are going in the right direction.  I had a similar
>>>> thought but built around a different axis.  I.e. if strip=1 strips
>>>> one from the left, perhaps we want to have rstrip=1 that strips one
>>>> from the right, and also strip=-1 to mean strip everything except
>>>> one from the left and so on?
>>> ...
>>
>>> If we do implement strip with negative numbers, it definitely
>>> would be neat, but to get the desired feature which I've mentioned
>>> below, we'd need to call strip twice, i.e
>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>> strip=1,strip=-1, which could be
>>> done but ...
>>
>> ... would be unnecessary if this is the only use case:
>>
>>> strbuf_addf(&fmt,
>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>> local.buf, remote.buf);
>>
>> You can "strip to leave only 2 components" and compare the result
>> with refs/remotes instead, no?
>>
>
> Of course, my only objective was that someone would find it useful to
> have these two additional
> atoms. So if you think it's unnecessary we could drop it entirely :D
>
> --
> Regards,
> Karthik Nayak

>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>> strip=1,strip=-1, which could be
>>>> done but ...
>>>
>>> ... would be unnecessary if this is the only use case:
>>>
>>>> strbuf_addf(&fmt,
>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>> local.buf, remote.buf);
>>>
>>> You can "strip to leave only 2 components" and compare the result
>>> with refs/remotes instead, no?
>>>
>>
>> Of course, my only objective was that someone would find it useful to
>> have these two additional
>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>
>> --
>> Regards,
>> Karthik Nayak
>
> I think having strip and rstrip make sense, (along with support for
> negative numbers) I don't think we need to make them work together
> unless someone is interested, since we can use strip=-2 to get the
> behavior we need today.

> Jacob Keller <jacob.keller@gmail.com> writes:
> 
>> dirname makes sense. What about implementing a reverse variant of
>> strip, which you could perform stripping of right-most components and
>> instead of stripping by a number, strip "to" a number, ie: keep the
>> left N most components, and then you could use something like
>> ...
>> I think that would be more general purpose than basename, and less confusing?
> 
> I think you are going in the right direction.  I had a similar
> thought but built around a different axis.  I.e. if strip=1 strips
> one from the left, perhaps we want to have rstrip=1 that strips one
> from the right, and also strip=-1 to mean strip everything except
> one from the left and so on?.  I think this and your keep (and
> perhaps you'll have rkeep for completeness) have the same expressive
> power.  I do not offhand have a preference one over the other.
> 
> Somehow it sounds a bit strange to me to treat 'remotes' as the same
> class of token as 'heads' and 'tags' (I'd expect 'heads' and
> 'remotes/origin' would be at the same level in end-user's mind), but
> that is probably an unrelated tangent.  The reason this series wants
> to introduce :base must be to emulate an existing feature, so that
> existing feature is a concrete counter-example that argues against
> my "it sounds a bit strange" reaction.

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>>> strip=1,strip=-1, which could be
>>>>> done but ...
>>>>
>>>> ... would be unnecessary if this is the only use case:
>>>>
>>>>> strbuf_addf(&fmt,
>>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>>> local.buf, remote.buf);
>>>>
>>>> You can "strip to leave only 2 components" and compare the result
>>>> with refs/remotes instead, no?
>>>>
>>>
>>> Of course, my only objective was that someone would find it useful to
>>> have these two additional
>>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>>
>>> --
>>> Regards,
>>> Karthik Nayak
>>
>> I think having strip and rstrip make sense, (along with support for
>> negative numbers) I don't think we need to make them work together
>> unless someone is interested, since we can use strip=-2 to get the
>> behavior we need today.
>
> I am OK with multiple strips Karthik suggests, e.g.
>
>     %(refname:strip=1,rstrip=-1)
>
> if it is cleanly implemented.
>
> I have a bit of trouble with these names, though.  If we call one
> strip and the other rstrip, to only those who know about rstrip it
> would be clear that strip is about stripping from the left.  Perhaps
> we should call it lstrip for symmetry and ease-of-remembering?
>
>     refs/heads/master:lstrip=-1 => master (strip all but one level
>     from the left)
>
>     refs/heads/master:rstrip=-2 => refs/heads (strip all but two
>     levels from the right)
>
>     refs/heads/master:lstrip=1,rstrip=-1 => heads (strip one level
>     from the left and then strip all but one level from the right)
>
> I dunno.

> W dniu 15.11.2016 o 18:42, Junio C Hamano pisze:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>> dirname makes sense. What about implementing a reverse variant of
>>> strip, which you could perform stripping of right-most components and
>>> instead of stripping by a number, strip "to" a number, ie: keep the
>>> left N most components, and then you could use something like
>>> ...
>>> I think that would be more general purpose than basename, and less confusing?
>>
>> I think you are going in the right direction.  I had a similar
>> thought but built around a different axis.  I.e. if strip=1 strips
>> one from the left, perhaps we want to have rstrip=1 that strips one
>> from the right, and also strip=-1 to mean strip everything except
>> one from the left and so on?.  I think this and your keep (and
>> perhaps you'll have rkeep for completeness) have the same expressive
>> power.  I do not offhand have a preference one over the other.
>>
>> Somehow it sounds a bit strange to me to treat 'remotes' as the same
>> class of token as 'heads' and 'tags' (I'd expect 'heads' and
>> 'remotes/origin' would be at the same level in end-user's mind), but
>> that is probably an unrelated tangent.  The reason this series wants
>> to introduce :base must be to emulate an existing feature, so that
>> existing feature is a concrete counter-example that argues against
>> my "it sounds a bit strange" reaction.
>
> If it is to implement the feature where we select if to display only
> local branches (refs/heads/**), only remote-tracking branches
> (refs/remotes/**/**), or only tags (refs/tags/**), then perhaps
> ':category' or ':type' would make sense?
>
> As in '%(refname:category)', e.g.
>
>   %(if:equals=heads)%(refname:category)%(then)...%(end)
>

> On Fri, Nov 18, 2016 at 11:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>>>> strip=1,strip=-1, which could be
>>>>>> done but ...
>>>>>
>>>>> ... would be unnecessary if this is the only use case:
>>>>>
>>>>>> strbuf_addf(&fmt,
>>>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>>>> local.buf, remote.buf);
>>>>>
>>>>> You can "strip to leave only 2 components" and compare the result
>>>>> with refs/remotes instead, no?
>>>>>
>>>>
>>>> Of course, my only objective was that someone would find it useful to
>>>> have these two additional
>>>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>>>
>>>> --
>>>> Regards,
>>>> Karthik Nayak
>>>
>>> I think having strip and rstrip make sense, (along with support for
>>> negative numbers) I don't think we need to make them work together
>>> unless someone is interested, since we can use strip=-2 to get the
>>> behavior we need today.
>>
>> I am OK with multiple strips Karthik suggests, e.g.
>>
>>     %(refname:strip=1,rstrip=-1)
>>
>> if it is cleanly implemented.
>>
>> I have a bit of trouble with these names, though.  If we call one
>> strip and the other rstrip, to only those who know about rstrip it
>> would be clear that strip is about stripping from the left.  Perhaps
>> we should call it lstrip for symmetry and ease-of-remembering?
>>
>>     refs/heads/master:lstrip=-1 => master (strip all but one level
>>     from the left)
>>
>>     refs/heads/master:rstrip=-2 => refs/heads (strip all but two
>>     levels from the right)
>>
>>     refs/heads/master:lstrip=1,rstrip=-1 => heads (strip one level
>>     from the left and then strip all but one level from the right)
>>
>> I dunno.
>
> We could have lstrip and rstrip as you suggested and perhaps make it work
> together too. But I see this going off the scope of this series. Maybe
> I'll follow up
> with another series introducing these features. Since we can currently
> make do with
> 'strip=2' I'll drop this patch from v8 of this series and pursue this
> idea after this.
>

> We could have lstrip and rstrip as you suggested and perhaps make it work
> together too. But I see this going off the scope of this series. Maybe
> I'll follow up
> with another series introducing these features. Since we can currently
> make do with
> 'strip=2' I'll drop this patch from v8 of this series and pursue this
> idea after this.

> Karthik Nayak <karthik.188@gmail.com> writes:
> 
>> We could have lstrip and rstrip as you suggested and perhaps make
>> it work together too. But I see this going off the scope of this
>> series. Maybe I'll follow up with another series introducing these
>> features. Since we can currently make do with 'strip=2' I'll drop
>> this patch from v8 of this series and pursue this idea after this.
> 
> My primary point was that if we know we want to add "rstrip" later
> and still decide not to add it right now, it is OK, but we will
> regret it if we named the one we are going to add right now "strip".
> That will mean that future users, when "rstrip" is introduced, will
> end up having to choose between "strip" and "rstrip" (as opposed to
> "lstrip" and "rstrip"), wondering why left-variant is more important
> and named without left/right prefix.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We could have lstrip and rstrip as you suggested and perhaps make it work
>> together too. But I see this going off the scope of this series. Maybe
>> I'll follow up
>> with another series introducing these features. Since we can currently
>> make do with
>> 'strip=2' I'll drop this patch from v8 of this series and pursue this
>> idea after this.
>
> My primary point was that if we know we want to add "rstrip" later
> and still decide not to add it right now, it is OK, but we will
> regret it if we named the one we are going to add right now "strip".
> That will mean that future users, when "rstrip" is introduced, will
> end up having to choose between "strip" and "rstrip" (as opposed to
> "lstrip" and "rstrip"), wondering why left-variant is more important
> and named without left/right prefix.
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Port branch.c to use ref-filter APIs for printing. This clears out
> most of the code used in branch.c for printing and replaces them with
> calls made to the ref-filter library.

>
> Before this patch, all cross-prefix symrefs weren't shortened. Since
> we're using ref-filter APIs, we shorten all symrefs by default. We also
> allow the user to change the format if needed with the introduction of
> the '--format' option in the next patch.
>

> 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>
> ---
>  builtin/branch.c         | 234 ++++++++++++++---------------------------------
>  t/t3203-branch-output.sh |   2 +-
>  t/t6040-tracking-info.sh |   2 +-
>  3 files changed, 70 insertions(+), 168 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index dead2b8..a19e05d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -36,12 +36,12 @@ static unsigned char head_sha1[20];
>
>  static int branch_use_color = -1;
>  static char branch_colors[][COLOR_MAXLEN] = {
> -       GIT_COLOR_RESET,
> -       GIT_COLOR_NORMAL,       /* PLAIN */
> -       GIT_COLOR_RED,          /* REMOTE */
> -       GIT_COLOR_NORMAL,       /* LOCAL */
> -       GIT_COLOR_GREEN,        /* CURRENT */
> -       GIT_COLOR_BLUE,         /* UPSTREAM */
> +       "%(color:reset)",
> +       "%(color:reset)",       /* PLAIN */
> +       "%(color:red)",         /* REMOTE */
> +       "%(color:reset)",       /* LOCAL */
> +       "%(color:green)",       /* CURRENT */
> +       "%(color:blue)",        /* UPSTREAM */
>  };
>  enum color_branch {
>         BRANCH_COLOR_RESET = 0,
> @@ -280,162 +280,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>         return(ret);
>  }
>
> -static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
> -               int show_upstream_ref)
> -{
> -       int ours, theirs;
> -       char *ref = NULL;
> -       struct branch *branch = branch_get(branch_name);
> -       const char *upstream;
> -       struct strbuf fancy = STRBUF_INIT;
> -       int upstream_is_gone = 0;
> -       int added_decoration = 1;
> -
> -       if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
> -               if (!upstream)
> -                       return;
> -               upstream_is_gone = 1;
> -       }
> -
> -       if (show_upstream_ref) {
> -               ref = shorten_unambiguous_ref(upstream, 0);
> -               if (want_color(branch_use_color))
> -                       strbuf_addf(&fancy, "%s%s%s",
> -                                       branch_get_color(BRANCH_COLOR_UPSTREAM),
> -                                       ref, branch_get_color(BRANCH_COLOR_RESET));
> -               else
> -                       strbuf_addstr(&fancy, ref);
> -       }
> -
> -       if (upstream_is_gone) {
> -               if (show_upstream_ref)
> -                       strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
> -               else
> -                       added_decoration = 0;
> -       } else if (!ours && !theirs) {
> -               if (show_upstream_ref)
> -                       strbuf_addf(stat, _("[%s]"), fancy.buf);
> -               else
> -                       added_decoration = 0;
> -       } else if (!ours) {
> -               if (show_upstream_ref)
> -                       strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
> -               else
> -                       strbuf_addf(stat, _("[behind %d]"), theirs);
> -
> -       } else if (!theirs) {
> -               if (show_upstream_ref)
> -                       strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
> -               else
> -                       strbuf_addf(stat, _("[ahead %d]"), ours);
> -       } else {
> -               if (show_upstream_ref)
> -                       strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
> -                                   fancy.buf, ours, theirs);
> -               else
> -                       strbuf_addf(stat, _("[ahead %d, behind %d]"),
> -                                   ours, theirs);
> -       }
> -       strbuf_release(&fancy);
> -       if (added_decoration)
> -               strbuf_addch(stat, ' ');
> -       free(ref);
> -}
> -
> -static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
> -                            struct ref_filter *filter, const char *refname)
> -{
> -       struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
> -       const char *sub = _(" **** invalid ref ****");
> -       struct commit *commit = item->commit;
> -
> -       if (!parse_commit(commit)) {
> -               pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);
> -               sub = subject.buf;
> -       }
> -
> -       if (item->kind == FILTER_REFS_BRANCHES)
> -               fill_tracking_info(&stat, refname, filter->verbose > 1);
> -
> -       strbuf_addf(out, " %s %s%s",
> -               find_unique_abbrev(item->commit->object.oid.hash, filter->abbrev),
> -               stat.buf, sub);
> -       strbuf_release(&stat);
> -       strbuf_release(&subject);
> -}
> -
> -static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
> -                                     struct ref_filter *filter, const char *remote_prefix)
> -{
> -       char c;
> -       int current = 0;
> -       int color;
> -       struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> -       const char *prefix_to_show = "";
> -       const char *prefix_to_skip = NULL;
> -       const char *desc = item->refname;
> -       char *to_free = NULL;
> -
> -       switch (item->kind) {
> -       case FILTER_REFS_BRANCHES:
> -               prefix_to_skip = "refs/heads/";
> -               skip_prefix(desc, prefix_to_skip, &desc);
> -               if (!filter->detached && !strcmp(desc, head))
> -                       current = 1;
> -               else
> -                       color = BRANCH_COLOR_LOCAL;
> -               break;
> -       case FILTER_REFS_REMOTES:
> -               prefix_to_skip = "refs/remotes/";
> -               skip_prefix(desc, prefix_to_skip, &desc);
> -               color = BRANCH_COLOR_REMOTE;
> -               prefix_to_show = remote_prefix;
> -               break;
> -       case FILTER_REFS_DETACHED_HEAD:
> -               desc = to_free = get_head_description();
> -               current = 1;
> -               break;
> -       default:
> -               color = BRANCH_COLOR_PLAIN;
> -               break;
> -       }
> -
> -       c = ' ';
> -       if (current) {
> -               c = '*';
> -               color = BRANCH_COLOR_CURRENT;
> -       }
> -
> -       strbuf_addf(&name, "%s%s", prefix_to_show, desc);
> -       if (filter->verbose) {
> -               int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
> -               strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
> -                           maxwidth + utf8_compensation, name.buf,
> -                           branch_get_color(BRANCH_COLOR_RESET));
> -       } else
> -               strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
> -                           name.buf, branch_get_color(BRANCH_COLOR_RESET));
> -
> -       if (item->symref) {
> -               const char *symref = item->symref;
> -               if (prefix_to_skip)
> -                       skip_prefix(symref, prefix_to_skip, &symref);
> -               strbuf_addf(&out, " -> %s", symref);
> -       }
> -       else if (filter->verbose)
> -               /* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
> -               add_verbose_info(&out, item, filter, desc);
> -       if (column_active(colopts)) {
> -               assert(!filter->verbose && "--column and --verbose are incompatible");
> -               string_list_append(&output, out.buf);
> -       } else {
> -               printf("%s\n", out.buf);
> -       }
> -       strbuf_release(&name);
> -       strbuf_release(&out);
> -       free(to_free);
> -}
> -
>  static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>  {
>         int i, max = 0;
> @@ -446,7 +290,12 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>
>                 skip_prefix(it->refname, "refs/heads/", &desc);
>                 skip_prefix(it->refname, "refs/remotes/", &desc);
> -               w = utf8_strwidth(desc);
> +               if (it->kind == FILTER_REFS_DETACHED_HEAD) {
> +                       char *head_desc = get_head_description();
> +                       w = utf8_strwidth(head_desc);
> +                       free(head_desc);
> +               } else
> +                       w = utf8_strwidth(desc);
>
>                 if (it->kind == FILTER_REFS_REMOTES)
>                         w += remote_bonus;
> @@ -456,12 +305,52 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>         return max;
>  }
>
> +static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
> +{
> +       struct strbuf fmt = STRBUF_INIT;
> +       struct strbuf local = STRBUF_INIT;
> +       struct strbuf remote = STRBUF_INIT;
> +

> +       if (filter->verbose) {
> +               strbuf_addf(&local, "%%(align:%d,left)%%(refname:strip=2)%%(end)", maxwidth);
> +               strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> +               strbuf_addf(&local, " %%(objectname:short=7) ");
> +

> +               else
> +                       strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
> +
> +               strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:strip=2)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> +                           "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> +                           branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
> +                           remote_prefix, branch_get_color(BRANCH_COLOR_RESET));

> +       } else {
> +               strbuf_addf(&local, "%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> +                           branch_get_color(BRANCH_COLOR_RESET));
> +               strbuf_addf(&remote, "%s%s%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> +                           branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> +       }
> +
> +       strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
> +

> +       strbuf_release(&local);
> +       strbuf_release(&remote);
> +       return strbuf_detach(&fmt, NULL);
> +}
> +
>  static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
>  {
>         int i;
>         struct ref_array array;
>         int maxwidth = 0;
>         const char *remote_prefix = "";
> +       struct strbuf out = STRBUF_INIT;
> +       char *format;
>
>         /*
>          * If we are listing more than just remote branches,
> @@ -473,12 +362,14 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>
>         memset(&array, 0, sizeof(array));
>
> -       verify_ref_format("%(refname)%(symref)");
>         filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
>
>         if (filter->verbose)
>                 maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
>
> +       format = build_format(filter, maxwidth, remote_prefix);
> +       verify_ref_format(format);
> +
>         /*
>          * If no sorting parameter is given then we default to sorting
>          * by 'refname'. This would give us an alphabetically sorted
> @@ -490,10 +381,21 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>                 sorting = ref_default_sorting();
>         ref_array_sort(sorting, &array);
>
> -       for (i = 0; i < array.nr; i++)
> -               format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
> +       for (i = 0; i < array.nr; i++) {
> +               format_ref_array_item(array.items[i], format, 0, &out);
> +               if (column_active(colopts)) {
> +                       assert(!filter->verbose && "--column and --verbose are incompatible");
> +                        /* format to a string_list to let print_columns() do its job */
> +                       string_list_append(&output, out.buf);
> +               } else {
> +                       fwrite(out.buf, 1, out.len, stdout);
> +                       putchar('\n');
> +               }
> +               strbuf_release(&out);
> +       }
>
>         ref_array_clear(&array);
> +       free(format);
>  }
>
>  static void reject_rebase_or_bisect_branch(const char *target)
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index c6a3ccb..980c732 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -189,7 +189,7 @@ test_expect_success 'local-branch symrefs shortened properly' '
>         git symbolic-ref refs/heads/ref-to-remote refs/remotes/origin/branch-one &&
>         cat >expect <<-\EOF &&
>           ref-to-branch -> branch-one
> -         ref-to-remote -> refs/remotes/origin/branch-one
> +         ref-to-remote -> origin/branch-one
>         EOF
>         git branch >actual.raw &&
>         grep ref-to <actual.raw >actual &&
> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 3d5c238..97a0765 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d
>  b2 [ahead 1, behind 1] d
>  b3 [behind 1] b
>  b4 [ahead 2] f
> -b5 g
> +b5 [gone] g
>  b6 c
>  EOF
>
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Port branch.c to use ref-filter APIs for printing. This clears out
>> most of the code used in branch.c for printing and replaces them with
>> calls made to the ref-filter library.
>
> Nice. This looks correct based on checking against the current
> branch.c implementation by hand. There was one minor change I
> suggested but I'm not really sure it buys is that much.
>

>> +               if (filter->verbose > 1)
>> +                       strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
>> +                                   "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
>> +                                   branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
>
> When we have extra verbose, we check whether we have an upstream, and
> if so, we print the short name of that upstream inside brackets. If we
> have tracking information, we print that without brackets, and then we
> end this section. Finally we print the subject.
>
> We could almost re-use the code for the subject bits, but I'm not sure
> it's worth it. Maybe drop the %contents:subject part and add it
> afterwards since we always want it? It would remove some duplication
> but overall not sure it's actually worth it.
>

> Hello
>
> On Wed, Nov 9, 2016 at 5:44 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Port branch.c to use ref-filter APIs for printing. This clears out
>>> most of the code used in branch.c for printing and replaces them with
>>> calls made to the ref-filter library.
>>
>> Nice. This looks correct based on checking against the current
>> branch.c implementation by hand. There was one minor change I
>> suggested but I'm not really sure it buys is that much.
>>
>
> Thanks for this review. More down.
>
>>> +               if (filter->verbose > 1)
>>> +                       strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
>>> +                                   "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
>>> +                                   branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
>>
>> When we have extra verbose, we check whether we have an upstream, and
>> if so, we print the short name of that upstream inside brackets. If we
>> have tracking information, we print that without brackets, and then we
>> end this section. Finally we print the subject.
>>
>> We could almost re-use the code for the subject bits, but I'm not sure
>> it's worth it. Maybe drop the %contents:subject part and add it
>> afterwards since we always want it? It would remove some duplication
>> but overall not sure it's actually worth it.
>>
>
> If you see that's the last part we add to the 'local' strbuf in the
> verbose case.
> If we want to remove the duplication we'll end up adding one more
> strbuf_addf(...).
> So I guess its better this way.
>

> +	struct strbuf fmt = STRBUF_INIT;
> +	struct strbuf local = STRBUF_INIT;
> +	struct strbuf remote = STRBUF_INIT;
> +
> +	strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));

> +	if (filter->verbose) {
> +		strbuf_addf(&local, "%%(align:%d,left)%%(refname:strip=2)%%(end)", maxwidth);
> +		strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> +		strbuf_addf(&local, " %%(objectname:short=7) ");
> +
> +		if (filter->verbose > 1)
> +			strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> +				    "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
> +				    branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
> +		else
> +			strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
> +
> +		strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:strip=2)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> +			    "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
> +			    remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> +	} else {
> +		strbuf_addf(&local, "%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_RESET));
> +		strbuf_addf(&remote, "%s%s%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> +	}

> One worry that I have is if the strings embedded in this function to
> the final format are safe.  As far as I can tell, the pieces of
> strings that are literally inserted into the resulting format string
> by this function are maxwidth, remote_prefix, and return values from
> branch_get_color() calls.
>
> The maxwidth is inserted via "%d" and made into decimal constant,
> and there is no risk for it being in the resulting format.  Are
> the return values of branch_get_color() calls safe?  I do not think
> they can have '%' in them, but if they do, they need to be quoted.
> The same worry exists for remote_prefix.  Currently it can either be
> an empty string or "remotes/", and is safe to be embedded in a
> format string.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> One worry that I have is if the strings embedded in this function to
>> the final format are safe.  As far as I can tell, the pieces of
>> strings that are literally inserted into the resulting format string
>> by this function are maxwidth, remote_prefix, and return values from
>> branch_get_color() calls.
>>
>> The maxwidth is inserted via "%d" and made into decimal constant,
>> and there is no risk for it being in the resulting format.  Are
>> the return values of branch_get_color() calls safe?  I do not think
>> they can have '%' in them, but if they do, they need to be quoted.
>> The same worry exists for remote_prefix.  Currently it can either be
>> an empty string or "remotes/", and is safe to be embedded in a
>> format string.
>
> In case it was not clear, in short, I do not think there is anything
> broken in the code, but it is a longer-term improvement to introduce
> a helper that takes a string and returns a version of the string
> that is safely quoted to be used in the for-each-ref format string
> use it like so:
>
>     strbuf_addf(&remote,
>                 "%s"
>                 "%%(align:%d,left)%s%%(refname:strip=2)%%(end)"
>                 ...
>                 "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
>                 quote_literal_for_format(branch_get_color(BRANCH_COLOR_REMOTE)),
>                 ...);
>
> and the implementation of the helper may look like:
>
>     const char *quote_literal_for_format(const char *s)
>     {
>         static strbuf buf = STRBUF_INIT;
>
>         strbuf_reset(&buf);
>         while (*s) {
>             const char *ep = strchrnul(s, '%');
>             if (s < ep)
>                 strbuf_add(&buf, s, ep - s);
>             if (*ep == '%') {
>                 strbuf_addstr(&buf, "%%");
>                 s = ep + 1;
>             } else {
>                 s = ep;
>             }
>         }
>         return buf.buf;
>     }
>

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Call ref-filter's setup_ref_filter_porcelain_msg() to enable
> translated messages for the %(upstream:tack) atom. Although branch.c
> doesn't currently use ref-filter's printing API's, this will ensure
> that when it does in the future patches, we do not need to worry about
> translation.
>

> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> 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>
> ---
>  builtin/branch.c | 2 ++
>  builtin/tag.c    | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index be9773a..dead2b8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -656,6 +656,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>                 OPT_END(),
>         };
>
> +       setup_ref_filter_porcelain_msg();
> +
>         memset(&filter, 0, sizeof(filter));
>         filter.kind = FILTER_REFS_BRANCHES;
>         filter.abbrev = -1;
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 50e4ae5..a00e9a7 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -373,6 +373,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>                 OPT_END()
>         };
>
> +       setup_ref_filter_porcelain_msg();
> +
>         git_config(git_tag_config, sorting_tail);
>
>         memset(&opt, 0, sizeof(opt));
> --
> 2.10.2
>

> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
>
> Initially posted here: $(gmane/279226). It was decided that this series
> would follow up after refactoring ref-filter parsing mechanism, which
> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>
> v1 can be found here: $(gmane/288342)
> v2 can be found here: $(gmane/288863)
> v3 can be found here: $(gmane/290299)
> v4 can be found here: $(gmane/291106)
> v5b can be found here: $(gmane/292467)
> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
>

> Changes in this version:
>
> 1. Rebased on top of master.
>
> Karthik Nayak (17):
>   ref-filter: implement %(if), %(then), and %(else) atoms
>   ref-filter: include reference to 'used_atom' within 'atom_value'
>   ref-filter: implement %(if:equals=<string>) and
>     %(if:notequals=<string>)
>   ref-filter: modify "%(objectname:short)" to take length
>   ref-filter: move get_head_description() from branch.c
>   ref-filter: introduce format_ref_array_item()
>   ref-filter: make %(upstream:track) prints "[gone]" for invalid
>     upstreams
>   ref-filter: add support for %(upstream:track,nobracket)
>   ref-filter: make "%(symref)" atom work with the ':short' modifier
>   ref-filter: introduce refname_atom_parser_internal()
>   ref-filter: introduce symref_atom_parser() and refname_atom_parser()
>   ref-filter: make remote_ref_atom_parser() use
>     refname_atom_parser_internal()
>   ref-filter: add `:dir` and `:base` options for ref printing atoms
>   ref-filter: allow porcelain to translate messages in the output
>   branch, tag: use porcelain output
>   branch: use ref-filter printing APIs
>   branch: implement '--format' option
>
>  Documentation/git-branch.txt       |   7 +-
>  Documentation/git-for-each-ref.txt |  82 ++++++-
>  builtin/branch.c                   | 277 +++++++---------------
>  builtin/tag.c                      |   2 +
>  ref-filter.c                       | 456 +++++++++++++++++++++++++++++++------
>  ref-filter.h                       |   7 +
>  t/t3203-branch-output.sh           |  16 +-
>  t/t6040-tracking-info.sh           |   2 +-
>  t/t6300-for-each-ref.sh            |  73 +++++-
>  t/t6302-for-each-ref-filter.sh     |  94 ++++++++
>  10 files changed, 725 insertions(+), 291 deletions(-)
>
> --
> 2.10.2
>

> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> This is part of unification of the commands 'git tag -l, git branch -l
>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>> printing options.
>>
>> Initially posted here: $(gmane/279226). It was decided that this series
>> would follow up after refactoring ref-filter parsing mechanism, which
>> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>>
>> v1 can be found here: $(gmane/288342)
>> v2 can be found here: $(gmane/288863)
>> v3 can be found here: $(gmane/290299)
>> v4 can be found here: $(gmane/291106)
>> v5b can be found here: $(gmane/292467)
>> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
>>
>
> I reviewed the full series. I found a few minor things I would have
> done differently, but overall I think it looks good. Thanks for the
> hard work and the time invested here. I remember seeing this on the
> list quite some time ago, so it's nice to see it finally come
> together.
>

> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
>
> Karthik Nayak (17):
>   ref-filter: implement %(if), %(then), and %(else) atoms
>   ref-filter: include reference to 'used_atom' within 'atom_value'
>   ref-filter: implement %(if:equals=<string>) and
>     %(if:notequals=<string>)
>   ref-filter: modify "%(objectname:short)" to take length
>   ref-filter: move get_head_description() from branch.c
>   ref-filter: introduce format_ref_array_item()
>   ref-filter: make %(upstream:track) prints "[gone]" for invalid
>     upstreams
>   ref-filter: add support for %(upstream:track,nobracket)
>   ref-filter: make "%(symref)" atom work with the ':short' modifier
>   ref-filter: introduce refname_atom_parser_internal()
>   ref-filter: introduce symref_atom_parser() and refname_atom_parser()
>   ref-filter: make remote_ref_atom_parser() use
>     refname_atom_parser_internal()
>   ref-filter: add `:dir` and `:base` options for ref printing atoms
>   ref-filter: allow porcelain to translate messages in the output
>   branch, tag: use porcelain output
>   branch: use ref-filter printing APIs
>   branch: implement '--format' option

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This is part of unification of the commands 'git tag -l, git branch -l
>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>> printing options.
>>
>> Karthik Nayak (17):
>>   ref-filter: implement %(if), %(then), and %(else) atoms
>>   ref-filter: include reference to 'used_atom' within 'atom_value'
>>   ref-filter: implement %(if:equals=<string>) and
>>     %(if:notequals=<string>)
>>   ref-filter: modify "%(objectname:short)" to take length
>>   ref-filter: move get_head_description() from branch.c
>>   ref-filter: introduce format_ref_array_item()
>>   ref-filter: make %(upstream:track) prints "[gone]" for invalid
>>     upstreams
>>   ref-filter: add support for %(upstream:track,nobracket)
>>   ref-filter: make "%(symref)" atom work with the ':short' modifier
>>   ref-filter: introduce refname_atom_parser_internal()
>>   ref-filter: introduce symref_atom_parser() and refname_atom_parser()
>>   ref-filter: make remote_ref_atom_parser() use
>>     refname_atom_parser_internal()
>>   ref-filter: add `:dir` and `:base` options for ref printing atoms
>>   ref-filter: allow porcelain to translate messages in the output
>>   branch, tag: use porcelain output
>>   branch: use ref-filter printing APIs
>>   branch: implement '--format' option
>
> This is not a new issue, but --format='%(HEAD)' you stole from
> for-each-ref is broken when you are on an unborn branch, and the
> second patch from the tip makes "git branch" (no other args) on
> an unborn branch to segfault, when there are real branches that
> have commits.
>
> Something like this needs to go before that step.
>
>  ref-filter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 944671af5a..c71d7360d2 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1318,7 +1318,7 @@ static void populate_value(struct ref_array_item *ref)
>
>                         head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
>                                                   sha1, NULL);
> -                       if (!strcmp(ref->refname, head))
> +                       if (head && !strcmp(ref->refname, head))
>                                 v->s = "*";
>                         else
>                                 v->s = " ";
>
>

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Thanks, will add it in.
>
> OK, here is a reroll of what I sent earlier in
>
>     http://public-inbox.org/git/<xmqq7f84tqa7.fsf_-_@gitster.mtv.corp.google.com>
>
> but rebased so that it can happen as a preparatory bugfix before
> your series.
>
> The bug dates back to the very original implementation of %(HEAD) in
> 7a48b83219 ("for-each-ref: introduce %(HEAD) asterisk marker",
> 2013-11-18) and was moved to the current location in the v2.6 days
> at c95b758587 ("ref-filter: move code from 'for-each-ref'",
> 2015-06-14).
>