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

Karthik Nayak <karthik.188@gmail.com> writes:

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

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

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

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.

// cs && else

// !cs && else

// !cs && !else

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

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

char * s -> char *s

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.

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

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'd say explicitly "... may have been popped within at_end, hence ..."