diff mbox series

[v2,9/9,RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts

Message ID 20191206160614.631724-10-phillip.wood123@gmail.com (mailing list archive)
State New, archived
Headers show
Series commit: fix advice for empty commits during rebases | expand

Commit Message

Phillip Wood Dec. 6, 2019, 4:06 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

Since the inception of CHERRY_PICK_HEAD in d7e5c0cbfb ("Introduce
CHERRY_PICK_HEAD", 2011-02-19) 'rebase -i' has removed it when there are
conflicts. The rationale for this was that the rebase wanted to handle
the conflicts itself. However sometimes (e.g. after an edit command) the
user wants to commit the conflict resolution before making some other
changes or running some tests. Without CHERRY_PICK_HEAD the authorship
information is lost when the user makes the commit. Fix this by leaving
CHERRY_PICK_HEAD when we're not amending.

Note that this changes the output of `git status`. The advice to run
`git reset` is not appropriate for rebase as we do not allow partial
commits.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    This has semantic conflicts with ra/rebase-i-more-options as it does not
    respect the options passed to rebase when the user commits
    
    I haven't checked how this affects the shell prompt in contrib yet, I
    suspect it may need changing to cope the presence of CHERRY_PICK_HEAD
    during a rebase.
    
    I'd like to change the existing authorship tests to rely on the "Original
    Author" changes here, but they are a web of hidden interdependencies which is
    hard to untangle.

 sequencer.c                   |  12 ++--
 t/t3404-rebase-interactive.sh | 104 +++++++++++++++++++++++++---------
 t/t7512-status-help.sh        |   2 -
 3 files changed, 85 insertions(+), 33 deletions(-)

Comments

Phillip Wood Dec. 18, 2019, 2:35 p.m. UTC | #1 
On 06/12/2019 16:06, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> Since the inception of CHERRY_PICK_HEAD in d7e5c0cbfb ("Introduce
> CHERRY_PICK_HEAD", 2011-02-19) 'rebase -i' has removed it when there are
> conflicts. The rationale for this was that the rebase wanted to handle
> the conflicts itself. However sometimes (e.g. after an edit command) the
> user wants to commit the conflict resolution before making some other
> changes or running some tests. Without CHERRY_PICK_HEAD the authorship
> information is lost when the user makes the commit. Fix this by leaving
> CHERRY_PICK_HEAD when we're not amending.

I'm not so sure about this approach as it wont work with 'merge' 
commands when rebasing. I wonder if it would be better to add a new file 
COMMIT_AUTHOR (or maybe MERGE_AUTHOR) that can be parsed by 
split_ident() and sets the authorship for a commit. The file would 
override $GIT_AUTHOR_NAME/EMAIL/DATE but could be overridden on the 
commandline by --author/date/reset-author

Best Wishes

Phillip

> Note that this changes the output of `git status`. The advice to run
> `git reset` is not appropriate for rebase as we do not allow partial
> commits.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> 
> Notes:
>      This has semantic conflicts with ra/rebase-i-more-options as it does not
>      respect the options passed to rebase when the user commits
>      
>      I haven't checked how this affects the shell prompt in contrib yet, I
>      suspect it may need changing to cope the presence of CHERRY_PICK_HEAD
>      during a rebase.
>      
>      I'd like to change the existing authorship tests to rely on the "Original
>      Author" changes here, but they are a web of hidden interdependencies which is
>      hard to untangle.
> 
>   sequencer.c                   |  12 ++--
>   t/t3404-rebase-interactive.sh | 104 +++++++++++++++++++++++++---------
>   t/t7512-status-help.sh        |   2 -
>   3 files changed, 85 insertions(+), 33 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 64242f4ce7..624e96c930 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -372,11 +372,15 @@ static void print_advice(struct repository *r, int show_hint,
>   	if (msg) {
>   		fprintf(stderr, "%s\n", msg);
>   		/*
> -		 * A conflict has occurred but the porcelain
> -		 * (typically rebase --interactive) wants to take care
> -		 * of the commit itself so remove CHERRY_PICK_HEAD
> +		 * A conflict has occurred but the porcelain wants to take care
> +		 * of the commit itself so remove CHERRY_PICK_HEAD. Note that we
> +		 * do not do this for interactive rebases anymore in order to
> +		 * preserve the author identity when the user runs 'git commit'
> +		 * to commit the conflict resolution rather than relying on
> +		 * 'rebase --continue' to do it for them.
>   		 */
> -		unlink(git_path_cherry_pick_head(r));
> +		if (!is_rebase_i(opts))
> +			unlink(git_path_cherry_pick_head(r));
>   		return;
>   	}
>   
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 5afa6f28cd..5cd7db18f8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -33,31 +33,35 @@ Initial setup:
>   # in the expect2 file for the 'stop on conflicting pick' test.
>   
>   test_expect_success 'setup' '
> -	test_commit A file1 &&
> -	test_commit B file1 &&
> -	test_commit C file2 &&
> -	test_commit D file1 &&
> -	test_commit E file3 &&
> -	git checkout -b branch1 A &&
> -	test_commit F file4 &&
> -	test_commit G file1 &&
> -	test_commit H file5 &&
> -	git checkout -b branch2 F &&
> -	test_commit I file6 &&
> -	git checkout -b conflict-branch A &&
> -	test_commit one conflict &&
> -	test_commit two conflict &&
> -	test_commit three conflict &&
> -	test_commit four conflict &&
> -	git checkout -b no-conflict-branch A &&
> -	test_commit J fileJ &&
> -	test_commit K fileK &&
> -	test_commit L fileL &&
> -	test_commit M fileM &&
> -	git checkout -b no-ff-branch A &&
> -	test_commit N fileN &&
> -	test_commit O fileO &&
> -	test_commit P fileP
> +	(
> +		GIT_AUTHOR_NAME="Original Author" &&
> +		GIT_AUTHOR_EMAIL="original.author@example.com" &&
> +		test_commit A file1 &&
> +		test_commit B file1 &&
> +		test_commit C file2 &&
> +		test_commit D file1 &&
> +		test_commit E file3 &&
> +		git checkout -b branch1 A &&
> +		test_commit F file4 &&
> +		test_commit G file1 &&
> +		test_commit H file5 &&
> +		git checkout -b branch2 F &&
> +		test_commit I file6 &&
> +		git checkout -b conflict-branch A &&
> +		test_commit one conflict &&
> +		test_commit two conflict &&
> +		test_commit three conflict &&
> +		test_commit four conflict &&
> +		git checkout -b no-conflict-branch A &&
> +		test_commit J fileJ &&
> +		test_commit K fileK &&
> +		test_commit L fileL &&
> +		test_commit M fileM &&
> +		git checkout -b no-ff-branch A &&
> +		test_commit N fileN &&
> +		test_commit O fileO &&
> +		test_commit P fileP
> +	)
>   '
>   
>   # "exec" commands are run with the user shell by default, but this may
> @@ -252,12 +256,12 @@ test_expect_success 'stop on conflicting pick' '
>   	-A
>   	+G
>   	EOF
> -	cat >expect2 <<-\EOF &&
> +	cat >expect2 <<-EOF &&
>   	<<<<<<< HEAD
>   	D
>   	=======
>   	G
> -	>>>>>>> 5d18e54... G
> +	>>>>>>> $(git rev-parse --short HEAD)... G
>   	EOF
>   	git tag new-branch1 &&
>   	test_must_fail git rebase -i master &&
> @@ -1628,6 +1632,52 @@ test_expect_success 'correct error message for commit --amend after empty pick'
>   	test_i18ngrep "middle of a rebase -- cannot amend." err
>   '
>   
> +test_expect_success 'correct error message for partial commit after confilct' '
> +	test_when_finished "git rebase --abort" &&
> +	git checkout D &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES="2 3" &&
> +		export FAKE_LINES &&
> +		test_must_fail git rebase -i A
> +	) &&
> +	echo x >file1 &&
> +	echo y >file2 &&
> +	git add file1 file2 &&
> +	test_must_fail git commit file1 2>err &&
> +	test_i18ngrep "cannot do a partial commit during a rebase." err
> +'
> +
> +test_expect_success 'correct error message for commit --amend after conflict' '
> +	test_when_finished "git rebase --abort" &&
> +	git checkout D &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES=3 &&
> +		export FAKE_LINES &&
> +		test_must_fail git rebase -i A
> +	) &&
> +	echo x>file1 &&
> +	test_must_fail git commit -a --amend 2>err &&
> +	test_i18ngrep "middle of a rebase -- cannot amend." err
> +'
> +
> +test_expect_success 'correct authorship and message after conflict' '
> +	git checkout D &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES=3 &&
> +		export FAKE_LINES &&
> +		test_must_fail git rebase -i A
> +	) &&
> +	echo x >file1 &&
> +	git commit -a &&
> +	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 D >expect &&
> +	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 HEAD >actual &&
> +	test_cmp expect actual &&
> +	git rebase --continue
> +'
> +
>   # This must be the last test in this file
>   test_expect_success '$EDITOR and friends are unchanged' '
>   	test_editor_unchanged
> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index c1eb72555d..2adceb35e2 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -148,7 +148,6 @@ You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
>     (use "git rebase --abort" to check out the original branch)
>   
>   Unmerged paths:
> -  (use "git reset HEAD <file>..." to unstage)
>     (use "git add <file>..." to mark resolution)
>   
>   	both modified:   main.txt
> @@ -176,7 +175,6 @@ You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
>     (all conflicts fixed: run "git rebase --continue")
>   
>   Changes to be committed:
> -  (use "git reset HEAD <file>..." to unstage)
>   
>   	modified:   main.txt
>   
>
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index 64242f4ce7..624e96c930 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -372,11 +372,15 @@  static void print_advice(struct repository *r, int show_hint,
 	if (msg) {
 		fprintf(stderr, "%s\n", msg);
 		/*
-		 * A conflict has occurred but the porcelain
-		 * (typically rebase --interactive) wants to take care
-		 * of the commit itself so remove CHERRY_PICK_HEAD
+		 * A conflict has occurred but the porcelain wants to take care
+		 * of the commit itself so remove CHERRY_PICK_HEAD. Note that we
+		 * do not do this for interactive rebases anymore in order to
+		 * preserve the author identity when the user runs 'git commit'
+		 * to commit the conflict resolution rather than relying on
+		 * 'rebase --continue' to do it for them.
 		 */
-		unlink(git_path_cherry_pick_head(r));
+		if (!is_rebase_i(opts))
+			unlink(git_path_cherry_pick_head(r));
 		return;
 	}
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5afa6f28cd..5cd7db18f8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -33,31 +33,35 @@  Initial setup:
 # in the expect2 file for the 'stop on conflicting pick' test.
 
 test_expect_success 'setup' '
-	test_commit A file1 &&
-	test_commit B file1 &&
-	test_commit C file2 &&
-	test_commit D file1 &&
-	test_commit E file3 &&
-	git checkout -b branch1 A &&
-	test_commit F file4 &&
-	test_commit G file1 &&
-	test_commit H file5 &&
-	git checkout -b branch2 F &&
-	test_commit I file6 &&
-	git checkout -b conflict-branch A &&
-	test_commit one conflict &&
-	test_commit two conflict &&
-	test_commit three conflict &&
-	test_commit four conflict &&
-	git checkout -b no-conflict-branch A &&
-	test_commit J fileJ &&
-	test_commit K fileK &&
-	test_commit L fileL &&
-	test_commit M fileM &&
-	git checkout -b no-ff-branch A &&
-	test_commit N fileN &&
-	test_commit O fileO &&
-	test_commit P fileP
+	(
+		GIT_AUTHOR_NAME="Original Author" &&
+		GIT_AUTHOR_EMAIL="original.author@example.com" &&
+		test_commit A file1 &&
+		test_commit B file1 &&
+		test_commit C file2 &&
+		test_commit D file1 &&
+		test_commit E file3 &&
+		git checkout -b branch1 A &&
+		test_commit F file4 &&
+		test_commit G file1 &&
+		test_commit H file5 &&
+		git checkout -b branch2 F &&
+		test_commit I file6 &&
+		git checkout -b conflict-branch A &&
+		test_commit one conflict &&
+		test_commit two conflict &&
+		test_commit three conflict &&
+		test_commit four conflict &&
+		git checkout -b no-conflict-branch A &&
+		test_commit J fileJ &&
+		test_commit K fileK &&
+		test_commit L fileL &&
+		test_commit M fileM &&
+		git checkout -b no-ff-branch A &&
+		test_commit N fileN &&
+		test_commit O fileO &&
+		test_commit P fileP
+	)
 '
 
 # "exec" commands are run with the user shell by default, but this may
@@ -252,12 +256,12 @@  test_expect_success 'stop on conflicting pick' '
 	-A
 	+G
 	EOF
-	cat >expect2 <<-\EOF &&
+	cat >expect2 <<-EOF &&
 	<<<<<<< HEAD
 	D
 	=======
 	G
-	>>>>>>> 5d18e54... G
+	>>>>>>> $(git rev-parse --short HEAD)... G
 	EOF
 	git tag new-branch1 &&
 	test_must_fail git rebase -i master &&
@@ -1628,6 +1632,52 @@  test_expect_success 'correct error message for commit --amend after empty pick'
 	test_i18ngrep "middle of a rebase -- cannot amend." err
 '
 
+test_expect_success 'correct error message for partial commit after confilct' '
+	test_when_finished "git rebase --abort" &&
+	git checkout D &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="2 3" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A
+	) &&
+	echo x >file1 &&
+	echo y >file2 &&
+	git add file1 file2 &&
+	test_must_fail git commit file1 2>err &&
+	test_i18ngrep "cannot do a partial commit during a rebase." err
+'
+
+test_expect_success 'correct error message for commit --amend after conflict' '
+	test_when_finished "git rebase --abort" &&
+	git checkout D &&
+	(
+		set_fake_editor &&
+		FAKE_LINES=3 &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A
+	) &&
+	echo x>file1 &&
+	test_must_fail git commit -a --amend 2>err &&
+	test_i18ngrep "middle of a rebase -- cannot amend." err
+'
+
+test_expect_success 'correct authorship and message after conflict' '
+	git checkout D &&
+	(
+		set_fake_editor &&
+		FAKE_LINES=3 &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A
+	) &&
+	echo x >file1 &&
+	git commit -a &&
+	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 D >expect &&
+	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 HEAD >actual &&
+	test_cmp expect actual &&
+	git rebase --continue
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index c1eb72555d..2adceb35e2 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -148,7 +148,6 @@  You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
   (use "git rebase --abort" to check out the original branch)
 
 Unmerged paths:
-  (use "git reset HEAD <file>..." to unstage)
   (use "git add <file>..." to mark resolution)
 
 	both modified:   main.txt
@@ -176,7 +175,6 @@  You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
   (all conflicts fixed: run "git rebase --continue")
 
 Changes to be committed:
-  (use "git reset HEAD <file>..." to unstage)
 
 	modified:   main.txt