Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

On Sat, Oct 3, 2015 at 3:09 PM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

Implement %(if), %(then) and %(else) atoms. Used as
%(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end).

I prefer ... to .., which often means "interval" as in HEAD^^..HEAD.

Seems good, will change.

quoted

If there is an atom with value or string literal after the %(if)

I find this explanation hard to read, and ambiguous: what does "atom
with value" mean?

quoted

then everything after the %(then) is printed, else if the %(else) atom
is used, then everything after %(else) is printed. If the string
contains only whitespaces, then it is not considered.

"the string" is ambiguous again. I guess it's "what's between %(if) and
%(then)", but it could be clearer. And it's not clear what "not
considered" means.

My take on it:

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 string following %(then) is printed.
Otherwise, the string following %(else), if any, is printed.

This (the one you sent again after Junio's suggestion, looks better),
I'll use this thanks.

quoted

+When a scripting language specific quoting is in effect,

This may not be immediately clear to the reader. I'd add explicitly:

When a scripting language specific quoting is in effect (i.e. one of
`--shell`, `--perl`, `--python`, `--tcl` is used), ...

Makes sense.

quoted

 EXAMPLES
 --------

This is just the context of the patch, but I read it as a hint that we
could add some examples with complex --format usage to illustrate the
theory above.

I was thinking about adding :

An example to show the usage of %(if)...%(then)...%(else)...%(end).
This prefixes the current branch with a star.

------------
#!/bin/sh

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 adds a red color to authorname, if present

------------
#!/bin/sh

git for-each-ref
--format="%(refname)%(if)%(authorname)%(then)%(color:red)%(end)
%(authorname)"
------------

quoted

+     if (if_then_else->condition_satisfied && if_then_else->else_atom) {

// cs && else

quoted

+             strbuf_reset(&cur->output);
+             pop_stack_element(&cur);
+     } else if (if_then_else->else_atom) {

// !cs && else

quoted

+             strbuf_swap(&cur->output, &prev->output);
+             strbuf_reset(&cur->output);
+             pop_stack_element(&cur);
+     } else if (!if_then_else->condition_satisfied)

// !cs && !else

quoted

+             strbuf_reset(&cur->output);

This if/else if/else if looks hard to read to me. I had to add the
comments above as a note to myself to get the actual full condition for
3 branches.

The reasoning would be clearer to me as:

if (if_then_else->else_atom) {
        /*
         * 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);
        } else {
                strbuf_swap(&cur->output, &prev->output);
                strbuf_reset(&cur->output);
                pop_stack_element(&cur);
        }
} else if (if_then_else->condition_satisfied)
        /*
         * No %(else) atom: just drop the %(then) branch if the
         * condition is not satisfied.
         */
        strbuf_reset(&cur->output);

This looks neater thanks.

quoted

+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);
+
+     if_then_else->if_atom = 1;

Do you ever use this "if_atom"? It doesn't seem so in the current patch,
and it seems like a tautology to me: if you have a struct if_then_else,
then you have seen the %(if).

Yea I'll remove that.

quoted

+static int is_empty(const char * s){

char * s -> char *s

will do.

quoted

+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 = (struct if_then_else *)cur->at_end_data;
+
+     if (!if_then_else)
+             die(_("format: %%(then) atom used without an %%(if) atom"));

You've just casted at_end_data to if_then_else. if_then_else being not
NULL does not mean that it is properly typed. It can be the at_end_data
of another opening atom. What happens if you use
%(align)foo%(then)bar%(end)?

Nice catch, didn't see that possibility.

One way to be safer would be to check that cur->at_end does point to
if_then_else_handler. Or add information to struct ref_formatting_stack
(a Boolean is_if_then_else or an enum).

Checking cur->at_end with if_then_else_handler seems good to me.

Also, you need to check that if_then_else->then_atom is not 1.

Ah! multiple usage of the same atom.

quoted

+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 = (struct if_then_else *)state->stack->at_end_data;
+
+     if (!if_then_else)
+             die(_("format: %%(else) atom used without an %%(if) atom"));

Same as above, I guess (not tested) %(align)...%(else) is accepted.

Will change.

quoted

+     if (!if_then_else->then_atom)
+             die(_("format: %%(else) atom used without a %%(then) atom"));
+     if_then_else->else_atom = 1;
+     push_stack_element(&state->stack);

So, while parsing the %(else)...%(end), the stack contains both the
%(then)...%(else) part, and the %(else)...%(end).

I'm wondering if we can simplify this. We already know if the condition
is satisfied, and if it's not, we can just drop the %(then) part right
now, and write to the top of the stack normally (the %(end) atom will
only have to pop the string normally). But if the condition is not
satisfied, we need to preserve the %(then) part and need to do something
about the %(else).

I wanted to do something like this the problem is append_atom() and
append_literal()
would need to be informed about which part to ignore, and this moves
the code's logic
from the current handlers to append_atom() and append_literal(). Which I didn't
think was a nice way of doing this.

quoted

-     current->at_end(current);
+     current->at_end(&state->stack);
+
+     /*  Stack may have been popped, hence reset the current pointer */

I'd say explicitly "... may have been popped within at_end, hence ..."

Will do.

-- 
Regards,
Karthik Nayak