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@xxxxxxxxx>
> ---
> 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
