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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Mar 19, 2025 at 08:23:37AM +0100, Martin Ågren wrote:
> When we parse a padding directive ("%<" or "%>"), we might populate a
> few of the struct's fields before bailing. This can result in such
> half-parsed information being used to actually introduce some
> padding/truncation.
> 
> 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.

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.

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

> While setting up a temporary struct for parsing into, we might as well
> initialize it to a well-defined state. It's unnecessary for the current
> implementation since it always writes to all three fields in a
> successful case, but some future-proofing shouldn't hurt.
> 
> Note that the test relies on first using a correct placeholder
> "%<(4,trunc)" where "trunc" (`trunc_right`) lingers in our struct until
> it's then used instead of the invalid "bad". The next commit will teach
> us to clean up any remnants of "%<(4,trunc)" after handling it.
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  pretty.c                      | 18 ++++++++++++------
>  t/t4205-log-pretty-formats.sh |  6 ++++++
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/pretty.c b/pretty.c
> index e5e8ef24fa..a4fa052f8b 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1121,6 +1121,11 @@ static size_t parse_padding_placeholder(const char *placeholder,
>  	const char *ch = placeholder;
>  	enum flush_type flush_type;
>  	int to_column = 0;
> +	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.

Patrick