On Wed, Mar 19, 2025 at 08:23:38AM +0100, Martin Ågren wrote:
> After handling a padding directive ("%<" or "%>"), we leave the `struct
> padding_args` in a halfway state. We modify it a bit as we apply the
> padding/truncation so that by the time we're done, it can't be in quite
> as many states as when we started. Still, we don't fully restore it to
> its default, no-action state.
>
> "%<" and "%>" should only affect the next placeholder, but leaving a bit
> of state around doesn't make it obvious that we don't spill any of it
> into our handling of later placeholders. The previous commit closed off
> a way of populating only half the `struct padding_args`, thereby fixing
> a bug that *also* relied on then having the other half contain this kind
> of lingering data.
>
> After that fix, I haven't figured out a way to provoke a bug using just
> this here half of the issue. Still, after handling padding, let's drop
> all remnants of the previous "%<" or "%>".
>
> Unlike the bug fixed in the previous commit, this could have some
> realistic chance of regressing something for someone if they've actually
> been using such state leftovers (knowingly or not). Still, it seems
> worthwhile to try to tighten this.
Yeah, I agree. It's very surprising that we retain only a subset of
state, and that does feel like a bug to me.
> This change to pretty.c would have been sufficient to make the test
> added in the previous commit pass. Belt and suspenders.
>
> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
> ---
> pretty.c | 2 ++
> t/t4205-log-pretty-formats.sh | 9 +++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/pretty.c b/pretty.c
> index a4fa052f8b..f53e77ed86 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1893,6 +1893,8 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
> }
> strbuf_release(&local_sb);
> c->pad.flush_type = no_flush;
> + c->pad.truncate = trunc_none;
> + c->pad.padding = 0;
> return total_consumed;
> }
This is using the same default values now as you started to use in the
preceding commit. It might make sense to introduce a macro or function
to initialize the structure so that we don't duplicate initialization.
Patrick
