From: Phillip Wood <phillip.wood@dunelm.org.uk>
When the sequencer processes a chain of "fixup" and "squash" commands it keeps a list of the commands that have been executed. If there are conflicts, then the list is saved when the rebase stops for the user to resolve them. When the rebase resumes, the list is loaded and is used to initialize the count of how many "fixup" and "squash" commands have been processed; if a command has been skipped with "git rebase --skip", then the last command needs to be popped off the end of the list.
To count the number of commands, commit_staged_changes() uses the number of newlines in the file plus one. This is due to the slightly unusual way the list is constructed - instead of appending a newline when a command is added, a newline is inserted before the command if the current count is greater than zero. Therefore, when we pop a skipped command off the list, we should also remove the newline that precedes it. Otherwise, when a new command is added, a blank line will be left before it, which will contribute to the fixup count the next time the file is read. Unfortunately, the preceding newline is not removed, leading to an incorrect count. Fix this by removing the newline that appears before the skipped command.
In addition to fixing the code that removes a skipped command from the list, the code that reads the list is fixed to skip blank lines. We have had reports of users starting a rebase with one version of git and continuing it with another. Often this happens because the version of git bundled with an IDE or TUI differs from the one used at the command line. By fixing both the reading and writing ends of the problem we ensure the count is correct when an older version of git reads the fixup file written by a newer version and vice versa.
Triggering the incorrect count requires the user to skip two "fixup" or "squash" commands before the final command in the chain. An existing test is extended to prevent future regressions. The consequence of miscounting is not serious: we just print the wrong count in the header of the commit message template.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 11 ++++++++++-
t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++++++++++++----
2 files changed, 42 insertions(+), 5 deletions(-)
Show changes to 2 files +42 −5
sequencer.c, t/t3418-rebase-continue.sh
diff --git a/sequencer.c b/sequencer.c
index 1355a99a092..af3d2c72616 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3281,7 +3281,13 @@ static int read_populate_opts(struct replay_opts *opts)
const char *p = ctx->current_fixups.buf;
ctx->current_fixup_count = 1;
while ((p = strchr(p, '\n'))) {
- ctx->current_fixup_count++;
+ /*
+ * Older versions of git accidentally
+ * inserted blank lines when a fixup
+ * was skipped.
+ */
+ if (p[1] != '\n')
+ ctx->current_fixup_count++;
p++;
}
}
@@ -5353,6 +5359,9 @@ static int commit_staged_changes(struct repository *r,
if (!len)
BUG("Incorrect current_fixups:\n%s", p);
while (len && p[len - 1] != '\n')
+ len--;
+ /* Remove trailing newline */
+ if (len)
len--;
strbuf_setlen(&ctx->current_fixups, len);
if (write_message(p, len, rebase_path_current_fixups(),
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f9b8999db50..3c248e97364 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -134,6 +134,7 @@ test_expect_success '--skip after failed fixup cleans commit message' '
EOF
: skip and continue &&
+ test_config commit.status false &&
echo "cp \"\$1\" .git/copy.txt" | write_script copy-editor.sh &&
(test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) &&
@@ -145,7 +146,8 @@ test_expect_success '--skip after failed fixup cleans commit message' '
: now, let us ensure that "squash" is handled correctly &&
git reset --hard wants-fixup-3 &&
- test_must_fail env FAKE_LINES="1 squash 2 squash 1 squash 3 squash 1" \
+ test_must_fail env \
+ FAKE_LINES="1 squash 2 squash 1 squash 3 squash 1 squash 4 squash 1" \
git rebase -i HEAD~4 &&
: the second squash failed, but there are two more in the chain &&
@@ -171,19 +173,45 @@ test_expect_success '--skip after failed fixup cleans commit message' '
fixup 2
EOF
+ (test_set_editor "$PWD/copy-editor.sh" &&
+ test_must_fail git rebase --skip) &&
+ : not the final squash, no need to edit the commit message &&
+ test_path_is_missing .git/copy.txt &&
+
+ : The first, third and fifth squashes succeeded, therefore: &&
+ cat >expect <<-\EOF &&
+ # This is a combination of 4 commits.
+ # This is the 1st commit message:
+
+ wants-fixup
+
+ # This is the commit message #2:
+
+ fixup 1
+
+ # This is the commit message #3:
+
+ fixup 2
+
+ # This is the commit message #4:
+
+ fixup 3
+ EOF
+ test_commit_message HEAD expect &&
+
(test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) &&
test_commit_message HEAD <<-\EOF &&
wants-fixup
fixup 1
fixup 2
+
+ fixup 3
EOF
: Final squash failed, but there was still a squash &&
- head -n1 .git/copy.txt >first-line &&
- test_grep "# This is a combination of 3 commits" first-line &&
- test_grep "# This is the commit message #3:" .git/copy.txt
+ test_cmp expect .git/copy.txt
'
test_expect_success 'setup rerere database' '