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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 7, 2024, 02:52 UTC
Message-ID: <xmqqo72rvjqk.fsf@gitster.g>
In-Reply-To: <20241106185102.GA880133@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

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

Show 5 quoted lines

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