On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>
> 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.
>
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.
Thanks,
Jake
> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
> Mentored-by: Matthieu Moy <matthieu.moy@xxxxxxxxxxxxxxx>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> ---
> 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.
> +
>
Nice, I like the explanation above.
> 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)
> {
So we now have to pass an array of stacks to the end_align_handler
instead? Ok. But for align this is simple since it just expects a
singleton.
> - 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);
So here, once we have a satisfied condition, we just drop the "else"
element entirely.
> + } else {
> + strbuf_swap(&cur->output, &prev->output);
> + strbuf_reset(&cur->output);
> + pop_stack_element(&cur);
Otherwise, we swap our current value into the value of the previous
element, and then drop the current. This is a bit tricky, but it
works.
> + }
> + } 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);
Finally, if no else element, then we just reset the current pointer.
> +
> + *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;
> +}
> +
Ok, so the new method is that to handle "if"s we push the sets onto
the stack and check their values. I like this, it makes things pretty
straight forward and simple. Allows for quite a bit of expression.
> +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);
> +}
So once we have a "%(then)" atom, we reset all the accumulated string
data we've gotten so far. Simple.
> +
> +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;
> +}
So for an else atom, we basically create another stack element on top
of the current one. Nice.
> +
> 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(¤t->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
>