Jeff King <peff@xxxxxxxx> writes: > who is doing: > > %(if:equals=%%foo) > > to match the literal "%%foo" will be broken if we change that. They are > not doing anything wrong; that is the only way to make it work now. Ah, you're absolutely right. Unescaping would start breaking them. > I wouldn't go so far as to call the current behavior a bug. It's > just...not very flexible. I also think it is unlikely that anybody would > care in practice (though I find matching refs with ")" in them already a > bit far-fetched). 100% agreed. For that matter, I find "if:equals=%%foo" equally implausible. > If we wanted to be extra careful, we could introduce a variant of > "equals" that indicates that it will be expanded before comparison. Or > even an extra tag, like: > > %(if:expand:equals=%%foo) Surely, but if nobody screams, I am tempted to suggest fixing the equals/notequals---we do not have to be bug-to-bug compatible with a buggy old implementation. After all, we do expand the string being inspected that appears between %(if) and %(then). I do not think of a good excuse for us to limit the string that it gets compared with to literals. The implementation may be a bit involved, but shouldn't be too bad. When .str is an empty string in if_atom_handler(), we can follow what the current code does. If .str is not empty, allocate a new stack element in order to parse the .str to its end by pointing .at_end of the new stack element to a new handler (call it if_cond_handler()), and pass the if_then_else structure it allocated as .at_end_data to it. And in the if_cond_handler(), grab the cur->output and overwrite the .str member with it (while being careful to avoid leaks). At the end of the if_cond_handler(), pass control to if_then_else_handler() by arranging the if_then_else_handler is called, imitating the way how if_atom_handler() passes control to if_then_else_handler() in the current code. Then things like %(if:equals=%(upstream:lstrip=3))%(refname:short)%(then)... would work as expected ;-)
