From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-10 09:32:04
If the user removes all the changes from the worktree without running git
reset then it rebase --continue leaves behind .git/MERGE_MSG which will then
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.
Phillip Wood (3):
t3403: fix commit authorship
rebase --apply: restore some tests
rebase --continue: remove .git/MERGE_MSG
sequencer.c | 3 +++
t/t3403-rebase-skip.sh | 13 +++++++++++--
t/t3418-rebase-continue.sh | 14 ++++++++++++--
3 files changed, 26 insertions(+), 4 deletions(-)
base-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1013
--
gitgitgadget
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-10 09:32:05
From: Phillip Wood <redacted>
Setting GIT_AUTHOR_* when committing with --amend will only change the
author if we also pass --reset-author
Signed-off-by: Phillip Wood <redacted>
---
t/t3403-rebase-skip.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-10 09:32:16
From: Phillip Wood <redacted>
980b482d28 ("rebase tests: mark tests specific to the am-backend with
--am", 2020-02-15) sought to prepare tests testing the "apply" backend
in preparation for 2ac0d6273f ("rebase: change the default backend
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
files, I have not audited all the other rebase test files.
Signed-off-by: Phillip Wood <redacted>
---
t/t3403-rebase-skip.sh | 2 +-
t/t3418-rebase-continue.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
@@ -52,7 +52,7 @@ test_expect_success setup '' test_expect_success'rebase with git am -3 (default)''-test_must_failgitrebasemain+test_must_failgitrebase--applymain' test_expect_success'rebase --skip can not be used with other options''
@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'gitreset--hard&&gitcheckoutmain&&-test_must_failgitrebase--ontomainmaintopic&&+test_must_failgitrebase--apply--ontomainmaintopic&&echo"Resolved">F2&&gitaddF2&&test-toolchmtime=-60F1&&
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-10 09:32:21
From: Phillip Wood <redacted>
If the user skips the final commit by removing all the changes from
the index and worktree with 'git restore' (or read-tree) and then runs
'git rebase --continue' .git/MERGE_MSG is left behind. This will seed
the commit message the next time the user commits which is not what we
want to happen.
Reported-by: Victor Gambier <redacted>
Signed-off-by: Phillip Wood <redacted>
---
sequencer.c | 3 +++
t/t3403-rebase-skip.sh | 8 ++++++++
t/t3418-rebase-continue.sh | 10 ++++++++++
3 files changed, 21 insertions(+)
@@ -4716,6 +4716,9 @@ static int commit_staged_changes(struct repository *r,refs_delete_ref(get_main_ref_store(r),"","CHERRY_PICK_HEAD",NULL,0))returnerror(_("could not remove CHERRY_PICK_HEAD"));+if(unlink(git_path_merge_msg(r))&&errno!=ENOENT)+returnerror_errno(_("could not remove '%s'"),+git_path_merge_msg(r));if(!final_fixup)return0;}
@@ -31,6 +31,16 @@ test_expect_success 'interactive rebase --continue works with touched file' 'gitrebase--continue'+test_expect_success'rebase --continue removes .git/MERGE_MSG''+gitcheckout-f--detachtopic&&++test_must_failgitrebase--ontomainHEAD^&&+gitread-tree--reset-uHEAD&&+test_path_is_file.git/MERGE_MSG&&+gitrebase--continue&&+test_path_is_missing.git/MERGE_MSG+'+ test_expect_success'non-interactive rebase --continue works with touched file''rm-fr.git/rebase-*&&gitreset--hard&&
On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
quoted hunk
From: Phillip Wood <redacted>
980b482d28 ("rebase tests: mark tests specific to the am-backend with
--am", 2020-02-15) sought to prepare tests testing the "apply" backend
in preparation for 2ac0d6273f ("rebase: change the default backend
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
files, I have not audited all the other rebase test files.
Signed-off-by: Phillip Wood <redacted>
---
t/t3403-rebase-skip.sh | 2 +-
t/t3418-rebase-continue.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'gitreset--hard&&gitcheckoutmain&&-test_must_failgitrebase--ontomainmaintopic&&+test_must_failgitrebase--apply--ontomainmaintopic&&
I think the point here is that you're noticing that "git rebase -i"
and "git rebase" are both built on the same rebase backend (the merge
one) and thus that testing interactive vs. non-interactive isn't much
of a test. The real test we are interested in is merge-backend vs.
apply-backend. Your code change here is the necessary one to do that,
but it least the test descriptions still talking about interactive vs.
non-interactive even though that's not what we're concentrating on
anymore.
I'd say we'd at least want to change the description for this test,
"non-interactive rebase ---continue works with..." => "rebase
--continue with the apply backend works with...", but that we'd
probably want to change the description of the test before it and
maybe even just use rebase --merge rather than rebase -i.
On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
quoted hunk
From: Phillip Wood <redacted>
Setting GIT_AUTHOR_* when committing with --amend will only change the
author if we also pass --reset-author
Signed-off-by: Phillip Wood <redacted>
---
t/t3403-rebase-skip.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Makes sense...but doesn't the fact that this test worked either way
suggest that the specifying of a special author name/email was totally
superfluous and could just be removed? If there really was a reason
for specifying a different name/email, then is the test faulty for not
checking for it somewhere?
On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
If the user removes all the changes from the worktree without running git
reset then it rebase --continue leaves behind .git/MERGE_MSG which will then
s/then it/then/
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.
I had a couple small comments on the first two patches. Otherwise,
the whole series looks good to me.
@@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'gitreset--hard&&gitcheckoutmain&&-test_must_failgitrebase--ontomainmaintopic&&+test_must_failgitrebase--apply--ontomainmaintopic&&
I think the point here is that you're noticing that "git rebase -i"
and "git rebase" are both built on the same rebase backend (the merge
one) and thus that testing interactive vs. non-interactive isn't much
of a test.
Exactly
The real test we are interested in is merge-backend vs.
apply-backend. Your code change here is the necessary one to do that,
but it least the test descriptions still talking about interactive vs.
non-interactive even though that's not what we're concentrating on
anymore.
I'd say we'd at least want to change the description for this test,
"non-interactive rebase ---continue works with..." => "rebase
--continue with the apply backend works with...", but that we'd
probably want to change the description of the test before it and
maybe even just use rebase --merge rather than rebase -i.
Rewording the test descriptions is a good suggestion
Thanks
Phillip
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-12 13:42:15
Thanks for the comments on V1. I have expanded the commit message for the
first patch to give a bit more context and reworded a couple of test
descriptions in patch 2 as suggested by Elijah. Patch 3 is unchanged.
V1 Cover Letter:
If the user removes all the changes from the worktree without running git
reset then rebase --continue leaves behind .git/MERGE_MSG which will then
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.
cc: Victor Gambier vgambier@excilys.com cc: Elijah Newren newren@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
Phillip Wood (3):
t3403: fix commit authorship
rebase --apply: restore some tests
rebase --continue: remove .git/MERGE_MSG
sequencer.c | 3 +++
t/t3403-rebase-skip.sh | 13 +++++++++++--
t/t3418-rebase-continue.sh | 18 ++++++++++++++----
3 files changed, 28 insertions(+), 6 deletions(-)
base-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1013
Range-diff vs v1:
1: 7559781ca92 ! 1: 8755dfa9d04 t3403: fix commit authorship
@@ Commit message
t3403: fix commit authorship
Setting GIT_AUTHOR_* when committing with --amend will only change the
- author if we also pass --reset-author
+ author if we also pass --reset-author. This commit is used in some
+ tests that ensure the author ident does not change when rebasing.
+ Creating this commit without changing the authorship meant that the
+ test would not catch regressions that caused rebase to discard the
+ original authorship information.
Signed-off-by: Phillip Wood [off-list ref]
2: f7722dab21d ! 2: 84fe5823b4f rebase --apply: restore some tests
@@ Commit message
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
- files, I have not audited all the other rebase test files.
+ files, I have not audited all the other rebase test files. I've
+ reworded a couple of the test descriptions to make it clear which
+ backend they are testing.
Signed-off-by: Phillip Wood [off-list ref]
@@ t/t3403-rebase-skip.sh: test_expect_success setup '
test_expect_success 'rebase --skip can not be used with other options' '
## t/t3418-rebase-continue.sh ##
-@@ t/t3418-rebase-continue.sh: test_expect_success 'non-interactive rebase --continue works with touched file'
+@@ t/t3418-rebase-continue.sh: test_expect_success 'setup' '
+ git checkout main
+ '
+
+-test_expect_success 'interactive rebase --continue works with touched file' '
++test_expect_success 'merge based rebase --continue with works with touched file' '
+ rm -fr .git/rebase-* &&
+ git reset --hard &&
+ git checkout main &&
+@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
+ git rebase --continue
+ '
+
+-test_expect_success 'non-interactive rebase --continue works with touched file' '
++test_expect_success 'apply based rebase --continue works with touched file' '
+ rm -fr .git/rebase-* &&
git reset --hard &&
git checkout main &&
3: 6a63b657f13 ! 3: 028c9dfc460 rebase --continue: remove .git/MERGE_MSG
@@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '
test_must_fail git rebase -i --onto goodbye \
## t/t3418-rebase-continue.sh ##
-@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
+@@ t/t3418-rebase-continue.sh: test_expect_success 'merge based rebase --continue with works with touched file'
git rebase --continue
'
-+test_expect_success 'rebase --continue removes .git/MERGE_MSG' '
++test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
+ git checkout -f --detach topic &&
+
+ test_must_fail git rebase --onto main HEAD^ &&
@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue w
+ test_path_is_missing .git/MERGE_MSG
+'
+
- test_expect_success 'non-interactive rebase --continue works with touched file' '
+ test_expect_success 'apply based rebase --continue works with touched file' '
rm -fr .git/rebase-* &&
git reset --hard &&
--
gitgitgadget
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-12 13:42:16
From: Phillip Wood <redacted>
Setting GIT_AUTHOR_* when committing with --amend will only change the
author if we also pass --reset-author. This commit is used in some
tests that ensure the author ident does not change when rebasing.
Creating this commit without changing the authorship meant that the
test would not catch regressions that caused rebase to discard the
original authorship information.
Signed-off-by: Phillip Wood <redacted>
---
t/t3403-rebase-skip.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-12 13:42:17
From: Phillip Wood <redacted>
980b482d28 ("rebase tests: mark tests specific to the am-backend with
--am", 2020-02-15) sought to prepare tests testing the "apply" backend
in preparation for 2ac0d6273f ("rebase: change the default backend
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
files, I have not audited all the other rebase test files. I've
reworded a couple of the test descriptions to make it clear which
backend they are testing.
Signed-off-by: Phillip Wood <redacted>
---
t/t3403-rebase-skip.sh | 2 +-
t/t3418-rebase-continue.sh | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
@@ -52,7 +52,7 @@ test_expect_success setup '' test_expect_success'rebase with git am -3 (default)''-test_must_failgitrebasemain+test_must_failgitrebase--applymain' test_expect_success'rebase --skip can not be used with other options''
@@ -21,7 +21,7 @@ test_expect_success 'setup' 'gitcheckoutmain'-test_expect_success'interactive rebase --continue works with touched file''+test_expect_success'merge based rebase --continue with works with touched file''rm-fr.git/rebase-*&&gitreset--hard&&gitcheckoutmain&&
@@ -31,12 +31,12 @@ test_expect_success 'interactive rebase --continue works with touched file' 'gitrebase--continue'-test_expect_success'non-interactive rebase --continue works with touched file''+test_expect_success'apply based rebase --continue works with touched file''rm-fr.git/rebase-*&&gitreset--hard&&gitcheckoutmain&&-test_must_failgitrebase--ontomainmaintopic&&+test_must_failgitrebase--apply--ontomainmaintopic&&echo"Resolved">F2&&gitaddF2&&test-toolchmtime=-60F1&&
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-08-12 13:42:18
From: Phillip Wood <redacted>
If the user skips the final commit by removing all the changes from
the index and worktree with 'git restore' (or read-tree) and then runs
'git rebase --continue' .git/MERGE_MSG is left behind. This will seed
the commit message the next time the user commits which is not what we
want to happen.
Reported-by: Victor Gambier <redacted>
Signed-off-by: Phillip Wood <redacted>
---
sequencer.c | 3 +++
t/t3403-rebase-skip.sh | 8 ++++++++
t/t3418-rebase-continue.sh | 10 ++++++++++
3 files changed, 21 insertions(+)
@@ -4716,6 +4716,9 @@ static int commit_staged_changes(struct repository *r,refs_delete_ref(get_main_ref_store(r),"","CHERRY_PICK_HEAD",NULL,0))returnerror(_("could not remove CHERRY_PICK_HEAD"));+if(unlink(git_path_merge_msg(r))&&errno!=ENOENT)+returnerror_errno(_("could not remove '%s'"),+git_path_merge_msg(r));if(!final_fixup)return0;}
@@ -31,6 +31,16 @@ test_expect_success 'merge based rebase --continue with works with touched file'gitrebase--continue'+test_expect_success'merge based rebase --continue removes .git/MERGE_MSG''+gitcheckout-f--detachtopic&&++test_must_failgitrebase--ontomainHEAD^&&+gitread-tree--reset-uHEAD&&+test_path_is_file.git/MERGE_MSG&&+gitrebase--continue&&+test_path_is_missing.git/MERGE_MSG+'+ test_expect_success'apply based rebase --continue works with touched file''rm-fr.git/rebase-*&&gitreset--hard&&
On Thu, Aug 12, 2021 at 6:46 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
Thanks for the comments on V1. I have expanded the commit message for the
first patch to give a bit more context and reworded a couple of test
descriptions in patch 2 as suggested by Elijah. Patch 3 is unchanged.
V1 Cover Letter:
If the user removes all the changes from the worktree without running git
reset then rebase --continue leaves behind .git/MERGE_MSG which will then
seed the message of the next commit. While working on this I noticed a
couple of fixups for the test files I was adding to, I've cc'd Elijah in
case my reasoning for patch 2 is off. Thanks to Victor for the bug report.
cc: Victor Gambier vgambier@excilys.com cc: Elijah Newren newren@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
Phillip Wood (3):
t3403: fix commit authorship
rebase --apply: restore some tests
rebase --continue: remove .git/MERGE_MSG
sequencer.c | 3 +++
t/t3403-rebase-skip.sh | 13 +++++++++++--
t/t3418-rebase-continue.sh | 18 ++++++++++++++----
3 files changed, 28 insertions(+), 6 deletions(-)
base-commit: 2d755dfac9aadab25c3e025b849252b8c0a61465
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1013%2Fphillipwood%2Fwip%2Frebase-remove-merge-msg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1013/phillipwood/wip/rebase-remove-merge-msg-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1013
Range-diff vs v1:
1: 7559781ca92 ! 1: 8755dfa9d04 t3403: fix commit authorship
@@ Commit message
t3403: fix commit authorship
Setting GIT_AUTHOR_* when committing with --amend will only change the
- author if we also pass --reset-author
+ author if we also pass --reset-author. This commit is used in some
+ tests that ensure the author ident does not change when rebasing.
+ Creating this commit without changing the authorship meant that the
+ test would not catch regressions that caused rebase to discard the
+ original authorship information.
Signed-off-by: Phillip Wood [off-list ref]
2: f7722dab21d ! 2: 84fe5823b4f rebase --apply: restore some tests
@@ Commit message
from "am" to "merge"", 2020-02-15). However some tests seem to have
been missed leading to us testing the "merge" backend twice. This
patch fixes some cases that I noticed while adding tests to these
- files, I have not audited all the other rebase test files.
+ files, I have not audited all the other rebase test files. I've
+ reworded a couple of the test descriptions to make it clear which
+ backend they are testing.
Signed-off-by: Phillip Wood [off-list ref]
@@ t/t3403-rebase-skip.sh: test_expect_success setup '
test_expect_success 'rebase --skip can not be used with other options' '
## t/t3418-rebase-continue.sh ##
-@@ t/t3418-rebase-continue.sh: test_expect_success 'non-interactive rebase --continue works with touched file'
+@@ t/t3418-rebase-continue.sh: test_expect_success 'setup' '
+ git checkout main
+ '
+
+-test_expect_success 'interactive rebase --continue works with touched file' '
++test_expect_success 'merge based rebase --continue with works with touched file' '
+ rm -fr .git/rebase-* &&
+ git reset --hard &&
+ git checkout main &&
+@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
+ git rebase --continue
+ '
+
+-test_expect_success 'non-interactive rebase --continue works with touched file' '
++test_expect_success 'apply based rebase --continue works with touched file' '
+ rm -fr .git/rebase-* &&
git reset --hard &&
git checkout main &&
3: 6a63b657f13 ! 3: 028c9dfc460 rebase --continue: remove .git/MERGE_MSG
@@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '
test_must_fail git rebase -i --onto goodbye \
## t/t3418-rebase-continue.sh ##
-@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue works with touched file' '
+@@ t/t3418-rebase-continue.sh: test_expect_success 'merge based rebase --continue with works with touched file'
git rebase --continue
'
-+test_expect_success 'rebase --continue removes .git/MERGE_MSG' '
++test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
+ git checkout -f --detach topic &&
+
+ test_must_fail git rebase --onto main HEAD^ &&
@@ t/t3418-rebase-continue.sh: test_expect_success 'interactive rebase --continue w
+ test_path_is_missing .git/MERGE_MSG
+'
+
- test_expect_success 'non-interactive rebase --continue works with touched file' '
+ test_expect_success 'apply based rebase --continue works with touched file' '
rm -fr .git/rebase-* &&
git reset --hard &&
--
gitgitgadget