On Mon, Feb 19, 2024 at 10:42:45AM -0800, Junio C Hamano wrote:
> Thanks, both, for finding a rather unfortunate regression. Perhaps
> it is worth delaying 2.44 final by a week or so to include a fix (or
> a revert if it comes to it).
Hmm, I had thought this was pre-2.44, but it was actually in the 2.43.x
maintenance series (so it is not a regression going from 2.43.2 to
2.44.0, but it is from 2.43.0 to 2.44.0).
Anyway, the fix is pretty simple, so I think it may be OK to apply it
for 2.44.0-rc2. Here it is (prepared on top of la/trailer-cleanups, so
it could also be merged down for a v2.43.3 if you want to).
-- >8 --
Subject: trailer: fix comment/cut-line regression with opts->no_divider
Commit 97e9d0b78a (trailer: find the end of the log message, 2023-10-20)
combined two code paths for finding the end of the log message. For the
"no_divider" case, we used to use find_trailer_end(), and that has now
been rolled into find_end_of_log_message(). But there's a regression;
that function returns early when no_divider is set, returning the whole
string.
That's not how find_trailer_end() behaved. Although it did skip the
"---" processing (which is what "no_divider" is meant to do), we should
still respect ignored_log_message_bytes(), which covers things like
comments, "commit -v" cut lines, and so on.
The bug is actually in the interpret-trailers command, but the obvious
way to experience it is by running "commit -v" with a "--trailer"
option. The new trailer will be added at the end of the verbose diff,
rather than before it (and consequently will be ignored entirely, since
everything after the diff's intro scissors line is thrown away).
I've added two tests here: one for interpret-trailers directly, which
shows the bug via the parsing routines, and one for "commit -v".
The fix itself is pretty simple: instead of returning early, no_divider
just skips the "---" handling but still calls ignored_log_message_bytes().
Reported-by: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
Viewing the diff with "-w" makes the change a little more obvious.
t/t7502-commit-porcelain.sh | 18 ++++++++++++++++++
t/t7513-interpret-trailers.sh | 19 +++++++++++++++++++
trailer.c | 15 +++++++--------
3 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b5bf7de7cd..d8e216613f 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '
test_cmp expected actual
'
+test_expect_success 'commit --trailer with --verbose' '
+ cat >msg <<-\EOF &&
+ subject
+
+ body
+ EOF
+ GIT_EDITOR=: git commit --edit -F msg --allow-empty \
+ --trailer="my-trailer: value" --verbose &&
+ {
+ cat msg &&
+ echo &&
+ echo "my-trailer: value"
+ } >expected &&
+ git cat-file commit HEAD >commit.msg &&
+ sed -e "1,/^\$/d" commit.msg >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'multiple -m' '
>negative &&
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 97f10905d2..3343ad0eb6 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1560,4 +1560,23 @@ test_expect_success 'suppress --- handling' '
test_cmp expected actual
'
+test_expect_success 'suppressing --- does not disable cut-line handling' '
+ echo "real-trailer: before the cut" >expected &&
+
+ git interpret-trailers --parse --no-divider >actual <<-\EOF &&
+ subject
+
+ This input has a cut-line in it; we should stop parsing when we see it
+ and consider only trailers before that line.
+
+ real-trailer: before the cut
+
+ # ------------------------ >8 ------------------------
+ # Nothing below this line counts as part of the commit message.
+ not-a-trailer: too late
+ EOF
+
+ test_cmp expected actual
+'
+
test_done
diff --git a/trailer.c b/trailer.c
index 816f8b28a9..009ee80dee 100644
--- a/trailer.c
+++ b/trailer.c
@@ -837,16 +837,15 @@ static size_t find_end_of_log_message(const char *input, int no_divider)
/* Assume the naive end of the input is already what we want. */
end = strlen(input);
- if (no_divider)
- return end;
-
/* Optionally skip over any patch part ("---" line and below). */
- for (s = input; *s; s = next_line(s)) {
- const char *v;
+ if (!no_divider) {
+ for (s = input; *s; s = next_line(s)) {
+ const char *v;
- if (skip_prefix(s, "---", &v) && isspace(*v)) {
- end = s - input;
- break;
+ if (skip_prefix(s, "---", &v) && isspace(*v)) {
+ end = s - input;
+ break;
+ }
}
}
--
2.44.0.rc1.413.gdc9df0ba3d
