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

From: Jeff King <peff@peff.net>
Date: Nov 6, 2024, 02:25 UTC
Message-ID: <20241106022552.GA816908@coredump.intra.peff.net>
In-Reply-To: <xmqqikt1qhwt.fsf@gitster.g>

On Tue, Nov 05, 2024 at 05:18:10PM -0800, Junio C Hamano wrote:

Show 8 quoted lines

> > This raises the question of what can be done to parse ')' in values of
> > the format correctly.  It seems to me like a clean solution would
> > involve a huge refactoring involving a large portion of ref-filter but I
> > maybe wrong.
> 
> Yes, so I wouldn't even call the current behaviour "bug".  The
> language is merely "limited" and the user cannot express certain
> values with it at all.

Agreed. I think we may have discussed this quoting problem before, but it's not usually a big deal because the set of likely values is quite limited. Most of them are just keywords or numeric values. I _think_ that the equals/notequals parameters of %(if) are the only ones.

Which isn't to say we shouldn't make things better if we can. Just that I am not too surprised nobody has run into it before.

Show 11 quoted lines

> Having said that, I just tried this
> 
>     $ git for-each-ref --format='%28%(refname)%29' refs/heads/master
>     (refs/heads/master)
> 
> So, if there is anything that needs "fixing", wouldn't it be
> documentation?
> 
> If I knew (or easily find out from "git for-each-ref --help") that
> hex escapes %XX can be used, I wouldn't have written any of what I
> said before "Having said that" in this response.

I tried something similar, but I don't think it quite works for the case in question. Within %(if:equals=<foo>) we do not further expand the <foo> value (at least from my limited tests). And so something like:

  git for-each-ref --format='%(if:equals=ref-with-%29)%(refname:short)...etc'

would never match "ref-with-(", but only a literal "ref-with-%29".

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.

Another option: in the rest of the "if" design we tried to keep arbitrary text outside of the parentheses. So you could imagine a syntax like:

  %(if:equals)ref-with-)%(foo)%(refname:short)%(then)...%(end)

where %(foo) is some placeholder that separate the two arguments to the "equals". In sane languages that is a space or a comma, but I'm not sure that works here. We have %(end) which would otherwise be a syntax error here, but it feels word. I dunno. The whole language is kind of hideously verbose. I feel sorry for anybody trying to write non-trivial formats. :)

-Peff