Re: [PATCH 5/8] pretty: after padding, reset padding info

[Patrick Steinhardt <ps@pks.im>]

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@gmail.com>
> ---
>  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