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

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Nov 6, 2024, 02:40 UTC
Message-ID: <ZyrXIKU8Qn46Z0LF@five231003>
In-Reply-To: <xmqqikt1qhwt.fsf@gitster.g>

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

Show 7 quoted lines

> Kousik Sanagavarapu <five231003@gmail.com> writes:
> 
> > Document that values containing ')' in formats are not parsed correctly
> > in ref-filter.
> 
> The problem is probably lack of a way to quote such a closing
> parenthesis.

Yes correct. We currently don't have a way to quote such strings in "value" of "%(atom:someparam=value)"

Show 31 quoted lines

> > However formats having a '(' instead in "value" will parse correctly
> > because in a general format string we also mark start of the format by
> > making note of '%(' instead of just '('.
> 
> So if you wanted to have a two-char sequence '%(' in value, you'd
> see a similar problem?  If so, it is not quite a "bug" or "not
> parsed correctly"---it is "because there is no way to include
> closing ')' in the value (e.g., by quoting), you cannot write such a
> string in the value part".
> 
> > 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.
> 
> 
> 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.

Hmm, but hex escapes do work as intended and the problem here is not the ')' outside the atom but within it. To be more clear, let's take

	$ git for-each-ref --format="%(if:equals=refs/heads/step-1)start)%(refname)%(then)%(objectname:short)%(end)" refs/heads/

(Sorry for not wrapping the line above x<)

First let's notice the difference between what these two commands are trying to do. Your command asks to print all the refs matching "refs/heads/master" in the format of (%(refname)) and since we support escaping literals in the form of %xx, where xx is the hexcode of the literal to be escaped during the parsing of the format, this would obviously work as intended.

Now let's come to my command. My command asks to print all the abbreviated commit ids of the refs which compare equal to "refs/heads/step-1)start" from all of "refs/heads/". Now here, since ref-filter parses the format string by making note of '%(' and ')', it accidentally thinks that I want to compare equality with "refs/heads/step-1" instead of "refs/heads/step-1)start", which I actually wanted. If my local repo contained both the "refs/heads/step1" and "refs/heads/step1)start", wouldn't this be a bug?

So I do agree that it is a lack of quoting when entering the "value" part of "%(atom:someparam=value)", but another part of me also thinks that ref-filter should be intelligent enough while parsing the format string to acknowledge where exactly the atom ends and which is the last closing ')' and hence follows whatever I wrote below the "---" line, till the script.

Also, I'm thinking the commit msg was not clear as it lead you (perhaps someone else too when they visit this topic) to think about escape literals while that not exactly is the problem I'm trying to get at.

Also if you think a change to the documentation would be more proper than reflecting this with a test breakage, I'll do that. My intention with the test was that - in the future if we parse the "value" correctly then that commit would also include a

s/test_expect_failure/test_expect_success

change.

Thanks!