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

From: Martin Ågren <martin.agren@gmail.com>
Date: Mar 20, 2025, 16:11 UTC
Message-ID: <CAN0heSpN-k886+RsZ0+djLd974Mq57B4quZK1yKXRMxCnOvzZw@mail.gmail.com>
In-Reply-To: <Z9vdS4bxY6spILsc@pks.im>

On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:

Show 11 quoted lines

>
> 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.

Show 5 quoted lines

> > We could let the caller use a temporary struct and only copy the data on
> > success. Let's instead make our parsing function easy to use correctly
> > by letting it only touch the output struct in the success case.
>
> s/success/&ful/

Thanks.

Show 12 quoted lines

> > +     struct padding_args ans = {
> > +             .flush_type = no_flush,
> > +             .truncate = trunc_none,
> > +             .padding = 0,
> > +     };
> >
> >       switch (*ch++) {
> >       case '<':
>
> I honestly have no idea what `ans` stands for. You could call it
> `result` to signify that it's what we'll ultimately bubble up to the
> caller in the successful case.

Fair. :-) It's "answer", but "result" is much better. Thanks.

Martin