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

From: Jeff King <peff@peff.net>
Date: Nov 6, 2024, 18:51 UTC
Message-ID: <20241106185102.GA880133@coredump.intra.peff.net>
In-Reply-To: <xmqq8qtxqcye.fsf@gitster.g>

On Tue, Nov 05, 2024 at 07:05:13PM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I am tempted to say the solution is to expand that "equals" value, and
> > possibly add some less-arcane version of the character (maybe "%)"?).
> > But it be a break in backwards compatibility if somebody is trying to
> > match literal %-chars in their "if" block.
> 
> If they were trying to write a literal %, wouldn't they be writing
> %% already, not because % followed by a byte without any special
> meaning happens to be passed intact by the implementation, but
> because that is _the_ right thing to do, when % is used as an
> introducer for escape sequences?  So I do agree it would be a change
> that breaks backward compatibility but I do not think we want to
> stay bug to bug compatible with the current behaviour here.

I think "because that is the right thing to do" is what is in question. It is not like we happen to allow "%", but you should be writing "%%" in an if:equals value already. They mean two different things, and anybody 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.

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

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)

> I am not sure with the wisdom of %) though.  Wouldn't "%(foo %)" look
> as if %( opens and %) closes a group in our language?

Yeah, I agree it is ugly and possibly confusing. Normally I'd suggest "\" for escaping, but it isn't otherwise syntactically important within these formats (I don't think, anyway). The magic character is "%" so that is what we have to work with.

-Peff