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

Matthieu Moy [off-list ref] writes:

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.

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), ...

I found all the suggestions very good, except that the distinction
between "expands to" and "is printed" bothers me a bit, as they want
to mean exactly the same thing (imagine this whole thing were inside
another %(if)...%(then)).

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.

Yes ;-)

quoted

+static int is_empty(const char * s){

char * s -> char *s

Also "{" must sit alone on the next line.

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)?

One way to be safer would be to check that cur->at_end does point to
if_then_else_handler.

Good eyes.  Thanks.

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).

Good suggestion.

Thanks.