git-rebase: reword squashes commits in case of merge-conflicts

After the rebase the 'stuff' branch only has a single commit even though I'd expect there to be two according to the instructions that were passed to git-rebase. It works as expected if there's either no merge-conflict at the reword or if the conflicting commit is applied as 'pick'.

I'm running git version 2.17.1.windows.2. I also tried native git version 2.7.4 via WSL (running Ubuntu 16.04.4 LTS) and this version does not exhibit this behavior.

Hi all!

During a recent rebase operation on one of my repositories a number of commits unexpectedly ended up getting squashed into other commits. After some experiments it turned out that the 'reword' instruction seems to squash the referenced commit into the preceding commit if there's a merge-conflict.

Here's a small reproduction recipe to illustrate the behavior:

1. Create a small test repository using the following Bash script:

----
function add_file()

{
     echo "$1" > "$2"
     git add "$2"
     git commit -m "$3"
}

git init .

add_file "test" "file-1" "master"

git checkout -b stuff

add_file "aaa" "file-2" "feature_a"
add_file "bbb" "file-2" "fixup! feature_a"
add_file "ccc" "file-2" "fixup! feature_a"

add_file "ddd" "file-2" "feature_b"
add_file "eee" "file-2" "fixup! feature_b"
add_file "fff" "file-2" "fixup! feature_b"
----

2. Run

        $ git rebase --autosquash --interactive --onto master master stuff

to interactively rebase 'stuff' onto 'master'. This should generate the following todo-stream:

----
pick ... feature_a
fixup ... fixup! feature_a
fixup <hash_1> fixup! feature_a
pick <hash_2> feature_b
fixup ... fixup! feature_b
fixup ... fixup! feature_b
----

3. Remove the fixup line right before the second pick (i.e. 'fixup <hash_1>') in order to enforce a merge-conflict later when applying commit <hash_2>.

4. Replace the second pick instruction (i.e. 'pick <hash_2>') with a reword.

5. Launch the rebase operation. It should fail at the 'reword <hash_2>' instruction due to a merge-conflict.

6. Resolve the conflict by taking the remote-side (i.e. 'ddd'), add the change to the index with git-add and run

$ git rebase --continue

This should spawn the commit message editor and after saving and quitting the rebase should finish cleanly.

After the rebase the 'stuff' branch only has a single commit even though I'd expect there to be two according to the instructions that were passed to git-rebase. It works as expected if there's either no merge-conflict at the reword or if the conflicting commit is applied as 'pick'.

I'm running git version 2.17.1.windows.2. I also tried native git version 2.7.4 via WSL (running Ubuntu 16.04.4 LTS) and this version does not exhibit this behavior.

- ch

Thanks for a thorough report. I couldn't reproduce it on v2.17.1 on Linux, which makes me wonder if the issue is related to git-for-windows somehow. To the best of my knowledge (and a quick scan of "git diff" results) the code should be the same, though.

After the rebase the 'stuff' branch only has a single commit even though I'd expect there to be two according to the instructions that were passed to git-rebase.

During a recent rebase operation on one of my repositories a number of commits unexpectedly ended up getting squashed into other commits. After some experiments it turned out that the 'reword' instruction seems to squash the referenced commit into the preceding commit if there's a merge-conflict.

Here's a small reproduction recipe to illustrate the behavior:

t/t3423-rebase-reword.sh +7 -0

+       git checkout --theirs file-2 &&
+       git add file-2 &&
+       FAKE_COMMIT_MESSAGE="feature_b_reworded" git rebase --continue &&
+
+       test "$(git log -1 --format=%B)" = "feature_b_reworded" &&
+       test $(git rev-list --count HEAD) = 2
+'

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin", 2017-02-09), when a commit marked as 'reword' in an interactive rebase has conflicts and fails to apply, when the rebase is resumed that commit will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which introduced error_with_patch() for the edit command. For the edit command, it needs to stop the rebase whether or not the patch applies cleanly. If the patch does apply cleanly, then when it resumes it knows it needs to amend all changes into the previous commit. If it does not apply cleanly, then the changes should not be amended. Thus, it passes !res (success of applying the 'edit' commit) to error_with_patch() for the to_amend flag.

The problematic line of code actually came from commit 04efc8b57c ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02). Note that to get to this point in the code:
* !!res (i.e. patch application failed)
* item->command < TODO_SQUASH
* item->command != TODO_EDIT
* !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a pick, revert, or reword. For any of those cases we want a new commit, so we should not set the to_amend flag.

Signed-off-by Elijah Newren <[email protected]>

---
Differences since v1 (Thanks to Eric Sunshine for the suggestions):
* Add test_when_finished "reset_rebase" calls
* Remove unnecessary word from description of test

  sequencer.c              |  2 +-
  t/t3423-rebase-reword.sh | 48 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 49 insertions(+), 1 deletion(-)

create mode 100755 t/t3423-rebase-reword.sh

sequencer.c +1 -1

index cca968043e..9e6d1ee368 100644
@@ -3217,7 +3217,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
  			} else if (res && is_rebase_i(opts) && item->commit)
  				return res | error_with_patch(item->commit,
  					item->arg, item->arg_len, opts, res,
-					item->command == TODO_REWORD);
+					0);
  		} else if (item->command == TODO_EXEC) {
  			char *end_of_arg = (char *)(item->arg + item->arg_len);
  			int saved = *end_of_arg;

t/t3423-rebase-reword.sh new file +48 -0

new file mode 100755
index 0000000000..6963750794
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='git rebase interactive with rewording'
+
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup' '
+	test_commit master file-1 test &&
+
+	git checkout -b stuff &&
+
+	test_commit feature_a file-2 aaa &&
+	test_commit feature_b file-2 ddd
+'
+
+test_expect_success 'reword without issues functions as intended' '
+	test_when_finished "reset_rebase" &&
+
+	git checkout stuff^0 &&
+
+	set_fake_editor &&
+	FAKE_LINES="pick 1 reword 2" FAKE_COMMIT_MESSAGE="feature_b_reworded" \
+		git rebase -i -v master &&
+
+	test "$(git log -1 --format=%B)" = "feature_b_reworded" &&
+	test $(git rev-list --count HEAD) = 3
+'
+
+test_expect_success 'reword after a conflict preserves commit' '
+	test_when_finished "reset_rebase" &&
+
+	git checkout stuff^0 &&
+
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="reword 2" \
+		git rebase -i -v master &&
+
+	git checkout --theirs file-2 &&
+	git add file-2 &&
+	FAKE_COMMIT_MESSAGE="feature_b_reworded" git rebase --continue &&
+
+	test "$(git log -1 --format=%B)" = "feature_b_reworded" &&
+	test $(git rev-list --count HEAD) = 2
+'
+
+test_done

Quoted 14 lines · Elijah Newren

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin", 2017-02-09), when a commit marked as 'reword' in an interactive rebase has conflicts and fails to apply, when the rebase is resumed that commit will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which introduced error_with_patch() for the edit command. For the edit command, it needs to stop the rebase whether or not the patch applies cleanly. If the patch does apply cleanly, then when it resumes it knows it needs to amend all changes into the previous commit. If it does not apply cleanly, then the changes should not be amended. Thus, it passes !res (success of applying the 'edit' commit) to error_with_patch() for the to_amend flag.

Quoted 10 lines

The problematic line of code actually came from commit 04efc8b57c ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02). Note that to get to this point in the code:
* !!res (i.e. patch application failed)
* item->command < TODO_SQUASH
* item->command != TODO_EDIT
* !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a pick, revert, or reword. For any of those cases we want a new commit, so we should not set the to_amend flag.

Unfortunately I'm not sure it's that simple. Looking and do_pick() sometimes reword amends HEAD and sometimes it does not. In the "normal" case then the commit is picked and committed with '--edit'. However when fast-forwarding the code fast forwards to the commit to be reworded and then amends it. If the root commit is being reworded it takes the same code path. While these cases cannot fail with conflicts, it is possible for the user to cancel the commit or for them to fail due to collisions with untracked files.

If I remember correctly the shell version always picks the commit and then calls 'git commit --amend' afterwards which is less efficient but consistent.

I'm afraid I don't have a simple solution for fixing this, as currently pick_commits() does not know if the commit was called with AMEND_MSG, I guess that means adding some kind of flag for do_pick() to set.

Hi Phillip,

Quoted 37 lines · Phillip Wood

Quoted 14 lines · Elijah Newren

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin", 2017-02-09), when a commit marked as 'reword' in an interactive rebase has conflicts and fails to apply, when the rebase is resumed that commit will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which introduced error_with_patch() for the edit command. For the edit command, it needs to stop the rebase whether or not the patch applies cleanly. If the patch does apply cleanly, then when it resumes it knows it needs to amend all changes into the previous commit. If it does not apply cleanly, then the changes should not be amended. Thus, it passes !res (success of applying the 'edit' commit) to error_with_patch() for the to_amend flag.

Quoted 10 lines

The problematic line of code actually came from commit 04efc8b57c ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02). Note that to get to this point in the code:
* !!res (i.e. patch application failed)
* item->command < TODO_SQUASH
* item->command != TODO_EDIT
* !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a pick, revert, or reword. For any of those cases we want a new commit, so we should not set the to_amend flag.

Unfortunately I'm not sure it's that simple. Looking and do_pick() sometimes reword amends HEAD and sometimes it does not. In the "normal" case then the commit is picked and committed with '--edit'. However when fast-forwarding the code fast forwards to the commit to be reworded and then amends it. If the root commit is being reworded it takes the same code path. While these cases cannot fail with conflicts, it is possible for the user to cancel the commit or for them to fail due to collisions with untracked files.

If I remember correctly the shell version always picks the commit and then calls 'git commit --amend' afterwards which is less efficient but consistent.

Quoted 3 lines

I'm afraid I don't have a simple solution for fixing this, as currently pick_commits() does not know if the commit was called with AMEND_MSG, I guess that means adding some kind of flag for do_pick() to set.

Oh, you're right, the fast-forwarding path would pose a problem. I think there is an easy way to resolve this, though: in the case that we do want to amend the to-be-reworded commit, we simply have to see whether HEAD points to the very same commit mentioned in the reword command:

-- snip --

sequencer.c +14 -2

index 2dad7041960..99d33d4e063 100644
@@ -3691,10 +3691,22 @@ static int pick_commits(struct todo_list
 *todo_list, struct replay_opts *opts)
                                        intend_to_amend();
                                return error_failed_squash(item->commit, opts,
                                        item->arg_len, item->arg);
-                       } else if (res && is_rebase_i(opts) && item->commit)
+                       } else if (res && is_rebase_i(opts) && item->commit) {
+                               int to_amend = 0;
+
+                               if (item->command == TODO_REWORD) {
+                                       struct object_id head;
+
+                                       if (!get_oid("HEAD", &head) &&
+					    !oidcmp(&item->commit->object.oid,
+                                                   &head))
+                                               to_amend = 1;
+                               }
+
                                return res | error_with_patch(item->commit,
                                        item->arg, item->arg_len, opts, res,
-                                       item->command == TODO_REWORD);
+                                       to_amend);
+                       }
                } else if (item->command == TODO_EXEC) {
                        char *end_of_arg = (char *)(item->arg + item->arg_len);

                        int saved = *end_of_arg;

-- snap --

Note that

- this patch is only compile-tested, and

- it is on top of my sequencer-shears branch thicket, so it might not apply cleanly to master, and

- it could probably use a comment what we are doing here (see whether we wanted to amend a fast-forwarded commit).

What do you think about this approach?

Dscho

Quoted 4 lines

Oh, you're right, the fast-forwarding path would pose a problem. I think there is an easy way to resolve this, though: in the case that we do want to amend the to-be-reworded commit, we simply have to see whether HEAD points to the very same commit mentioned in the reword command:

That's clever, I think to get it to work for rewording the root commit, it will need to do something like comparing HEAD to squash-onto as well.

Quoted 49 lines · Johannes Schindelin

Quoted 45 lines · Phillip Wood

Quoted 16 lines · Elijah Newren

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin", 2017-02-09), when a commit marked as 'reword' in an interactive rebase has conflicts and fails to apply, when the rebase is resumed that commit will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which introduced error_with_patch() for the edit command. For the edit command, it needs to stop the rebase whether or not the patch applies cleanly. If the patch does apply cleanly, then when it resumes it knows it needs to amend all changes into the previous commit. If it does not apply cleanly, then the changes should not be amended. Thus, it passes !res (success of applying the 'edit' commit) to error_with_patch() for the to_amend flag.

Quoted 10 lines

The problematic line of code actually came from commit 04efc8b57c ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02). Note that to get to this point in the code:
* !!res (i.e. patch application failed)
* item->command < TODO_SQUASH
* item->command != TODO_EDIT
* !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a pick, revert, or reword. For any of those cases we want a new commit, so we should not set the to_amend flag.

Unfortunately I'm not sure it's that simple. Looking and do_pick() sometimes reword amends HEAD and sometimes it does not. In the "normal" case then the commit is picked and committed with '--edit'. However when fast-forwarding the code fast forwards to the commit to be reworded and then amends it. If the root commit is being reworded it takes the same code path. While these cases cannot fail with conflicts, it is possible for the user to cancel the commit or for them to fail due to collisions with untracked files.

If I remember correctly the shell version always picks the commit and then calls 'git commit --amend' afterwards which is less efficient but consistent.

I'm afraid I don't have a simple solution for fixing this, as currently pick_commits() does not know if the commit was called with AMEND_MSG, I guess that means adding some kind of flag for do_pick() to set.

Quoted 5 lines

Oh, you're right, the fast-forwarding path would pose a problem. I think there is an easy way to resolve this, though: in the case that we do want to amend the to-be-reworded commit, we simply have to see whether HEAD points to the very same commit mentioned in the reword command:

That's clever, I think to get it to work for rewording the root commit, it will need to do something like comparing HEAD to squash-onto as well.

Quoted 13 lines

-- snip --

sequencer.c +2 -1

index 2dad7041960..99d33d4e063 100644
@@ -3691,10 +3691,22 @@ static int pick_commits(struct todo_list
 *todo_list, struct replay_opts *opts)
                                        intend_to_amend();
                                return error_failed_squash(item->commit, opts,
                                        item->arg_len, item->arg);
-                       } else if (res && is_rebase_i(opts) && item->commit)
+                       } else if (res && is_rebase_i(opts) && item->commit) {
+                               int to_amend = 0;

sequencer.c static int pick_commits(struct todo_list · line 3704

Quoted 8 lines

sequencer.c +8 -0

+
+                               if (item->command == TODO_REWORD) {
+                                       struct object_id head;
+
+                                       if (!get_oid("HEAD", &head) &&
+					    !oidcmp(&item->commit->object.oid,
+                                                   &head))
+                                               to_amend = 1;

Hi Phillip,

Quoted 50 lines · Phillip Wood

Quoted 49 lines · Johannes Schindelin

Quoted 45 lines · Phillip Wood

Quoted 16 lines · Elijah Newren

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin", 2017-02-09), when a commit marked as 'reword' in an interactive rebase has conflicts and fails to apply, when the rebase is resumed that commit will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which introduced error_with_patch() for the edit command. For the edit command, it needs to stop the rebase whether or not the patch applies cleanly. If the patch does apply cleanly, then when it resumes it knows it needs to amend all changes into the previous commit. If it does not apply cleanly, then the changes should not be amended. Thus, it passes !res (success of applying the 'edit' commit) to error_with_patch() for the to_amend flag.

Quoted 10 lines

The problematic line of code actually came from commit 04efc8b57c ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02). Note that to get to this point in the code:
* !!res (i.e. patch application failed)
* item->command < TODO_SQUASH
* item->command != TODO_EDIT
* !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a pick, revert, or reword. For any of those cases we want a new commit, so we should not set the to_amend flag.

Unfortunately I'm not sure it's that simple. Looking and do_pick() sometimes reword amends HEAD and sometimes it does not. In the "normal" case then the commit is picked and committed with '--edit'. However when fast-forwarding the code fast forwards to the commit to be reworded and then amends it. If the root commit is being reworded it takes the same code path. While these cases cannot fail with conflicts, it is possible for the user to cancel the commit or for them to fail due to collisions with untracked files.

If I remember correctly the shell version always picks the commit and then calls 'git commit --amend' afterwards which is less efficient but consistent.

I'm afraid I don't have a simple solution for fixing this, as currently pick_commits() does not know if the commit was called with AMEND_MSG, I guess that means adding some kind of flag for do_pick() to set.

Quoted 8 lines

Quoted 5 lines

Oh, you're right, the fast-forwarding path would pose a problem. I think there is an easy way to resolve this, though: in the case that we do want to amend the to-be-reworded commit, we simply have to see whether HEAD points to the very same commit mentioned in the reword command:

That's clever, I think to get it to work for rewording the root commit, it will need to do something like comparing HEAD to squash-onto as well.

.... because squash-onto is a fresh, empty root commit (to be "amended" when a non-root commit is to be picked as a new root commit). Good point.

Quoted 13 lines

Quoted 13 lines

-- snip --

sequencer.c +2 -1

index 2dad7041960..99d33d4e063 100644
@@ -3691,10 +3691,22 @@ static int pick_commits(struct todo_list
 *todo_list, struct replay_opts *opts)
                                        intend_to_amend();
                                return error_failed_squash(item->commit, opts,
                                        item->arg_len, item->arg);
-                       } else if (res && is_rebase_i(opts) && item->commit)
+                       } else if (res && is_rebase_i(opts) && item->commit) {
+                               int to_amend = 0;

Quoted 8 lines

sequencer.c static int pick_commits(struct todo_list · line 3704

Quoted 8 lines

sequencer.c +8 -0

+
+                               if (item->command == TODO_REWORD) {
+                                       struct object_id head;
+
+                                       if (!get_oid("HEAD", &head) &&
+					    !oidcmp(&item->commit->object.oid,
+                                                   &head))
+                                               to_amend = 1;

This would now become

					if (!get_oid("HEAD", &head) &&
					    (!oidcmp(&item->commit->object.oid,
						     &head) ||
					     (opts->have_squash_onto &&
					      !oidcmp(&opts->squash_onto,
						      &head))))
						to_amend = 1;

This is awfully indented, so a better idea would probably be to avoid the extra block just to declare head:

(quoted diff) +19 -2

-                       } else if (res && is_rebase_i(opts) && item->commit)
+                       } else if (res && is_rebase_i(opts) && item->commit) {
+                               int to_amend = 0;
+                               struct object_id oid;
+
+				/*
+				 * If we fast-forwarded already, or if we
+				 * are about to create a new root commit,
+				 * we definitely want to amend, otherwise
+				 * we do not.
+				 */
+                               if (item->command == TODO_REWORD &&
+				    !get_oid("HEAD", &oid) &&
+				    (!oidcmp(&item->commit->object.oid, &oid) ||
+				     (opts->have_squash_onto &&
+				      !oidcmp(&opts->squash_onto, &head))))
+					to_amend = 1;
+
                                return res | error_with_patch(item->commit,
                                        item->arg, item->arg_len, opts, res,
-                                       item->command == TODO_REWORD);
+                                       to_amend);
+                       }
                } else if (item->command == TODO_EXEC) {

What do you think?

And: could you perchance add a regression test for a failing pick onto squash-onto (I am desperately in need of sleeping, otherwise I would do it myself)? Something that executes reset [new root] and then pick abcdef where abcdef would overwrite an untracked file should trigger this relatively easily.

Thanks,
Dscho

From: Phillip Wood <[email protected]>

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin", 2017-02-09), when a commit marked as 'reword' in an interactive rebase has conflicts and fails to apply, when the rebase is resumed that commit will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which introduced error_with_patch() for the edit command. For the edit command, it needs to stop the rebase whether or not the patch applies cleanly. If the patch does apply cleanly, then when it resumes it knows it needs to amend all changes into the previous commit. If it does not apply cleanly, then the changes should not be amended. Thus, it passes !res (success of applying the 'edit' commit) to error_with_patch() for the to_amend flag.

The problematic line of code actually came from commit 04efc8b57c ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02). Note that to get to this point in the code:
* !!res (i.e. patch application failed)
* item->command < TODO_SQUASH
* item->command != TODO_EDIT
* !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a pick, revert, or reword. We only need to amend HEAD when rewording the root commit or a commit that has been fast-forwarded, for any of the other cases we want a new commit, so we should not set the to_amend flag.

Helped-by Johannes Schindelin <[email protected]>

Original-patch-by Elijah Newren <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---

I wasn't really sure what to do about the authorship. This is Elijah's patch, with the message tweaked, fixed up with a corrected version of Johannes' code and a couple of new tests by my for picking and rewording the root commit when it has untracked file confilcts.

 sequencer.c                   | 23 ++++++++++++++---
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++++++++
 t/t3423-rebase-reword.sh      | 48 +++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 3 deletions(-)
 create mode 100755 t/t3423-rebase-reword.sh

sequencer.c +20 -3

index 4034c0461b..7bf2b62727 100644
@@ -3214,10 +3214,27 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
                                        intend_to_amend();
                                return error_failed_squash(item->commit, opts,
                                        item->arg_len, item->arg);
-                       } else if (res && is_rebase_i(opts) && item->commit)
+                       } else if (res && is_rebase_i(opts) && item->commit) {
+                               int to_amend = 0;
+                               struct object_id oid;
+
+                               /*
+                                * If we are rewording and have either
+                                * fast-forwarded already, or are about to
+                                * create a new root commit, we want to amend,
+                                * otherwise we do not.
+                                */
+                               if (item->command == TODO_REWORD &&
+                                   !get_oid("HEAD", &oid) &&
+                                   (!oidcmp(&item->commit->object.oid, &oid) ||
+                                    (opts->have_squash_onto &&
+                                     !oidcmp(&opts->squash_onto, &oid))))
+                                       to_amend = 1;
+
                                return res | error_with_patch(item->commit,
-                                       item->arg, item->arg_len, opts, res,
-                                       item->command == TODO_REWORD);
+                                               item->arg, item->arg_len, opts,
+                                               res, to_amend);
+                       }
                } else if (item->command == TODO_EXEC) {
                        char *end_of_arg = (char *)(item->arg + item->arg_len);
                        int saved = *end_of_arg;

t/t3404-rebase-interactive.sh +28 -0

index e500d7c320..3786879e80 100755
@@ -980,7 +980,35 @@ test_expect_success 'rebase -i --root reword root commit' '
        git show HEAD^ | grep "A changed"
 '
 
+test_expect_success 'rebase -i --root when root has untracked file confilct' '
+       test_when_finished "reset_rebase" &&
+       git checkout -b failing-root-pick A &&
+       echo x >file2 &&
+       git rm file1 &&
+       git commit -m "remove file 1 add file 2" &&
+       echo z >file1 &&
+       set_fake_editor &&
+       test_must_fail env FAKE_LINES="1 2" git rebase -i --root &&
+       rm file1 &&
+       git rebase --continue &&
+       test "$(git log -1 --format=%B)" = "remove file 1 add file 2" &&
+       test "$(git rev-list --count HEAD)" = 2
+'
+
+test_expect_success 'rebase -i --root reword root when root has untracked file conflict' '
+       test_when_finished "reset_rebase" &&
+       echo z>file1 &&
+       set_fake_editor &&
+       test_must_fail env FAKE_LINES="reword 1 2" \
+               FAKE_COMMIT_MESSAGE="Modified A" git rebase -i --root &&
+       rm file1 &&
+       FAKE_COMMIT_MESSAGE="Reworded A" git rebase --continue &&
+       test "$(git log -1 --format=%B HEAD^)" = "Reworded A" &&
+       test "$(git rev-list --count HEAD)" = 2
+'
+
 test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' '
+       git checkout reword-root-branch &&
        git reset --hard &&
        git checkout conflict-branch &&
        set_fake_editor &&

t/t3423-rebase-reword.sh new file +48 -1

new file mode 100755
index 0000000000..6963750794
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='git rebase interactive with rewording'
+
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup' '
+       test_commit master file-1 test &&
+
+       git checkout -b stuff &&
+
+       test_commit feature_a file-2 aaa &&
+       test_commit feature_b file-2 ddd
+'
+
+test_expect_success 'reword without issues functions as intended' '
+       test_when_finished "reset_rebase" &&
+
+       git checkout stuff^0 &&
+
+       set_fake_editor &&
+       FAKE_LINES="pick 1 reword 2" FAKE_COMMIT_MESSAGE="feature_b_reworded" \
+               git rebase -i -v master &&
+
+       test "$(git log -1 --format=%B)" = "feature_b_reworded" &&
+       test $(git rev-list --count HEAD) = 3
+'
+
+test_expect_success 'reword after a conflict preserves commit' '
+       test_when_finished "reset_rebase" &&
+
+       git checkout stuff^0 &&
+
+       set_fake_editor &&
+       test_must_fail env FAKE_LINES="reword 2" \
+               git rebase -i -v master &&
+
+       git checkout --theirs file-2 &&
+       git add file-2 &&
+       FAKE_COMMIT_MESSAGE="feature_b_reworded" git rebase --continue &&
+
+       test "$(git log -1 --format=%B)" = "feature_b_reworded" &&
+       test $(git rev-list --count HEAD) = 2
+'
+
+test_done
--

2.17.1

[As an aside, I know there are multiple other outstanding emails for me to respond to, unrelated to this patch. I'll try to get some time in the next day or two to respond. Just responding to this one since Junio mentioned picking it up for 2.18.]

The verbose output of the test 'reword without issues functions as intended' in 't3423-rebase-reword.sh', added in a9279c6785 (sequencer:
do not squash 'reword' commits when we hit conflicts, 2018-06-19), contains the following error output:

sed: -e expression #1, char 2: extra characters after command

This error comes from within the 'fake-editor.sh' script created by 'lib-rebase.sh's set_fake_editor() function, and the root cause is the FAKE_LINES="pick 1 reword 2" variable in the test in question, in particular the "pick" word. 'fake-editor.sh' assumes 'pick' to be the default rebase command and doesn't support an explicit 'pick' command in FAKE_LINES. As a result, 'pick' will be used instead of a line number when assembling the following 'sed' script:

sed -n picks/^pick/pick/p

which triggers the aforementioned error.

Luckily, this didn't affect the test's correctness: the erroring 'sed' command doesn't write anything to the todo script, and processing the rest of FAKE_LINES generates the desired todo script, as if that 'pick' command were not there at all.

The minimal fix would be to remove the 'pick' word from FAKE_LINES, but that would leave us susceptible to similar issues in the future.

Instead, teach the fake-editor script to recognize an explicit 'pick' command, which is still a fairly trivial change.

In the future we might want to consider reinforcing this fake editor script with an &&-chain and stricter parsing of the FAKE_LINES variable (e.g. to error out when encountering unknown rebase commands or commands and line numbers in the wrong order).

Signed-off-by SZEDER Gábor <[email protected]>

---
 t/lib-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/lib-rebase.sh +1 -1

index 25a77ee5cb..592865d019 100644
@@ -47,7 +47,7 @@ set_fake_editor () {
 	action=pick
 	for line in $FAKE_LINES; do
 		case $line in
-		squash|fixup|edit|reword|drop)
+		pick|squash|fixup|edit|reword|drop)
 			action="$line";;
 		exec*)
 			echo "$line" | sed 's/_/ /g' >> "$1";;

Signature · 1 line

2.19.0.rc0.136.gd2dd172e64

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin", 2017-02-09), when a commit marked as 'reword' in an interactive rebase has conflicts and fails to apply, when the rebase is resumed that commit will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which introduced error_with_patch() for the edit command. For the edit command, it needs to stop the rebase whether or not the patch applies cleanly. If the patch does apply cleanly, then when it resumes it knows it needs to amend all changes into the previous commit. If it does not apply cleanly, then the changes should not be amended. Thus, it passes !res (success of applying the 'edit' commit) to error_with_patch() for the to_amend flag.

The problematic line of code actually came from commit 04efc8b57c ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02). Note that to get to this point in the code:
* !!res (i.e. patch application failed)
* item->command < TODO_SQUASH
* item->command != TODO_EDIT
* !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a pick, revert, or reword. For any of those cases we want a new commit, so we should not set the to_amend flag.

Signed-off-by Elijah Newren <[email protected]>

---
Differences since v1 (Thanks to Eric Sunshine for the suggestions):
* Add test_when_finished "reset_rebase" calls
* Remove unnecessary word from description of test

 sequencer.c              |  2 +-
 t/t3423-rebase-reword.sh | 48 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100755 t/t3423-rebase-reword.sh

sequencer.c +0 -0

index cca968043e..9e6d1ee368 100644