rebase -i: fix counting of fixups after rebase --skip

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/sequencer.c b/sequencer.c
index 1355a99a0922681a4443b03df7387974009d1fee..4640ee9b7f536b7e3bfb37b7c82ecff3307759b4 100644 (file)
--- 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] && p[1] != '\n')
+                                       ctx->current_fixup_count++;
                                p++;
                        }
                }
@@ -5354,6 +5360,9 @@ static int commit_staged_changes(struct repository *r,
                                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(),
                                          0) < 0) {

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f9b8999db50f1b77f03ec51cec01ace8849a9137..3c248e973649ea20c38d1a7548bbe78b077e107c 100755 (executable)
--- 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,6 +173,32 @@ 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
@@ -178,12 +206,12 @@ test_expect_success '--skip after failed fixup cleans commit message' '
        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' '