95 lines
3.7 KiB
Diff
95 lines
3.7 KiB
Diff
From 1de69c0cdd388b0a5b7bdde0bfa0bda514a354b0 Mon Sep 17 00:00:00 2001
|
|
From: Patrick Steinhardt <ps@pks.im>
|
|
Date: Thu, 1 Dec 2022 15:46:39 +0100
|
|
Subject: [PATCH] pretty: fix adding linefeed when placeholder is not expanded
|
|
|
|
When a formatting directive has a `+` or ` ` after the `%`, then we add
|
|
either a line feed or space if the placeholder expands to a non-empty
|
|
string. In specific cases though this logic doesn't work as expected,
|
|
and we try to add the character even in the case where the formatting
|
|
directive is empty.
|
|
|
|
One such pattern is `%w(1)%+d%+w(2)`. `%+d` expands to reference names
|
|
pointing to a certain commit, like in `git log --decorate`. For a tagged
|
|
commit this would for example expand to `\n (tag: v1.0.0)`, which has a
|
|
leading newline due to the `+` modifier and a space added by `%d`. Now
|
|
the second wrapping directive will cause us to rewrap the text to
|
|
`\n(tag:\nv1.0.0)`, which is one byte shorter due to the missing leading
|
|
space. The code that handles the `+` magic now notices that the length
|
|
has changed and will thus try to insert a leading line feed at the
|
|
original posititon. But as the string was shortened, the original
|
|
position is past the buffer's boundary and thus we die with an error.
|
|
|
|
Now there are two issues here:
|
|
|
|
1. We check whether the buffer length has changed, not whether it
|
|
has been extended. This causes us to try and add the character
|
|
past the string boundary.
|
|
|
|
2. The current logic does not make any sense whatsoever. When the
|
|
string got expanded due to the rewrap, putting the separator into
|
|
the original position is likely to put it somewhere into the
|
|
middle of the rewrapped contents.
|
|
|
|
It is debatable whether `%+w()` makes any sense in the first place.
|
|
Strictly speaking, the placeholder never expands to a non-empty string,
|
|
and consequentially we shouldn't ever accept this combination. We thus
|
|
fix the bug by simply refusing `%+w()`.
|
|
|
|
Signed-off-by: Patrick Steinhardt <ps@pks.im>
|
|
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
---
|
|
pretty.c | 14 +++++++++++++-
|
|
t/t4205-log-pretty-formats.sh | 8 ++++++++
|
|
2 files changed, 21 insertions(+), 1 deletion(-)
|
|
|
|
diff --git a/pretty.c b/pretty.c
|
|
index d55b88607a..c6c757c0ce 100644
|
|
--- a/pretty.c
|
|
+++ b/pretty.c
|
|
@@ -1597,9 +1597,21 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
|
|
default:
|
|
break;
|
|
}
|
|
- if (magic != NO_MAGIC)
|
|
+ if (magic != NO_MAGIC) {
|
|
placeholder++;
|
|
|
|
+ switch (placeholder[0]) {
|
|
+ case 'w':
|
|
+ /*
|
|
+ * `%+w()` cannot ever expand to a non-empty string,
|
|
+ * and it potentially changes the layout of preceding
|
|
+ * contents. We're thus not able to handle the magic in
|
|
+ * this combination and refuse the pattern.
|
|
+ */
|
|
+ return 0;
|
|
+ };
|
|
+ }
|
|
+
|
|
orig_len = sb->len;
|
|
if (((struct format_commit_context *)context)->flush_type != no_flush)
|
|
consumed = format_and_pad_commit(sb, placeholder, context);
|
|
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
|
|
index cdde37d325..1d768f7244 100755
|
|
--- a/t/t4205-log-pretty-formats.sh
|
|
+++ b/t/t4205-log-pretty-formats.sh
|
|
@@ -879,6 +879,14 @@ test_expect_success 'log --pretty with invalid padding format' '
|
|
test_cmp expect actual
|
|
'
|
|
|
|
+test_expect_success 'log --pretty with magical wrapping directives' '
|
|
+ commit_id=$(git commit-tree HEAD^{tree} -m "describe me") &&
|
|
+ git tag describe-me $commit_id &&
|
|
+ printf "\n(tag:\ndescribe-me)%%+w(2)" >expect &&
|
|
+ git log -1 --pretty="format:%w(1)%+d%+w(2)" $commit_id >actual &&
|
|
+ test_cmp expect actual
|
|
+'
|
|
+
|
|
test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' '
|
|
# We only assert that this command does not crash. This needs to be
|
|
# executed with the address sanitizer to demonstrate failure.
|
|
--
|
|
2.27.0
|
|
|