git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [BUG] git-rebase: reword squashes commits in case of merge-conflicts
@ 2018-06-11 16:06 ch
  2018-06-12 10:08 ` Jeff King
  2018-06-16 16:08 ` Elijah Newren
  0 siblings, 2 replies; 16+ messages in thread
From: ch @ 2018-06-11 16:06 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts
  2018-06-11 16:06 [BUG] git-rebase: reword squashes commits in case of merge-conflicts ch
@ 2018-06-12 10:08 ` Jeff King
  2018-06-15 14:35   ` ch
  2018-06-16 16:08 ` Elijah Newren
  1 sibling, 1 reply; 16+ messages in thread
From: Jeff King @ 2018-06-12 10:08 UTC (permalink / raw)
  To: ch; +Cc: Johannes Schindelin, git

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts
  2018-06-12 10:08 ` Jeff King
@ 2018-06-15 14:35   ` ch
  0 siblings, 0 replies; 16+ messages in thread
From: ch @ 2018-06-15 14:35 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, git

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts
  2018-06-11 16:06 [BUG] git-rebase: reword squashes commits in case of merge-conflicts ch
  2018-06-12 10:08 ` Jeff King
@ 2018-06-16 16:08 ` Elijah Newren
  2018-06-17  3:36   ` Eric Sunshine
  1 sibling, 1 reply; 16+ messages in thread
From: Elijah Newren @ 2018-06-16 16:08 UTC (permalink / raw)
  To: ch; +Cc: johannes.schindelin, git, gitster, 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts
  2018-06-16 16:08 ` Elijah Newren
@ 2018-06-17  3:36   ` Eric Sunshine
  2018-06-17  5:37     ` [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts Elijah Newren
  0 siblings, 1 reply; 16+ messages in thread
From: Eric Sunshine @ 2018-06-17  3:36 UTC (permalink / raw)
  To: Elijah Newren; +Cc: cr, Johannes Schindelin, Git List, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-17  3:36   ` Eric Sunshine
@ 2018-06-17  5:37     ` Elijah Newren
  2018-06-17 15:04       ` Phillip Wood
  2018-06-17 18:46       ` [PATCH v2] " Johannes Schindelin
  0 siblings, 2 replies; 16+ messages in thread
From: Elijah Newren @ 2018-06-17  5:37 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: ch, johannes.schindelin, git, gitster, 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-17  5:37     ` [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts Elijah Newren
@ 2018-06-17 15:04       ` Phillip Wood
  2018-06-17 19:28         ` Johannes Schindelin
  2018-06-17 18:46       ` [PATCH v2] " Johannes Schindelin
  1 sibling, 1 reply; 16+ messages in thread
From: Phillip Wood @ 2018-06-17 15:04 UTC (permalink / raw)
  To: Elijah Newren, Eric Sunshine; +Cc: ch, johannes.schindelin, git, gitster

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
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-17  5:37     ` [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts Elijah Newren
  2018-06-17 15:04       ` Phillip Wood
@ 2018-06-17 18:46       ` " Johannes Schindelin
  1 sibling, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2018-06-17 18:46 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Eric Sunshine, ch, git, gitster

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-17 15:04       ` Phillip Wood
@ 2018-06-17 19:28         ` Johannes Schindelin
  2018-06-18 10:20           ` Phillip Wood
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2018-06-17 19:28 UTC (permalink / raw)
  To: phillip.wood; +Cc: Elijah Newren, Eric Sunshine, ch, git, gitster

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-17 19:28         ` Johannes Schindelin
@ 2018-06-18 10:20           ` Phillip Wood
  2018-06-18 15:42             ` Junio C Hamano
  2018-06-18 21:42             ` Johannes Schindelin
  0 siblings, 2 replies; 16+ messages in thread
From: Phillip Wood @ 2018-06-18 10:20 UTC (permalink / raw)
  To: Johannes Schindelin, phillip.wood
  Cc: Elijah Newren, Eric Sunshine, ch, git, gitster

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
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-18 10:20           ` Phillip Wood
@ 2018-06-18 15:42             ` Junio C Hamano
  2018-06-18 21:42             ` Johannes Schindelin
  1 sibling, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2018-06-18 15:42 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin, phillip.wood, Elijah Newren, Eric Sunshine, ch, git

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-18 10:20           ` Phillip Wood
  2018-06-18 15:42             ` Junio C Hamano
@ 2018-06-18 21:42             ` Johannes Schindelin
  2018-06-19 10:00               ` [PATCH v2] sequencer: do not squash 'reword' commits when wehit conflicts Phillip Wood
  2018-06-19 12:46               ` [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts Phillip Wood
  1 sibling, 2 replies; 16+ messages in thread
From: Johannes Schindelin @ 2018-06-18 21:42 UTC (permalink / raw)
  To: phillip.wood; +Cc: Elijah Newren, Eric Sunshine, ch, git, gitster

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] sequencer: do not squash 'reword' commits when wehit conflicts
  2018-06-18 21:42             ` Johannes Schindelin
@ 2018-06-19 10:00               ` Phillip Wood
  2018-06-19 12:46               ` [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts Phillip Wood
  1 sibling, 0 replies; 16+ messages in thread
From: Phillip Wood @ 2018-06-19 10:00 UTC (permalink / raw)
  To: Johannes Schindelin, phillip.wood
  Cc: Elijah Newren, Eric Sunshine, ch, git, gitster

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
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-18 21:42             ` Johannes Schindelin
  2018-06-19 10:00               ` [PATCH v2] sequencer: do not squash 'reword' commits when wehit conflicts Phillip Wood
@ 2018-06-19 12:46               ` Phillip Wood
  2018-06-19 14:29                 ` Elijah Newren
  1 sibling, 1 reply; 16+ messages in thread
From: Phillip Wood @ 2018-06-19 12:46 UTC (permalink / raw)
  To: Junio C Hamano, Johannes Schindelin, Elijah Newren, Git Mailing List
  Cc: Eric Sunshine, ch, 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-19 12:46               ` [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts Phillip Wood
@ 2018-06-19 14:29                 ` Elijah Newren
  2018-06-19 16:59                   ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Elijah Newren @ 2018-06-19 14:29 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Junio C Hamano, Johannes Schindelin, Git Mailing List, Eric Sunshine, ch

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
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts
  2018-06-19 14:29                 ` Elijah Newren
@ 2018-06-19 16:59                   ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2018-06-19 16:59 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Phillip Wood, Johannes Schindelin, Git Mailing List, Eric Sunshine, ch

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 ;-)


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-11 16:06 [BUG] git-rebase: reword squashes commits in case of merge-conflicts ch
2018-06-12 10:08 ` Jeff King
2018-06-15 14:35   ` ch
2018-06-16 16:08 ` Elijah Newren
2018-06-17  3:36   ` Eric Sunshine
2018-06-17  5:37     ` [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts Elijah Newren
2018-06-17 15:04       ` Phillip Wood
2018-06-17 19:28         ` Johannes Schindelin
2018-06-18 10:20           ` Phillip Wood
2018-06-18 15:42             ` Junio C Hamano
2018-06-18 21:42             ` Johannes Schindelin
2018-06-19 10:00               ` [PATCH v2] sequencer: do not squash 'reword' commits when wehit conflicts Phillip Wood
2018-06-19 12:46               ` [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts Phillip Wood
2018-06-19 14:29                 ` Elijah Newren
2018-06-19 16:59                   ` Junio C Hamano
2018-06-17 18:46       ` [PATCH v2] " Johannes Schindelin

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox