| Message ID | e34ae37982e76179aee780c70b48aaaf959a307b.1742367347.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pretty: minor bugfixing, some refactorings | expand |
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
On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote: > > On Wed, Mar 19, 2025 at 08:23:38AM +0100, Martin Ågren wrote: > Yeah, I agree. It's very surprising that we retain only a subset of > state, and that does feel like a bug to me. > > > 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. Good point. I'll make the preceding commit use a new `padding_args_clear()`, then reuse it here. BTW, we rely on initializing the struct with all-zeroes to put it in this cleared state. Which is true, since the "none"/"no" enum members are indeed zero. That's not explicit though. I'm thinking of adding a preparatory patch to make `no_flush` and `trunc_none` be explicitly zero, and see if there are other such enum values in this file. Martin
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; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 26987ecd77..d34a7cec09 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -1124,6 +1124,15 @@ test_expect_success 'log --pretty with space stealing' ' test_cmp expect actual ' +test_expect_success 'only the next placeholder gets truncated' ' + { + git log -1 --pretty="format:%<(4,trunc)%H" && + printf "$(git rev-parse HEAD)" + } >expect && + git log -1 --pretty="format:%<(4,trunc)%H%H" >actual && + test_cmp expect actual +' + test_expect_success 'log --pretty with invalid padding format' ' printf "%s%%<(20" "$(git rev-parse HEAD)" >expect && git log -1 --pretty="format:%H%<(20" >actual &&
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. 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(+)