Re: [PATCH] t6300: values containing ')' are broken in ref formats

[Kousik Sanagavarapu <five231003@gmail.com>]

On Thu, Nov 07, 2024 at 11:52:03AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> [...]
> 
> 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)...

So if I understand correctly, we grab the .str and operate on it so that
we expand the atom within it and then do the comparision.

This seems nice, but there is a problem.  Since we always look for the
first occurring ')' in our format string to indicate the end of the atom,
we end up with

	.str = %(upstream:lstrip=3

(the call chain is

	verify_ref_format() -> parse_ref_filter_atom() -> if_atom_parser()
)

Since we have now left out a ')', this ')' gets appended to our output
buf, which would also show up in cur->output when we do the comparision
in then_atom_handler().  For example, in this case our cur->output would
be ")master" instead of "master" after we get the value of
%(refname:short), meaning our comparision always fails.