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

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Nov 8, 2024, 04:11 UTC
Message-ID: <Zy2PV+yywkS64D1p@five231003>
In-Reply-To: <xmqqo72rvjqk.fsf@gitster.g>

On Thu, Nov 07, 2024 at 11:52:03AM +0900, Junio C Hamano wrote:

Show 23 quoted lines

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