1
0
Fork 0
mirror of https://github.com/git/git.git synced 2024-06-01 19:26:10 +02:00

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>
This commit is contained in:
Patrick Steinhardt 2022-12-01 15:46:39 +01:00 committed by Junio C Hamano
parent f6e0b9f389
commit 1de69c0cdd
2 changed files with 21 additions and 1 deletions

View File

@ -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);

View File

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