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

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Nov 8, 2024, 18:12 UTC
Message-ID: <Zy5Ui0tHtKL1vYpw@five231003>
In-Reply-To: <20241108171637.GA548990@coredump.intra.peff.net>

On Fri, Nov 08, 2024 at 12:16:37PM -0500, Jeff King wrote:

Show 13 quoted lines

> On Fri, Nov 08, 2024 at 09:41:03AM +0530, Kousik Sanagavarapu wrote:
> 
> [...]
> 
> 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.

Oh!  I remember this, let me search up the thread...

Quoting you from

	https://lore.kernel.org/git/20230901191639.GA1955435@coredump.intra.peff.net/

    IMHO the code would be a lot easier to work with if the atoms were
    structured as a parse tree with child pointers (especially when you get
    into things like "if" that have sub-expressions). I think one of the
    reasons that used_atom is an array is to de-duplicate repeated mentions
    (so if you formatted "%(foo) %(foo)" it would only have to store the
    computed value once).

    But I think that is the wrong way to optimize it. We shouldn't be
    storing any strings per-atom, but rather walking the parse tree to
    produce a single output buffer. And the values should be cheap to fill
    in, because we should parse the object as necessary up front. This is
    more or less the way the pretty.c parser does it.

> But I think it's a non-trivial task.

True.