Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> 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.
> 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?
> 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.
> +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), ...
> 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.
> + if (if_then_else->condition_satisfied && if_then_else->else_atom) {
// cs && else
> + strbuf_reset(&cur->output);
> + pop_stack_element(&cur);
> + } else if (if_then_else->else_atom) {
// !cs && else
> + strbuf_swap(&cur->output, &prev->output);
> + strbuf_reset(&cur->output);
> + pop_stack_element(&cur);
> + } else if (!if_then_else->condition_satisfied)
// !cs && !else
> + 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);
> +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).
> +static int is_empty(const char * s){
char * s -> char *s
> +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. Or add information to struct ref_formatting_stack
(a Boolean is_if_then_else or an enum).
Also, you need to check that if_then_else->then_atom is not 1.
> +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.
> + 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).
> - 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 ..."
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html