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

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Nov 7, 2024, 02:29 UTC
Message-ID: <ZywmElhgd1om2Y3E@five231003>
In-Reply-To: <20241106185102.GA880133@coredump.intra.peff.net>

On Wed, Nov 06, 2024 at 01:51:02PM -0500, Jeff King wrote:

Show 26 quoted lines

> On Tue, Nov 05, 2024 at 07:05:13PM -0800, Junio C Hamano wrote:
> 
> > 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.

True.

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

Yeah. I really don't think anyone in practice will hit upon this case. As I mentioned already before, I was just trying to pick out a corner case for another implementation in ref-filter and stumbled upon this.

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)

This seems like a nice idea, if we are thinking about not breaking backwards compatibility but then there is also this discussion about the formats being too verbose but I dunno.