sequencer: fix empty commit check when amending
cover sequencer: fix empty commit check when amending
I noticed this while adding some tests for a re-roll of js/advise-rebase-skip
This patch is based on pw/post-commit-from-sequencer
Phillip Wood (1):
sequencer: fix empty commit check when amending
sequencer.c | 24 +++++++++++++++++++----- t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-)
base-commit: 4627bc777e9ade5e3a85d6b8e8630fc4b6e2f8f6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-467%2Fphillipwood%2Fwip%2Frebase-fix-empty-commit-check-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-467/phillipwood/wip/rebase-fix-empty-commit-check-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/467
Signature · 1 line
gitgitgadget
PATCH 1/1 sequencer: fix empty commit check when amending
From: Phillip Wood <[email protected]>
This fixes a regression introduced in 356ee4659b ("sequencer: try to commit without forking 'git commit'", 2017-11-24). When amending a commit try_to_commit() was using the wrong parent when checking if the commit would be empty. When amending we need to check against HEAD^ not HEAD.
t3403 may not seem like the natural home for the new tests but a further patch series will improve the advice printed by git commit. That series will mutate these tests to check that the advice includes suggesting rebase --skip to skip the fixup that would empty the commit.
--- sequencer.c | 24 +++++++++++++++++++----- t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-)
sequencer.c +19 -5
@@ -1351,11 +1351,25 @@ static int try_to_commit(struct repository *r, goto out; } -if (!(flags &ALLOW_EMPTY ) &&oideq (current_head ? -get_commit_tree_oid (current_head) : - the_hash_algo->empty_tree, &tree)) { - res =1 ;/* run 'git commit' to display error message */ - goto out; +if (!(flags &ALLOW_EMPTY )) { +struct commit *first_parent = current_head; + +if (flags &AMEND_MSG ) { +if (current_head->parents) { + first_parent = current_head->parents->item; +if (repo_parse_commit (r, first_parent)) { + res =error (_ ("could not parse HEAD commit" )); + goto out; + } + }else { + first_parent =NULL ; + } + } +if (oideq (first_parent ?get_commit_tree_oid (first_parent) : + the_hash_algo->empty_tree, &tree)) { + res =1 ;/* run 'git commit' to display error message */ + goto out; + } }if (find_hook ("prepare-commit-msg" )) {
t/t3403-rebase-skip.sh +32 -0
@@ -7,6 +7,8 @@ test_description='git rebase --merge --skip tests' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh + # we assume the default git am -3 --skip strategy is tested independently # and always works :)
@@ -20,6 +22,13 @@ test_expect_success setup ' git commit -a -m "hello world" && echo goodbye >> hello && git commit -a -m "goodbye" && + git tag goodbye && + + git checkout --detach && + git checkout HEAD^ . && + test_tick && + git commit -m reverted-goodbye && + git tag reverted-goodbye && git checkout -f skip-reference && echo moo > hello &&
@@ -76,4 +85,27 @@ test_expect_success 'moved back to branch correctly' ' test_debug 'gitk --all & sleep 1' +test_expect_success 'fixup that empties commit fails' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \ + goodbye^ reverted-goodbye + ) +' + +test_expect_success 'squash that empties commit fails' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \ + goodbye^ reverted-goodbye + ) +' + +# Must be the last test in this file +test_expect_success '$EDITOR and friends are unchanged' ' + test_editor_unchanged +' + test_done
Signature · 1 line
gitgitgadget
Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
This fixes a regression introduced in
356ee4659b("sequencer: try to commit without forking 'git commit'", 2017-11-24). When amending a committry_to_commit()was using the wrong parent when checking if the commit would be empty. When amending we need to check against HEAD^ not HEAD.
Thanks. That makes sense.
If you compare with HEAD and find there is no difference, that would mean that the "amend" itself is a no-op (i.e. the user said they want to make changes, but after all changed their mind)? It might be something worth noticing, but certainly not in the same way as "we were told to skip an empty commit---is this one empty?" gets treated.
Hi Junio
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
This fixes a regression introduced in
356ee4659b("sequencer: try to commit without forking 'git commit'", 2017-11-24). When amending a committry_to_commit()was using the wrong parent when checking if the commit would be empty. When amending we need to check against HEAD^ not HEAD.
Quoted 8 lines
Thanks. That makes sense.
If you compare with HEAD and find there is no difference, that would mean that the "amend" itself is a no-op (i.e. the user said they want to make changes, but after all changed their mind)? It might be something worth noticing, but certainly not in the same way as "we were told to skip an empty commit---is this one empty?" gets treated.
Yes I'm trying to decide what to do about fixups and squashes that become empty. We'd need to do such a check in the sequencer before trying to commit I think.
Another thing to think about for the future is what message we want to display when a fixup makes the amended commit become empty. If there is another fixup following it then the commit may not end up empty by the time we've finished applying all the fixups for that commit. If the user runs 'reset HEAD^' after the first fixup we'll end up fixing up the wrong commit with the later fixups.
Best Wishes
Phillip
sequencer.c static int try_to_commit(struct repository *r, · line 1355
Quoted 2 lines · Phillip Wood via GitGitGadget
sequencer.c +2
-0+if (!(flags &ALLOW_EMPTY )) { +struct commit *first_parent = current_head;
I would have used first_parent_tree variable instead here. That way, you did not have to have an overlong ternary expression down there, I expect.
sequencer.c static int try_to_commit(struct repository *r, · line 1369
Quoted 13 lines
sequencer.c +13
-0+if (flags &AMEND_MSG ) { +if (current_head->parents) { + first_parent = current_head->parents->item; +if (repo_parse_commit (r, first_parent)) { + res =error (_ ("could not parse HEAD commit" )); + goto out; + } + }else { + first_parent =NULL ; + } + } +if (oideq (first_parent ?get_commit_tree_oid (first_parent) : + the_hash_algo->empty_tree, &tree)) {
Style. It often makes the code much easier to read when you strive to break lines at the boundary of larger syntactic units. In this (A ? B : C, D) parameter list, ternary A ? B : C binds much tighter than the comma after it, so if you are breaking line inside it, you should break line after the comma, too, i.e.
oideq(first_parent ? get_commit_tree_oid(first_parent) : the_hash_algo->empty_tree, &tree)
to avoid appearing if C and D have closer relationship than B and C, which your version gives.
But I hope that it becomes a moot point, if we computed first_parent_tree inside the new "if (flags & AMEND_MSG)" block to leave this oideq() just
if (oideq(first_parent_tree, &tree)) {Hi Junio
(sorry dscho I forgot to CC you on this patch)
Quoted 8 lines · Junio C Hamano
sequencer.c static int try_to_commit(struct repository *r, · line 1355
Quoted 2 lines · Phillip Wood via GitGitGadget
sequencer.c +2
-0+if (!(flags &ALLOW_EMPTY )) { +struct commit *first_parent = current_head;I would have used
first_parent_treevariable instead here. That way, you did not have to have an overlong ternary expression down there, I expect.
But current_head may be NULL so we'd end up with the equivalent of the ternary expression up here or an if/else to handle it. I thought it was clearer to find the parent we want to use and then get the tree from it.
Quoted 21 lines
Quoted 4 lines
sequencer.c +3
-0+if (flags &AMEND_MSG ) { +if (current_head->parents) { + first_parent = current_head->parents->item;sequencer.c static int try_to_commit(struct repository *r, · line 1369
Quoted 10 lines
sequencer.c +10
-0+if (repo_parse_commit (r, first_parent)) { + res =error (_ ("could not parse HEAD commit" )); + goto out; + } + }else { + first_parent =NULL ; + } + } +if (oideq (first_parent ?get_commit_tree_oid (first_parent) : + the_hash_algo->empty_tree, &tree)) {Style. It often makes the code much easier to read when you strive to break lines at the boundary of larger syntactic units. In this (A ? B : C, D) parameter list, ternary A ? B : C binds much tighter than the comma after it, so if you are breaking line inside it, you should break line after the comma, too, i.e.
Quoted 4 lines
oideq(first_parent ? get_commit_tree_oid(first_parent) : the_hash_algo->empty_tree, &tree)
I agree that's a clearer way of writing it. I'll re-roll with that
Best Wishes
Phillip
Quoted 9 lines
to avoid appearing if C and D have closer relationship than B and C, which your version gives.
But I hope that it becomes a moot point, if we computed
first_parent_treeinside the new "if (flags &AMEND_MSG)" block to leave thisoideq()justif (oideq(first_parent_tree, &tree)) {
v2 cover sequencer: fix empty commit check when amending
In reply to Phillip Wood via GitGitGadgetThanks for the comments, I've updated the patch with the style change Junio suggested
I noticed this while adding some tests for a re-roll of js/advise-rebase-skip
This patch is based on pw/post-commit-from-sequencer
Phillip Wood (1):
sequencer: fix empty commit check when amending
sequencer.c | 26 +++++++++++++++++++++----- t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-)
base-commit: 4627bc777e9ade5e3a85d6b8e8630fc4b6e2f8f6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-467%2Fphillipwood%2Fwip%2Frebase-fix-empty-commit-check-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-467/phillipwood/wip/rebase-fix-empty-commit-check-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/467
Range-diff vs v1:
1: 7d34c0ff80 ! 1: 037f2b2975 sequencer: fix empty commit check when amending
@@ -42,8 +42,10 @@
+ first_parent = NULL;
+ }
+ }
-+ if (oideq(first_parent ? get_commit_tree_oid(first_parent) :
-+ the_hash_algo->empty_tree, &tree)) {
++ if (oideq(first_parent
++ ? get_commit_tree_oid(first_parent)
++ : the_hash_algo->empty_tree,
++ &tree)) {
+ res = 1; /* run 'git commit' to display error message */
+ goto out;
+ }Signature · 1 line
gitgitgadget
PATCH v2 1/1 sequencer: fix empty commit check when amending
From: Phillip Wood <[email protected]>
This fixes a regression introduced in 356ee4659b ("sequencer: try to commit without forking 'git commit'", 2017-11-24). When amending a commit try_to_commit() was using the wrong parent when checking if the commit would be empty. When amending we need to check against HEAD^ not HEAD.
t3403 may not seem like the natural home for the new tests but a further patch series will improve the advice printed by git commit. That series will mutate these tests to check that the advice includes suggesting rebase --skip to skip the fixup that would empty the commit.
--- sequencer.c | 26 +++++++++++++++++++++----- t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-)
sequencer.c +21 -5
@@ -1351,11 +1351,27 @@ static int try_to_commit(struct repository *r, goto out; } -if (!(flags &ALLOW_EMPTY ) &&oideq (current_head ? -get_commit_tree_oid (current_head) : - the_hash_algo->empty_tree, &tree)) { - res =1 ;/* run 'git commit' to display error message */ - goto out; +if (!(flags &ALLOW_EMPTY )) { +struct commit *first_parent = current_head; + +if (flags &AMEND_MSG ) { +if (current_head->parents) { + first_parent = current_head->parents->item; +if (repo_parse_commit (r, first_parent)) { + res =error (_ ("could not parse HEAD commit" )); + goto out; + } + }else { + first_parent =NULL ; + } + } +if (oideq (first_parent + ?get_commit_tree_oid (first_parent) + : the_hash_algo->empty_tree, + &tree)) { + res =1 ;/* run 'git commit' to display error message */ + goto out; + } }if (find_hook ("prepare-commit-msg" )) {
t/t3403-rebase-skip.sh +32 -0
@@ -7,6 +7,8 @@ test_description='git rebase --merge --skip tests' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh + # we assume the default git am -3 --skip strategy is tested independently # and always works :)
@@ -20,6 +22,13 @@ test_expect_success setup ' git commit -a -m "hello world" && echo goodbye >> hello && git commit -a -m "goodbye" && + git tag goodbye && + + git checkout --detach && + git checkout HEAD^ . && + test_tick && + git commit -m reverted-goodbye && + git tag reverted-goodbye && git checkout -f skip-reference && echo moo > hello &&
@@ -76,4 +85,27 @@ test_expect_success 'moved back to branch correctly' ' test_debug 'gitk --all & sleep 1' +test_expect_success 'fixup that empties commit fails' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \ + goodbye^ reverted-goodbye + ) +' + +test_expect_success 'squash that empties commit fails' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \ + goodbye^ reverted-goodbye + ) +' + +# Must be the last test in this file +test_expect_success '$EDITOR and friends are unchanged' ' + test_editor_unchanged +' + test_done
Signature · 1 line
gitgitgadget
Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
This fixes a regression introduced in
356ee4659b("sequencer: try to commit without forking 'git commit'", 2017-11-24). When amending a committry_to_commit()was using the wrong parent when checking if the commit would be empty. When amending we need to check against HEAD^ not HEAD.
Thanks. Will queue.
sequencer.c static int try_to_commit(struct repository *r, · line 1358
Quoted 5 lines · Phillip Wood via GitGitGadget
sequencer.c +5
-0+if (!(flags &ALLOW_EMPTY )) { +struct commit *first_parent = current_head; + +if (flags &AMEND_MSG ) { +if (current_head->parents) {
It is not apparent to me that somebody guarantees that this access is safe; would we need to do things differently when !current_head?
Quoted 9 lines
sequencer.c +9
-0+ first_parent = current_head->parents->item; +if (repo_parse_commit (r, first_parent)) { + res =error (_ ("could not parse HEAD commit" )); + goto out; + } + }else { + first_parent =NULL ; + } + }
Hi Junio
Quoted 10 lines · Junio C Hamano
sequencer.c static int try_to_commit(struct repository *r, · line 1358
Quoted 5 lines · Phillip Wood via GitGitGadget
sequencer.c +5
-0+if (!(flags &ALLOW_EMPTY )) { +struct commit *first_parent = current_head; + +if (flags &AMEND_MSG ) { +if (current_head->parents) {It is not apparent to me that somebody guarantees that this access is safe; would we need to do things differently when !current_head?
That's a good point, I'll fix it, thanks for catching this
Best Wishes
Phillip
Quoted 10 lines
Quoted 10 lines
sequencer.c +9
-0+ first_parent = current_head->parents->item; +if (repo_parse_commit (r, first_parent)) { + res =error (_ ("could not parse HEAD commit" )); + goto out; + } + }else { + first_parent =NULL ; + } + }
Hi Junio
Quoted 13 lines · Phillip Wood
Hi Junio
Quoted 10 lines · Junio C Hamano
Quoted 5 lines · Phillip Wood via GitGitGadget
+ if (!(flags &
ALLOW_EMPTY)) {
+ struct commit*first_parent=current_head; +
+ if (flags &AMEND_MSG) {
+ if (current_head->parents) {It is not apparent to me that somebody guarantees that this access is safe; would we need to do things differently when !current_head?
We do actually check that there is a valid HEAD before we try to fixup a commit. Though perhaps we should still change this patch as HEAD may be changed by another process between that check and re-reading it here. If you try to fixup a commit without a valid HEAD you get
error: need a HEAD to fixup
hint: Could not execute the todo command
hint:
hint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint: git rebase --edit-todo hint: git rebase --continue error: could not copy '.git/rebase-merge/message-squash' to '.git/rebase-merge/message'
The last error message is unfortunate but we do exit in an orderly manner rather than segfaulting. It's a bit tricky to trigger this (there isn't a test at the moment) but something like this does it
t/t3404-rebase-interactive.sh +15 -0
@@ -67,6 +67,21 @@ test_expect_success 'setup' ' SHELL= export SHELL +test_expect_success 'fixup on orphan branch errors out' ' + + test_when_finished "git switch master" && + write_script switch-branch.sh <<-\EOF && + git symbolic-ref HEAD refs/heads/does-not-exist && + git rm -rf . + EOF + ( + set_fake_editor && + FAKE_LINES="exec_./switch-branch.sh \ + fixup 1" git rebase -i HEAD^ + ) && + test_pause +' +
I think it would be useful to add something like this to the test suite (changed to check the error message, with a better name for the script and modified to expect failure) What do you think?
Best Wishes
Phillip
Quoted 16 lines
That's a good point, I'll fix it, thanks for catching this
Best Wishes
Phillip
Quoted 10 lines
Quoted 10 lines
+
first_parent=current_head->parents->item; + if (repo_parse_commit(r, first_parent)) { + res =error(_("could not parse HEAD commit")); + goto out;
+ }
+ } else {
+first_parent= NULL;
+ }
+ }
Quoted 13 lines · Phillip Wood
We do actually check that there is a valid HEAD before we try to fixup a commit. Though perhaps we should still change this patch as HEAD may be changed by another process between that check and re-reading it here. If you try to fixup a commit without a valid HEAD you get
error: need a HEAD to fixup
hint: Could not execute the todo command
hint:
hint: fixupfaef1a5a7637ff91b3611aabd1b96541da5f5536P
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
Quoted 7 lines
hint: git rebase --edit-todo hint: git rebase --continue error: could not copy '.git/rebase-merge/message-squash' to'.git/rebase-merge/message'
The last error message is unfortunate but we do exit in an orderly manner rather than segfaulting.
Thanks for thinking about the issue further.
Quoted 5 lines
t/t3404-rebase-interactive.sh +0
-0
t/t3404-rebase-interactive.sh test_expect_success 'setup' ' · line 75
Quoted 9 lines
t/t3404-rebase-interactive.sh +6
-0@@ -67,6 +67,21 @@ test_expect_success 'setup' ' SHELL= export SHELL +test_expect_success 'fixup on orphan branch errors out' ' + + test_when_finished "git switch master" && + write_script switch-branch.sh <<-\EOF && + git symbolic-ref HEAD refs/heads/does-not-exist && + git rm -rf .
That "git rm -rf ." scares me, though.
Quoted 10 lines
t/t3404-rebase-interactive.sh +9
-0+ EOF + ( + set_fake_editor && + FAKE_LINES="exec_./switch-branch.sh \ + fixup 1" git rebase -i HEAD^ + ) && + test_pause +' +
Quoted 3 lines
I think it would be useful to add something like this to the test suite (changed to check the error message, with a better name for the script and modified to expect failure) What do you think?
So, we try an interactive rebase, try to apply a fix-up on an unborn branch and expect it to fail in a controlled way, something like
( # we are in subshell so freely export set_fake_editor && export FAKE_LINES="exec_./switch-branch.sh fixup 1" && test_must_fail git rebase -i HEAD^ 2>error && test_i18ngrep "... what we expect ..." error )
Perhaps. I do not think of a good reason why we should allow switching to another branch when "rebase -i" gives control back to the user, so in the longer run, the checked condition may not stay the same (I suspect you would catch "does-not-exist is unborn and there is nothing to 'fixup'" right now---I am envisioning that the condition that is checked and the message we would issue would be "we gave you a detached HEAD for a good reason---stay there and do not switch to any other branch") and the message expected by this test would have to be updated.
Thanks.
Quoted 33 lines · Junio C Hamano
Quoted 13 lines · Phillip Wood
We do actually check that there is a valid HEAD before we try to fixup a commit. Though perhaps we should still change this patch as HEAD may be changed by another process between that check and re-reading it here. If you try to fixup a commit without a valid HEAD you get
error: need a HEAD to fixup
hint: Could not execute the todo command
hint:
hint: fixupfaef1a5a7637ff91b3611aabd1b96541da5f5536P
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:Quoted 7 lines
hint: git rebase --edit-todo hint: git rebase --continue error: could not copy '.git/rebase-merge/message-squash' to'.git/rebase-merge/message'
The last error message is unfortunate but we do exit in an orderly manner rather than segfaulting.
Thanks for thinking about the issue further.
Quoted 8 lines
t/t3404-rebase-interactive.sh +0
-0@@ -67,6 +67,21 @@ test_expect_success 'setup' ' SHELL= export SHELL
Quoted 8 lines
t/t3404-rebase-interactive.sh test_expect_success 'setup' ' · line 75
Quoted 6 lines
t/t3404-rebase-interactive.sh +6
-0+test_expect_success 'fixup on orphan branch errors out' ' + + test_when_finished "git switch master" && + write_script switch-branch.sh <<-\EOF && + git symbolic-ref HEAD refs/heads/does-not-exist && + git rm -rf .That "git rm -rf ." scares me, though.
I know I'm not too keen on it my self but we need to empty the worktree and index if we're going to switch to an unborn branch
Quoted 26 lines
Quoted 11 lines
t/t3404-rebase-interactive.sh +9
-0+ EOF + ( + set_fake_editor && + FAKE_LINES="exec_./switch-branch.sh \ + fixup 1" git rebase -i HEAD^ + ) && + test_pause +' +Quoted 3 lines
I think it would be useful to add something like this to the test suite (changed to check the error message, with a better name for the script and modified to expect failure) What do you think?
So, we try an interactive rebase, try to apply a fix-up on an unborn branch and expect it to fail in a controlled way, something like
( # we are in subshell so freely export set_fake_editor && export FAKE_LINES="exec_./switch-branch.sh fixup 1" && test_must_fail git rebase -i HEAD^ 2>error && test_i18ngrep "... what we expect ..." error )
Quoted 9 lines
Perhaps. I do not think of a good reason why we should allow switching to another branch when "rebase -i" gives control back to the user, so in the longer run, the checked condition may not stay the same (I suspect you would catch "does-not-exist is unborn and there is nothing to 'fixup'" right now---I am envisioning that the condition that is checked and the message we would issue would be "we gave you a detached HEAD for a good reason---stay there and do not switch to any other branch") and the message expected by this test would have to be updated.
I agree there's no good reason for a user to do this. 'git switch' will refuse to switch branches during a rebase for that reason. At the moment we check HEAD with get_oid() so that would need changing to check if the user has switched to another branch (perhaps it could be done when we check that the index and worktree are clean after running an exec command).
Best Wishes
Phillip
Quoted 5 lines
Thanks.
Hi Phillip,
Quoted 44 lines · Phillip Wood
Quoted 34 lines · Junio C Hamano
Quoted 14 lines · Phillip Wood
We do actually check that there is a valid HEAD before we try to fixup a commit. Though perhaps we should still change this patch as HEAD may be changed by another process between that check and re-reading it here. If you try to fixup a commit without a valid HEAD you get
error: need a HEAD to fixup
hint: Could not execute the todo command
hint:
hint: fixupfaef1a5a7637ff91b3611aabd1b96541da5f5536P
hint:
hint: It has been rescheduled; To edit the command before continuing,
hint: please
hint: edit the todo list first:
hint:Quoted 7 lines
hint: git rebase --edit-todo hint: git rebase --continue error: could not copy '.git/rebase-merge/message-squash' to'.git/rebase-merge/message'
The last error message is unfortunate but we do exit in an orderly manner rather than segfaulting.
Thanks for thinking about the issue further.
Quoted 8 lines
t/t3404-rebase-interactive.sh +0
-0@@ -67,6 +67,21 @@ test_expect_success 'setup' ' SHELL= export SHELLQuoted 8 lines
t/t3404-rebase-interactive.sh test_expect_success 'setup' ' · line 75
Quoted 6 lines
t/t3404-rebase-interactive.sh +6
-0+test_expect_success 'fixup on orphan branch errors out' ' + + test_when_finished "git switch master" && + write_script switch-branch.sh <<-\EOF && + git symbolic-ref HEAD refs/heads/does-not-exist && + git rm -rf .That "git rm -rf ." scares me, though.
Quoted 2 lines
I know I'm not too keen on it my self but we need to empty the worktree and index if we're going to switch to an unborn branch
How about git worktree --orphan does-not-exist unborn?
Ciao,
Dscho
Quoted 8 lines · Johannes Schindelin
Quoted 5 lines · Phillip Wood
Quoted 1 line · Junio C Hamano
That "git rm -rf ." scares me, though.
I know I'm not too keen on it my self but we need to empty the worktree and index if we're going to switch to an unborn branch
How about
git worktree --orphan does-not-exist unborn?
git-worktree doesn't presently recognize --orphan, though it would be nice if it did. In fact, I clearly was thinking of --orphan (along with -b, -B, and --detach), when I wrote the implementation, as can be seen from the commentary in one of the original patches[1]. That --orphan never got added was either due to an oversight or it was one of those "we'll add it when someone actually needs it" deals.
Hi Eric,
Quoted 11 lines · Eric Sunshine
Quoted 8 lines · Johannes Schindelin
Quoted 5 lines · Phillip Wood
Quoted 1 line · Junio C Hamano
That "git rm -rf ." scares me, though.
I know I'm not too keen on it my self but we need to empty the worktree and index if we're going to switch to an unborn branch
How about
git worktree --orphan does-not-exist unborn?
Quoted 8 lines
git-worktree doesn't presently recognize --orphan, though it would be nice if it did. In fact, I clearly was thinking of --orphan (along with -b, -B, and --detach), when I wrote the implementation, as can be seen from the commentary in one of the original patches[1]. That --orphan never got added was either due to an oversight or it was one of those "we'll add it when someone actually needs it" deals.
You're absolutely correct, of course. I actually had looked at the output of git checkout -h instead of git worktree -h... And checkout does have that --orphan option.
But from the documentation at
https://git-scm.com/docs/git-checkout#Documentation/git-checkout.txt---orphanltnewbranchgt
I see that the command I had in mind does not work as I expected it to:git checkout --orphan new-branch $EMPTY_TREE will fail with
fatal: Cannot switch branch to a non-commit '4b825dc642cb6eb9a060e54bf8d69288fbee4904' (and the documentation of the --orphan option also suggests to use git rm -rf for the use case under discussion, so there...)
Sorry for the noise,
Dscho
Hi Dscho
Quoted 48 lines · Johannes Schindelin
Hi Phillip,
Quoted 44 lines · Phillip Wood
Quoted 42 lines · Junio C Hamano
Quoted 14 lines · Phillip Wood
We do actually check that there is a valid HEAD before we try to fixup a commit. Though perhaps we should still change this patch as HEAD may be changed by another process between that check and re-reading it here. If you try to fixup a commit without a valid HEAD you get
error: need a HEAD to fixup
hint: Could not execute the todo command
hint:
hint: fixupfaef1a5a7637ff91b3611aabd1b96541da5f5536P
hint:
hint: It has been rescheduled; To edit the command before continuing,
hint: please
hint: edit the todo list first:
hint:Quoted 7 lines
hint: git rebase --edit-todo hint: git rebase --continue error: could not copy '.git/rebase-merge/message-squash' to'.git/rebase-merge/message'
The last error message is unfortunate but we do exit in an orderly manner rather than segfaulting.
Thanks for thinking about the issue further.
Quoted 5 lines
t/t3404-rebase-interactive.sh +0
-0t/t3404-rebase-interactive.sh test_expect_success 'setup' ' · line 75
Quoted 9 lines
t/t3404-rebase-interactive.sh +6
-0@@ -67,6 +67,21 @@ test_expect_success 'setup' ' SHELL= export SHELL +test_expect_success 'fixup on orphan branch errors out' ' + + test_when_finished "git switch master" && + write_script switch-branch.sh <<-\EOF && + git symbolic-ref HEAD refs/heads/does-not-exist && + git rm -rf .That "git rm -rf ." scares me, though.
Quoted 4 lines
Quoted 2 lines
I know I'm not too keen on it my self but we need to empty the worktree and index if we're going to switch to an unborn branch
How about
git worktree --orphan does-not-exist unborn?
I'm trying to create the unborn branch in the current worktree as that is where the rebase is happening
Best Wishes
Phillip
Quoted 4 lines
Ciao,
Dscho
Quoted 11 lines · Phillip Wood
Quoted 8 lines
t/t3404-rebase-interactive.sh test_expect_success 'setup' ' · line 75
Quoted 6 lines
t/t3404-rebase-interactive.sh +6
-0+test_expect_success 'fixup on orphan branch errors out' ' + + test_when_finished "git switch master" && + write_script switch-branch.sh <<-\EOF && + git symbolic-ref HEAD refs/heads/does-not-exist && + git rm -rf .That "git rm -rf ." scares me, though.
I know I'm not too keen on it my self but we need to empty the worktree and index if we're going to switch to an unborn branch
"checkout --orphan" takes you to an unborn branch. If you need to also be with no contents in the index and nothing in the working tree, then "git rm -r ." may be needed, but applying a fixup without the target content does sound like asking for a conflict already.