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

From: Jeff King <peff@peff.net>
Date: Nov 8, 2024, 17:16 UTC
Message-ID: <20241108171637.GA548990@coredump.intra.peff.net>
In-Reply-To: <Zy2PV+yywkS64D1p@five231003>

On Fri, Nov 08, 2024 at 09:41:03AM +0530, Kousik Sanagavarapu wrote:

Show 23 quoted lines

> > Then things like
> > 
> >   %(if:equals=%(upstream:lstrip=3))%(refname:short)%(then)...
> 
> So if I understand correctly, we grab the .str and operate on it so that
> we expand the atom within it and then do the comparision.
> 
> This seems nice, but there is a problem.  Since we always look for the
> first occurring ')' in our format string to indicate the end of the atom,
> we end up with
> 
> 	.str = %(upstream:lstrip=3
> 
> (the call chain is
> 
> 	verify_ref_format() -> parse_ref_filter_atom() -> if_atom_parser()
> )
> 
> Since we have now left out a ')', this ')' gets appended to our output
> buf, which would also show up in cur->output when we do the comparision
> in then_atom_handler().  For example, in this case our cur->output would
> be ")master" instead of "master" after we get the value of
> %(refname:short), meaning our comparision always fails.

Yes, though I think the parser _could_ be improved here. This is different the earlier case of matching "ref-with-)". In that case it is syntactically ambiguous. For example given:

  %(if:equals=ref-with-))foo

you cannot tell the difference between:

  - matching "ref-with-)", followed by "foo"

  - matching "ref-with-", followed by ")foo"

But if the parenthesis in question is closing a %() item, like:

  %(if:equals=%(refname))foo

then we know that the inner ")" is closing %(refname), since parsing it as "%(refname" followed by ")foo" would leave an unbalanced pair. But finding that would require a real recursive descent parser, rather than a blind strchr() for the closing ")"[1].

In the meantime yeah, you'd have to spell it as:

  %(if:equals=%(refname%29)

which is...deeply unsatisfying.

I have long dreamed of throwing out all of this format code in favor of a recursive parser which generates an actual tree of nodes, and implements all of the ref-filter/pretty.c/cat-file format placeholders. But I think it's a non-trivial task.

-Peff

[1] Incidentally, the "%)" I proposed earlier would also fall afoul of
    this problem. The search for the closing ")" is done blindly without
    regard to possible quoting.