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

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

[ch]

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

Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts

[Jeff King]

On Mon, Jun 11, 2018 at 06:06:11PM +0200, ch wrote:

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

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.

I'm cc-ing Johannes, who is both the resident interactive-rebase expert
_and_ the Git for Windows maintainer. Copious quoting below.

-Peff

-- >8 --
> 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

Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts

[ch]

On 12.06.2018 12:08, Jeff King wrote:

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

I gave native Git version 2.17.1 on Ubuntu 18.04 (no WSL this time) a try
and was able to reproduce the issue outlined in my first email.

In case there is a misunderstanding, when I wrote

On 11.06.2018 18:06, ch wrote:

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

I actually meant that there's only a single commit exclusively reachable
from 'stuff' after the rebase, i.e.

   $ git log stuff ^master

only lists a single commit while there actually should be two unless I'm
missing something.

Sorry for being a bit sloppy in my initial report.

- ch

Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts

[Elijah Newren]

On Mon, Jun 11, 2018 at 06:06:11PM +0200, ch wrote:

> 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:

Indeed I can reproduce.  It bisects back to when we switched to using the
rebase-helper builtin, so this started with git-2.13.0.  I can also slim down
your reproduction testcase a bit; you don't need any additional commits after
the reword, and the only reason to have commits before it is to induce a merge
conflict by dropping them.  The fact that there are lots of fixups floating
around is not necessary.

Anyway, patch below.

-- >8 --
Subject: [PATCH] sequencer: do not squash 'reword' commits when we hit
 conflicts

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.

Reported-by: ch <cr@onlinehome.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 sequencer.c              |  2 +-
 t/t3423-rebase-reword.sh | 44 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100755 t/t3423-rebase-reword.sh

diff --git a/sequencer.c b/sequencer.c
index cca968043e..9e6d1ee368 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
new file mode 100755
index 0000000000..31c09efd22
--- /dev/null
+++ b/t/t3423-rebase-reword.sh
@@ -0,0 +1,44 @@
+#!/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' '
+	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 comes after a conflict preserves commit' '
+	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.18.0.rc2.3.g7fc0d4cb57

Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts

[Eric Sunshine]

On Sat, Jun 16, 2018 at 12:08 PM Elijah Newren <newren@gmail.com> wrote:
> Subject: [PATCH] sequencer: do not squash 'reword' commits when we hit
>  conflicts
> [...]
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> @@ -0,0 +1,44 @@
> +test_expect_success 'reword comes after a conflict preserves commit' '

s/comes//

> +       git checkout stuff^0 &&
> +
> +       set_fake_editor &&
> +       test_must_fail env FAKE_LINES="reword 2" \
> +               git rebase -i -v master &&

If some command fails between here and "git rebase --continue" below,
then the test will exit with an in-progress (dangling) rebase, which
could confuse tests added later to this script. I wonder, therefore,
if it would make sense to insert the following cleanup before "git
rebase -i":

    test_when_finished "reset_rebase" &&

> +       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
> +'

[PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts

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

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 <newren@gmail.com>
---
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

diff --git a/sequencer.c b/sequencer.c
index cca968043e..9e6d1ee368 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
new file mode 100755
index 0000000000..6963750794
--- /dev/null
+++ b/t/t3423-rebase-reword.sh
@@ -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.18.0.rc2.1.g5453d3f70b.dirty

Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts

[Phillip Wood]

Hi Elijah,

On 17/06/18 06:37, Elijah Newren wrote:
> 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.

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.

Best Wishes

Phillip

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> 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
> 
> diff --git a/sequencer.c b/sequencer.c
> index cca968043e..9e6d1ee368 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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;
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> new file mode 100755
> index 0000000000..6963750794
> --- /dev/null
> +++ b/t/t3423-rebase-reword.sh
> @@ -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
> 

Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts

[Johannes Schindelin]

Hi Phillip,

On Sun, 17 Jun 2018, Phillip Wood wrote:

> On 17/06/18 06:37, Elijah Newren wrote:
> > 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.
> 
> 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.

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 --
diff --git a/sequencer.c b/sequencer.c
index 2dad7041960..99d33d4e063 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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

Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts

[Phillip Wood]

Hi Johannes

On 17/06/18 20:28, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Sun, 17 Jun 2018, Phillip Wood wrote:
> 
>> On 17/06/18 06:37, Elijah Newren wrote:
>>> 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.
>>
>> 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.
> 
> 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.

> -- snip --
> diff --git a/sequencer.c b/sequencer.c
> index 2dad7041960..99d33d4e063 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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).

Yes that would be helpful for future readers I think.
> 
> What do you think about this approach?

I like it assuming it's easy to extend it to the 'reword the root
commit' case

Best Wishes

Phillip
> Dscho
> 

Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

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

So, for the second attempt by Elijah, Dscho said it is good while
you noticed a problem with ff, to which Dscho agreed is a better
approach, and you extended it which brings us here.

I'll leave the thread hanging here while I pick other leftover bits
for 2.18 final and then will come back.  Hopefully by that time I'll
find the final version of the patch to conclude this topic that I
can apply on top of 2.18.0 ;-)

Thanks.

Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts

[Johannes Schindelin]

Hi Phillip,

On Mon, 18 Jun 2018, Phillip Wood wrote:

> On 17/06/18 20:28, Johannes Schindelin wrote:
> > 
> > On Sun, 17 Jun 2018, Phillip Wood wrote:
> > 
> >> On 17/06/18 06:37, Elijah Newren wrote:
> >>> 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.
> >>
> >> 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.
> > 
> > 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.

> > -- snip --
> > diff --git a/sequencer.c b/sequencer.c
> > index 2dad7041960..99d33d4e063 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -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;

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`:


-                       } 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

Re: [PATCH v2] sequencer: do not squash 'reword' commits when wehit conflicts

[Phillip Wood]

Hi Johannes

On 18/06/18 22:42, Johannes Schindelin wrote:
> 
> Hi Phillip,
> 
> On Mon, 18 Jun 2018, Phillip Wood wrote:
> 
>> On 17/06/18 20:28, Johannes Schindelin wrote:
>>>
>>> On Sun, 17 Jun 2018, Phillip Wood wrote:
>>>
>>>> On 17/06/18 06:37, Elijah Newren wrote:
>>>>> 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.
>>>>
>>>> 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.
>>>
>>> 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.
> 
>>> -- snip --
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 2dad7041960..99d33d4e063 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -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;
> 
> 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`:
> 
> 
> -                       } 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?

Looks good

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

Done, I'm just waiting on travis to check everything before sending this
afternoon

Best Wishes

Phillip

> Thanks,
> Dscho
> 

[PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <Johannes.Schindelin@gmx.de>
Original-patch-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

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

diff --git a/sequencer.c b/sequencer.c
index 4034c0461b..7bf2b62727 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e500d7c320..3786879e80 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -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 &&
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
new file mode 100755
index 0000000000..6963750794
--- /dev/null
+++ b/t/t3423-rebase-reword.sh
@@ -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

Re: [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts

[Elijah Newren]

On Tue, Jun 19, 2018 at 5:46 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <Johannes.Schindelin@gmx.de>
> Original-patch-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>
> 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.

Yeah, this one was a little challenging on authorship.  What you've
done here is fine from my angle; thanks for catching the other
important cases I missed.

Junio: As a reminder, this is a bugfix for a regression that appeared
in git 2.13.0; it is not new to the 2.18 cycle.

[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.]

>  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
>
> diff --git a/sequencer.c b/sequencer.c
> index 4034c0461b..7bf2b62727 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index e500d7c320..3786879e80 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -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 &&
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> new file mode 100755
> index 0000000000..6963750794
> --- /dev/null
> +++ b/t/t3423-rebase-reword.sh
> @@ -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
>

Re: [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> [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.]

Thanks for a reminder.

I thought I said that I'll backburner/ignore the topic and expect
something that can be picked up to be there when I come back after I
cut 2.18 final, but I ended up coming back to read the topic anyway
it seems ;-)

[PATCH] t/lib-rebase.sh: support explicit 'pick' commands in 'fake_editor.sh'

[SZEDER Gábor]

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 <szeder.dev@gmail.com>
---
 t/lib-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 25a77ee5cb..592865d019 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -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";;
-- 
2.19.0.rc0.136.gd2dd172e64

Re: [PATCH] t/lib-rebase.sh: support explicit 'pick' commands in 'fake_editor.sh'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 25a77ee5cb..592865d019 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -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";;

There is a large comment before the function that says

    ...
    #   The following word combinations are possible:
    #
    #   "<lineno>" -- add a "pick" line with the SHA1 taken from the
    #       specified line.
    #
    #   "<cmd> <lineno>" -- add a line with the specified command
    #       ("squash", "fixup", "edit", "reword" or "drop") and the SHA1 taken
    #       from the specified line.
    #
    #   "exec_cmd_with_args" -- add an "exec cmd with args" line.
    ...

which probably needs an update, too.

Re: [PATCH] t/lib-rebase.sh: support explicit 'pick' commands in 'fake_editor.sh'

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

Hi Gábor,

On Thu, 23 Aug 2018, SZEDER Gábor wrote:

> 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 <szeder.dev@gmail.com>

ACK!

Thank you very much,
Dscho

> ---
>  t/lib-rebase.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 25a77ee5cb..592865d019 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -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";;
> -- 
> 2.19.0.rc0.136.gd2dd172e64
> 
> 

Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts

[Johannes Schindelin]

Hi Elijah,

On Sat, 16 Jun 2018, Elijah Newren wrote:

> 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 <newren@gmail.com>
> ---
> 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
> 
> diff --git a/sequencer.c b/sequencer.c
> index cca968043e..9e6d1ee368 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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);

I agree that this is the correct bug fix. ACK!

Thanks,
Dscho