Re: [PATCH 4/8] pretty: fix parsing of half-valid "%<" and "%>" placeholders

[Patrick Steinhardt <ps@pks.im>]

On Thu, Mar 20, 2025 at 05:11:09PM +0100, Martin Ågren wrote:
> On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Wed, Mar 19, 2025 at 08:23:37AM +0100, Martin Ågren wrote:
> > > When parsing a "%<" or "%>", only store the parsed data after parsing
> > > successfully. The added test would have failed before this commit. It
> > > also shows how the existing behavior is hardly something someone can
> > > rely on since the non-consumed modifier ("%<(10,bad)") shows up verbatim
> > > in the pretty output.
> >
> > Ideally I'd expect us to die when seeing misformatted placeholders like
> > this. This is way less confusing to the user as otherwise things _look_
> > like they work, but we silently do the wrong thing.
> 
> Right. I can see how it makes some kind of sense to print what we don't
> understand when it's something short and simple like "%X". But for more
> complex "%X(first,second)" it's kind of obvious that a misspelled
> "X(fist,second)" isn't something you want in the output. The whole "if
> we can't parse, return zero as the number of consumed characters so that
> we can print verbatim while looking for next '%'" is a central piece of
> the design here. One could certainly imagine a "strict" mode.
> 
> > That being said, I have no idea whether we can do such a change now
> > without breaking existing usecases. As you rightfully argue the result
> > already is wrong, but with my proposal we'd completely refuse to do
> > anything. Which I'd argue is a good thing in the end.
> 
> I can see the value of a strict mode, with command line options and
> config switches and whatnot, maybe even a changed default behavior at
> some point. I'd rather punt on that for now. TBH, I'd be afraid to do a
> hard switch from "0 means print it instead" to "0 means die". I don't
> disagree that it would be a better end-game though, at some point.

Yup, I fully agree that this is a bit more of a risky change and that it
doesn't have to be part of this patch series.

Patrick