{"thread":{"id":"52305","subject":"[PATCH 0/1] sequencer: fix empty commit check when amending","startedAt":"2019-11-21T14:06:25Z","lastAt":"2019-11-26T01:13:23Z","messageCount":19,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Phillip Wood","Johannes Schindelin","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"386726","messageId":"pull.467.git.1574345181.gitgitgadget@gmail.com","threadId":"52305","inReplyTo":null,"subject":"[PATCH 0/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T14:06:20Z","receivedAt":"2019-11-21T14:06:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"I noticed this while adding some tests for a re-roll of\njs/advise-rebase-skip\n\nThis patch is based on pw/post-commit-from-sequencer\n\nPhillip Wood (1):\n sequencer: fix empty commit check when amending\n\n sequencer.c | 24 +++++++++++++++++++-----\n t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 5 deletions(-)\n\n\nbase-commit: 4627bc777e9ade5e3a85d6b8e8630fc4b6e2f8f6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-467%2Fphillipwood%2Fwip%2Frebase-fix-empty-commit-check-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-467/phillipwood/wip/rebase-fix-empty-commit-check-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/467\n-- \ngitgitgadget\n"},{"id":"386727","messageId":"7d34c0ff805b8637b23d0ef0045370dfc931a3af.1574345181.git.gitgitgadget@gmail.com","threadId":"52305","inReplyTo":"pull.467.git.1574345181.gitgitgadget@gmail.com","subject":"[PATCH 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T14:06:21Z","receivedAt":"2019-11-21T14:06:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis fixes a regression introduced in 356ee4659b (\"sequencer: try to\ncommit without forking 'git commit'\", 2017-11-24). When amending a\ncommit try_to_commit() was using the wrong parent when checking if the\ncommit would be empty. When amending we need to check against HEAD^ not\nHEAD.\n\nt3403 may not seem like the natural home for the new tests but a further\npatch series will improve the advice printed by `git commit`. That\nseries will mutate these tests to check that the advice includes\nsuggesting `rebase --skip` to skip the fixup that would empty the\ncommit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 24 +++++++++++++++++++-----\n t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex da2decbd3a..4c8938ae46 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1351,11 +1351,25 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (!(flags & ALLOW_EMPTY) && oideq(current_head ?\n-\t\t\t\t\t get_commit_tree_oid(current_head) :\n-\t\t\t\t\t the_hash_algo->empty_tree, &tree)) {\n-\t\tres = 1; /* run 'git commit' to display error message */\n-\t\tgoto out;\n+\tif (!(flags & ALLOW_EMPTY)) {\n+\t\tstruct commit *first_parent = current_head;\n+\n+\t\tif (flags & AMEND_MSG) {\n+\t\t\tif (current_head->parents) {\n+\t\t\t\tfirst_parent = current_head->parents->item;\n+\t\t\t\tif (repo_parse_commit(r, first_parent)) {\n+\t\t\t\t\tres = error(_(\"could not parse HEAD commit\"));\n+\t\t\t\t\tgoto out;\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\tfirst_parent = NULL;\n+\t\t\t}\n+\t\t}\n+\t\tif (oideq(first_parent ? get_commit_tree_oid(first_parent) :\n+\t\t\t\t\t the_hash_algo->empty_tree, &tree)) {\n+\t\t\tres = 1; /* run 'git commit' to display error message */\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \tif (find_hook(\"prepare-commit-msg\")) {\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex 1f5122b632..ee8a8dba52 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -7,6 +7,8 @@ test_description='git rebase --merge --skip tests'\n \n . ./test-lib.sh\n \n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n+\n # we assume the default git am -3 --skip strategy is tested independently\n # and always works :)\n \n@@ -20,6 +22,13 @@ test_expect_success setup '\n \tgit commit -a -m \"hello world\" &&\n \techo goodbye >> hello &&\n \tgit commit -a -m \"goodbye\" &&\n+\tgit tag goodbye &&\n+\n+\tgit checkout --detach &&\n+\tgit checkout HEAD^ . &&\n+\ttest_tick &&\n+\tgit commit -m reverted-goodbye &&\n+\tgit tag reverted-goodbye &&\n \n \tgit checkout -f skip-reference &&\n \techo moo > hello &&\n@@ -76,4 +85,27 @@ test_expect_success 'moved back to branch correctly' '\n \n test_debug 'gitk --all & sleep 1'\n \n+test_expect_success 'fixup that empties commit fails' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"1 fixup 2\" git rebase -i \\\n+\t\t\tgoodbye^ reverted-goodbye\n+\t)\n+'\n+\n+test_expect_success 'squash that empties commit fails' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"1 squash 2\" git rebase -i \\\n+\t\t\tgoodbye^ reverted-goodbye\n+\t)\n+'\n+\n+# Must be the last test in this file\n+test_expect_success '$EDITOR and friends are unchanged' '\n+\ttest_editor_unchanged\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"386801","messageId":"xmqqh82w4do6.fsf@gitster-ct.c.googlers.com","threadId":"52305","inReplyTo":"7d34c0ff805b8637b23d0ef0045370dfc931a3af.1574345181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] sequencer: fix empty commit check when amending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T06:40:09Z","receivedAt":"2019-11-22T06:40:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n>\n> This fixes a regression introduced in 356ee4659b (\"sequencer: try to\n> commit without forking 'git commit'\", 2017-11-24). When amending a\n> commit try_to_commit() was using the wrong parent when checking if the\n> commit would be empty. When amending we need to check against HEAD^ not\n> HEAD.\n\nThanks. That makes sense.\n\nIf you compare with HEAD and find there is no difference, that would\nmean that the \"amend\" itself is a no-op (i.e. the user said they\nwant to make changes, but after all changed their mind)? It might\nbe something worth noticing, but certainly not in the same way as\n\"we were told to skip an empty commit---is this one empty?\" gets\ntreated.\n"},{"id":"386802","messageId":"xmqqblt44d3h.fsf@gitster-ct.c.googlers.com","threadId":"52305","inReplyTo":"7d34c0ff805b8637b23d0ef0045370dfc931a3af.1574345181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] sequencer: fix empty commit check when amending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T06:52:34Z","receivedAt":"2019-11-22T06:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> +\tif (!(flags & ALLOW_EMPTY)) {\n> +\t\tstruct commit *first_parent = current_head;\n\nI would have used first_parent_tree variable instead here. That\nway, you did not have to have an overlong ternary expression down\nthere, I expect.\n\n> +\t\tif (flags & AMEND_MSG) {\n> +\t\t\tif (current_head->parents) {\n> +\t\t\t\tfirst_parent = current_head->parents->item;\n> +\t\t\t\tif (repo_parse_commit(r, first_parent)) {\n> +\t\t\t\t\tres = error(_(\"could not parse HEAD commit\"));\n> +\t\t\t\t\tgoto out;\n> +\t\t\t\t}\n> +\t\t\t} else {\n> +\t\t\t\tfirst_parent = NULL;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (oideq(first_parent ? get_commit_tree_oid(first_parent) :\n> +\t\t\t\t\t the_hash_algo->empty_tree, &tree)) {\n\nStyle. It often makes the code much easier to read when you strive\nto break lines at the boundary of larger syntactic units. In this\n(A ? B : C, D) parameter list, ternary A ? B : C binds much tighter\nthan the comma after it, so if you are breaking line inside it, you\nshould break line after the comma, too, i.e.\n\n\toideq(first_parent\n\t ? get_commit_tree_oid(first_parent)\n\t : the_hash_algo->empty_tree,\n\t &tree)\n\nto avoid appearing if C and D have closer relationship than B and C,\nwhich your version gives.\n\nBut I hope that it becomes a moot point, if we computed first_parent_tree\ninside the new \"if (flags & AMEND_MSG)\" block to leave this oideq()\njust\n\n\tif (oideq(first_parent_tree, &tree)) {\n\n"},{"id":"386813","messageId":"b314c0b1-0e01-c8ea-bef2-7d1191b9080a@gmail.com","threadId":"52305","inReplyTo":"xmqqblt44d3h.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-22T11:09:49Z","receivedAt":"2019-11-22T11:09:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\n(sorry dscho I forgot to CC you on this patch)\n\nOn 22/11/2019 06:52, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> +\tif (!(flags & ALLOW_EMPTY)) {\n>> +\t\tstruct commit *first_parent = current_head;\n> \n> I would have used first_parent_tree variable instead here. That\n> way, you did not have to have an overlong ternary expression down\n> there, I expect.\n\nBut current_head may be NULL so we'd end up with the equivalent of the \nternary expression up here or an if/else to handle it. I thought it was \nclearer to find the parent we want to use and then get the tree from it.\n\n> \n>> +\t\tif (flags & AMEND_MSG) {\n>> +\t\t\tif (current_head->parents) {\n>> +\t\t\t\tfirst_parent = current_head->parents->item;\n>> +\t\t\t\tif (repo_parse_commit(r, first_parent)) {\n>> +\t\t\t\t\tres = error(_(\"could not parse HEAD commit\"));\n>> +\t\t\t\t\tgoto out;\n>> +\t\t\t\t}\n>> +\t\t\t} else {\n>> +\t\t\t\tfirst_parent = NULL;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t\tif (oideq(first_parent ? get_commit_tree_oid(first_parent) :\n>> +\t\t\t\t\t the_hash_algo->empty_tree, &tree)) {\n> \n> Style. It often makes the code much easier to read when you strive\n> to break lines at the boundary of larger syntactic units. In this\n> (A ? B : C, D) parameter list, ternary A ? B : C binds much tighter\n> than the comma after it, so if you are breaking line inside it, you\n> should break line after the comma, too, i.e.\n> \n> \toideq(first_parent\n> \t ? get_commit_tree_oid(first_parent)\n> \t : the_hash_algo->empty_tree,\n> \t &tree)\n\nI agree that's a clearer way of writing it. I'll re-roll with that\n\nBest Wishes\n\nPhillip\n\n> to avoid appearing if C and D have closer relationship than B and C,\n> which your version gives.\n> \n> But I hope that it becomes a moot point, if we computed first_parent_tree\n> inside the new \"if (flags & AMEND_MSG)\" block to leave this oideq()\n> just\n> \n> \tif (oideq(first_parent_tree, &tree)) {\n> \n"},{"id":"386814","messageId":"0c393fc0-b9d7-66d3-4562-f5278f8a5b59@gmail.com","threadId":"52305","inReplyTo":"xmqqh82w4do6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-22T11:01:51Z","receivedAt":"2019-11-22T11:10:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 22/11/2019 06:40, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> From: Phillip Wood \n>>\n>> This fixes a regression introduced in 356ee4659b (\"sequencer: try to\n>> commit without forking 'git commit'\", 2017-11-24). When amending a\n>> commit try_to_commit() was using the wrong parent when checking if the\n>> commit would be empty. When amending we need to check against HEAD^ not\n>> HEAD.\n> \n> Thanks. That makes sense.\n> \n> If you compare with HEAD and find there is no difference, that would\n> mean that the \"amend\" itself is a no-op (i.e. the user said they\n> want to make changes, but after all changed their mind)? It might\n> be something worth noticing, but certainly not in the same way as\n> \"we were told to skip an empty commit---is this one empty?\" gets\n> treated.\n\nYes I'm trying to decide what to do about fixups and squashes that \nbecome empty. We'd need to do such a check in the sequencer before \ntrying to commit I think.\n\nAnother thing to think about for the future is what message we want to \ndisplay when a fixup makes the amended commit become empty. If there is \nanother fixup following it then the commit may not end up empty by the \ntime we've finished applying all the fixups for that commit. If the user \nruns 'reset HEAD^' after the first fixup we'll end up fixing up the \nwrong commit with the later fixups.\n\nBest Wishes\n\nPhillip\n"},{"id":"386860","messageId":"pull.467.v2.git.1574451783.gitgitgadget@gmail.com","threadId":"52305","inReplyTo":"pull.467.git.1574345181.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-22T19:43:02Z","receivedAt":"2019-11-22T19:43:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks for the comments, I've updated the patch with the style change Junio\nsuggested\n\nI noticed this while adding some tests for a re-roll of\njs/advise-rebase-skip\n\nThis patch is based on pw/post-commit-from-sequencer\n\nPhillip Wood (1):\n sequencer: fix empty commit check when amending\n\n sequencer.c | 26 +++++++++++++++++++++-----\n t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 5 deletions(-)\n\n\nbase-commit: 4627bc777e9ade5e3a85d6b8e8630fc4b6e2f8f6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-467%2Fphillipwood%2Fwip%2Frebase-fix-empty-commit-check-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-467/phillipwood/wip/rebase-fix-empty-commit-check-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/467\n\nRange-diff vs v1:\n\n 1: 7d34c0ff80 ! 1: 037f2b2975 sequencer: fix empty commit check when amending\n @@ -42,8 +42,10 @@\n +\t\t\t\tfirst_parent = NULL;\n +\t\t\t}\n +\t\t}\n -+\t\tif (oideq(first_parent ? get_commit_tree_oid(first_parent) :\n -+\t\t\t\t\t the_hash_algo->empty_tree, &tree)) {\n ++\t\tif (oideq(first_parent\n ++\t\t\t ? get_commit_tree_oid(first_parent)\n ++\t\t\t : the_hash_algo->empty_tree,\n ++\t\t\t &tree)) {\n +\t\t\tres = 1; /* run 'git commit' to display error message */\n +\t\t\tgoto out;\n +\t\t}\n\n-- \ngitgitgadget\n"},{"id":"386861","messageId":"037f2b2975e06847443aef46939e3c712053dedf.1574451783.git.gitgitgadget@gmail.com","threadId":"52305","inReplyTo":"pull.467.v2.git.1574451783.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-22T19:43:03Z","receivedAt":"2019-11-22T19:43:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis fixes a regression introduced in 356ee4659b (\"sequencer: try to\ncommit without forking 'git commit'\", 2017-11-24). When amending a\ncommit try_to_commit() was using the wrong parent when checking if the\ncommit would be empty. When amending we need to check against HEAD^ not\nHEAD.\n\nt3403 may not seem like the natural home for the new tests but a further\npatch series will improve the advice printed by `git commit`. That\nseries will mutate these tests to check that the advice includes\nsuggesting `rebase --skip` to skip the fixup that would empty the\ncommit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 26 +++++++++++++++++++++-----\n t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex da2decbd3a..f4f81cbddc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1351,11 +1351,27 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (!(flags & ALLOW_EMPTY) && oideq(current_head ?\n-\t\t\t\t\t get_commit_tree_oid(current_head) :\n-\t\t\t\t\t the_hash_algo->empty_tree, &tree)) {\n-\t\tres = 1; /* run 'git commit' to display error message */\n-\t\tgoto out;\n+\tif (!(flags & ALLOW_EMPTY)) {\n+\t\tstruct commit *first_parent = current_head;\n+\n+\t\tif (flags & AMEND_MSG) {\n+\t\t\tif (current_head->parents) {\n+\t\t\t\tfirst_parent = current_head->parents->item;\n+\t\t\t\tif (repo_parse_commit(r, first_parent)) {\n+\t\t\t\t\tres = error(_(\"could not parse HEAD commit\"));\n+\t\t\t\t\tgoto out;\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\tfirst_parent = NULL;\n+\t\t\t}\n+\t\t}\n+\t\tif (oideq(first_parent\n+\t\t\t ? get_commit_tree_oid(first_parent)\n+\t\t\t : the_hash_algo->empty_tree,\n+\t\t\t &tree)) {\n+\t\t\tres = 1; /* run 'git commit' to display error message */\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \tif (find_hook(\"prepare-commit-msg\")) {\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex 1f5122b632..ee8a8dba52 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -7,6 +7,8 @@ test_description='git rebase --merge --skip tests'\n \n . ./test-lib.sh\n \n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n+\n # we assume the default git am -3 --skip strategy is tested independently\n # and always works :)\n \n@@ -20,6 +22,13 @@ test_expect_success setup '\n \tgit commit -a -m \"hello world\" &&\n \techo goodbye >> hello &&\n \tgit commit -a -m \"goodbye\" &&\n+\tgit tag goodbye &&\n+\n+\tgit checkout --detach &&\n+\tgit checkout HEAD^ . &&\n+\ttest_tick &&\n+\tgit commit -m reverted-goodbye &&\n+\tgit tag reverted-goodbye &&\n \n \tgit checkout -f skip-reference &&\n \techo moo > hello &&\n@@ -76,4 +85,27 @@ test_expect_success 'moved back to branch correctly' '\n \n test_debug 'gitk --all & sleep 1'\n \n+test_expect_success 'fixup that empties commit fails' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"1 fixup 2\" git rebase -i \\\n+\t\t\tgoodbye^ reverted-goodbye\n+\t)\n+'\n+\n+test_expect_success 'squash that empties commit fails' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"1 squash 2\" git rebase -i \\\n+\t\t\tgoodbye^ reverted-goodbye\n+\t)\n+'\n+\n+# Must be the last test in this file\n+test_expect_success '$EDITOR and friends are unchanged' '\n+\ttest_editor_unchanged\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"386890","messageId":"xmqqa78n2vuv.fsf@gitster-ct.c.googlers.com","threadId":"52305","inReplyTo":"037f2b2975e06847443aef46939e3c712053dedf.1574451783.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-23T02:02:32Z","receivedAt":"2019-11-23T02:02:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n>\n> This fixes a regression introduced in 356ee4659b (\"sequencer: try to\n> commit without forking 'git commit'\", 2017-11-24). When amending a\n> commit try_to_commit() was using the wrong parent when checking if the\n> commit would be empty. When amending we need to check against HEAD^ not\n> HEAD.\n\nThanks. Will queue.\n"},{"id":"386891","messageId":"xmqq5zjb2vsx.fsf@gitster-ct.c.googlers.com","threadId":"52305","inReplyTo":"037f2b2975e06847443aef46939e3c712053dedf.1574451783.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-23T02:03:42Z","receivedAt":"2019-11-23T02:04:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> +\tif (!(flags & ALLOW_EMPTY)) {\n> +\t\tstruct commit *first_parent = current_head;\n> +\n> +\t\tif (flags & AMEND_MSG) {\n> +\t\t\tif (current_head->parents) {\n\nIt is not apparent to me that somebody guarantees that this access\nis safe; would we need to do things differently when !current_head?\n\n> +\t\t\t\tfirst_parent = current_head->parents->item;\n> +\t\t\t\tif (repo_parse_commit(r, first_parent)) {\n> +\t\t\t\t\tres = error(_(\"could not parse HEAD commit\"));\n> +\t\t\t\t\tgoto out;\n> +\t\t\t\t}\n> +\t\t\t} else {\n> +\t\t\t\tfirst_parent = NULL;\n> +\t\t\t}\n> +\t\t}\n"},{"id":"386901","messageId":"340859a7-5cc4-f641-818d-fcedbf29a2a6@gmail.com","threadId":"52305","inReplyTo":"xmqq5zjb2vsx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-23T09:54:49Z","receivedAt":"2019-11-23T09:54:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 23/11/2019 02:03, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> +\tif (!(flags & ALLOW_EMPTY)) {\n>> +\t\tstruct commit *first_parent = current_head;\n>> +\n>> +\t\tif (flags & AMEND_MSG) {\n>> +\t\t\tif (current_head->parents) {\n> \n> It is not apparent to me that somebody guarantees that this access\n> is safe; would we need to do things differently when !current_head?\n\nThat's a good point, I'll fix it, thanks for catching this\n\nBest Wishes\n\nPhillip\n\n> \n>> +\t\t\t\tfirst_parent = current_head->parents->item;\n>> +\t\t\t\tif (repo_parse_commit(r, first_parent)) {\n>> +\t\t\t\t\tres = error(_(\"could not parse HEAD commit\"));\n>> +\t\t\t\t\tgoto out;\n>> +\t\t\t\t}\n>> +\t\t\t} else {\n>> +\t\t\t\tfirst_parent = NULL;\n>> +\t\t\t}\n>> +\t\t}\n"},{"id":"386936","messageId":"94573071-556b-caae-b159-40c168a08f44@gmail.com","threadId":"52305","inReplyTo":"340859a7-5cc4-f641-818d-fcedbf29a2a6@gmail.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-24T10:52:01Z","receivedAt":"2019-11-24T10:52:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 23/11/2019 09:54, Phillip Wood wrote:\n> Hi Junio\n> \n> On 23/11/2019 02:03, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" writes:\n>>\n>>> +    if (!(flags & ALLOW_EMPTY)) {\n>>> +        struct commit *first_parent = current_head;\n>>> +\n>>> +        if (flags & AMEND_MSG) {\n>>> +            if (current_head->parents) {\n>>\n>> It is not apparent to me that somebody guarantees that this access\n>> is safe; would we need to do things differently when !current_head?\n\nWe do actually check that there is a valid HEAD before we try to fixup a \ncommit. Though perhaps we should still change this patch as HEAD may be \nchanged by another process between that check and re-reading it here. If \nyou try to fixup a commit without a valid HEAD you get\n\nerror: need a HEAD to fixup\nhint: Could not execute the todo command\nhint:\nhint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P\nhint:\nhint: It has been rescheduled; To edit the command before continuing, please\nhint: edit the todo list first:\nhint:\nhint: git rebase --edit-todo\nhint: git rebase --continue\nerror: could not copy '.git/rebase-merge/message-squash' to \n'.git/rebase-merge/message'\n\nThe last error message is unfortunate but we do exit in an orderly \nmanner rather than segfaulting. It's a bit tricky to trigger this (there \nisn't a test at the moment) but something like this does it\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d2f1d5bd23..4f55f0cd1c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -67,6 +67,21 @@ test_expect_success 'setup' '\n SHELL=\n export SHELL\n\n+test_expect_success 'fixup on orphan branch errors out' '\n+\n+ test_when_finished \"git switch master\" &&\n+ write_script switch-branch.sh <<-\\EOF &&\n+ git symbolic-ref HEAD refs/heads/does-not-exist &&\n+ git rm -rf .\n+ EOF\n+ (\n+ set_fake_editor &&\n+ FAKE_LINES=\"exec_./switch-branch.sh \\\n+ fixup 1\" git rebase -i HEAD^\n+ ) &&\n+ test_pause\n+'\n+\n\nI think it would be useful to add something like this to the test suite \n(changed to check the error message, with a better name for the script \nand modified to expect failure) What do you think?\n\nBest Wishes\n\nPhillip\n\n> That's a good point, I'll fix it, thanks for catching this\n> \n> Best Wishes\n> \n> Phillip\n> \n>>\n>>> +                first_parent = current_head->parents->item;\n>>> +                if (repo_parse_commit(r, first_parent)) {\n>>> +                    res = error(_(\"could not parse HEAD commit\"));\n>>> +                    goto out;\n>>> +                }\n>>> +            } else {\n>>> +                first_parent = NULL;\n>>> +            }\n>>> +        }\n"},{"id":"386966","messageId":"xmqqr21wy80o.fsf@gitster-ct.c.googlers.com","threadId":"52305","inReplyTo":"94573071-556b-caae-b159-40c168a08f44@gmail.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-25T03:00:55Z","receivedAt":"2019-11-25T03:01:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> We do actually check that there is a valid HEAD before we try to fixup\n> a commit. Though perhaps we should still change this patch as HEAD may\n> be changed by another process between that check and re-reading it\n> here. If you try to fixup a commit without a valid HEAD you get\n>\n> error: need a HEAD to fixup\n> hint: Could not execute the todo command\n> hint:\n> hint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P\n> hint:\n> hint: It has been rescheduled; To edit the command before continuing, please\n> hint: edit the todo list first:\n> hint:\n> hint: git rebase --edit-todo\n> hint: git rebase --continue\n> error: could not copy '.git/rebase-merge/message-squash' to\n> '.git/rebase-merge/message'\n>\n> The last error message is unfortunate but we do exit in an orderly\n> manner rather than segfaulting.\n\nThanks for thinking about the issue further.\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index d2f1d5bd23..4f55f0cd1c 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -67,6 +67,21 @@ test_expect_success 'setup' '\n> SHELL=\n> export SHELL\n>\n> +test_expect_success 'fixup on orphan branch errors out' '\n> +\n> + test_when_finished \"git switch master\" &&\n> + write_script switch-branch.sh <<-\\EOF &&\n> + git symbolic-ref HEAD refs/heads/does-not-exist &&\n> + git rm -rf .\n\nThat \"git rm -rf .\" scares me, though.\n\n> + EOF\n> + (\n> + set_fake_editor &&\n> + FAKE_LINES=\"exec_./switch-branch.sh \\\n> + fixup 1\" git rebase -i HEAD^\n> + ) &&\n> + test_pause\n> +'\n> +\n>\n> I think it would be useful to add something like this to the test\n> suite (changed to check the error message, with a better name for the\n> script and modified to expect failure) What do you think?\n\nSo, we try an interactive rebase, try to apply a fix-up on an unborn\nbranch and expect it to fail in a controlled way, something like\n\n\t(\n\t\t# we are in subshell so freely export\n\t\tset_fake_editor &&\n\t\texport FAKE_LINES=\"exec_./switch-branch.sh fixup 1\" &&\n\t\ttest_must_fail git rebase -i HEAD^ 2>error &&\n\t\ttest_i18ngrep \"... what we expect ...\" error\n\t)\n\nPerhaps. I do not think of a good reason why we should allow\nswitching to another branch when \"rebase -i\" gives control back to\nthe user, so in the longer run, the checked condition may not stay\nthe same (I suspect you would catch \"does-not-exist is unborn and\nthere is nothing to 'fixup'\" right now---I am envisioning that the\ncondition that is checked and the message we would issue would be\n\"we gave you a detached HEAD for a good reason---stay there and do\nnot switch to any other branch\") and the message expected by this\ntest would have to be updated.\n\nThanks.\n\n\n\n"},{"id":"386992","messageId":"43bdadd2-9ea9-4e50-1f47-ec18e0db4794@gmail.com","threadId":"52305","inReplyTo":"xmqqr21wy80o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-25T14:23:26Z","receivedAt":"2019-11-25T14:23:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/11/2019 03:00, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> We do actually check that there is a valid HEAD before we try to fixup\n>> a commit. Though perhaps we should still change this patch as HEAD may\n>> be changed by another process between that check and re-reading it\n>> here. If you try to fixup a commit without a valid HEAD you get\n>>\n>> error: need a HEAD to fixup\n>> hint: Could not execute the todo command\n>> hint:\n>> hint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P\n>> hint:\n>> hint: It has been rescheduled; To edit the command before continuing, please\n>> hint: edit the todo list first:\n>> hint:\n>> hint: git rebase --edit-todo\n>> hint: git rebase --continue\n>> error: could not copy '.git/rebase-merge/message-squash' to\n>> '.git/rebase-merge/message'\n>>\n>> The last error message is unfortunate but we do exit in an orderly\n>> manner rather than segfaulting.\n> \n> Thanks for thinking about the issue further.\n> \n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index d2f1d5bd23..4f55f0cd1c 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -67,6 +67,21 @@ test_expect_success 'setup' '\n>> SHELL=\n>> export SHELL\n>>\n>> +test_expect_success 'fixup on orphan branch errors out' '\n>> +\n>> + test_when_finished \"git switch master\" &&\n>> + write_script switch-branch.sh <<-\\EOF &&\n>> + git symbolic-ref HEAD refs/heads/does-not-exist &&\n>> + git rm -rf .\n> \n> That \"git rm -rf .\" scares me, though.\n\nI know I'm not too keen on it my self but we need to empty the worktree \nand index if we're going to switch to an unborn branch\n\n> \n>> + EOF\n>> + (\n>> + set_fake_editor &&\n>> + FAKE_LINES=\"exec_./switch-branch.sh \\\n>> + fixup 1\" git rebase -i HEAD^\n>> + ) &&\n>> + test_pause\n>> +'\n>> +\n>>\n>> I think it would be useful to add something like this to the test\n>> suite (changed to check the error message, with a better name for the\n>> script and modified to expect failure) What do you think?\n> \n> So, we try an interactive rebase, try to apply a fix-up on an unborn\n> branch and expect it to fail in a controlled way, something like\n> \n> \t(\n> \t\t# we are in subshell so freely export\n> \t\tset_fake_editor &&\n> \t\texport FAKE_LINES=\"exec_./switch-branch.sh fixup 1\" &&\n> \t\ttest_must_fail git rebase -i HEAD^ 2>error &&\n> \t\ttest_i18ngrep \"... what we expect ...\" error\n> \t)\n> \n> Perhaps. I do not think of a good reason why we should allow\n> switching to another branch when \"rebase -i\" gives control back to\n> the user, so in the longer run, the checked condition may not stay\n> the same (I suspect you would catch \"does-not-exist is unborn and\n> there is nothing to 'fixup'\" right now---I am envisioning that the\n> condition that is checked and the message we would issue would be\n> \"we gave you a detached HEAD for a good reason---stay there and do\n> not switch to any other branch\") and the message expected by this\n> test would have to be updated.\n\nI agree there's no good reason for a user to do this. 'git switch' will \nrefuse to switch branches during a rebase for that reason. At the moment \nwe check HEAD with get_oid() so that would need changing to check if the \nuser has switched to another branch (perhaps it could be done when we \ncheck that the index and worktree are clean after running an exec command).\n\nBest Wishes\n\nPhillip\n\n> \n> Thanks.\n> \n> \n> \n"},{"id":"387004","messageId":"nycvar.QRO.7.76.6.1911251652510.31080@tvgsbejvaqbjf.bet","threadId":"52305","inReplyTo":"43bdadd2-9ea9-4e50-1f47-ec18e0db4794@gmail.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-25T15:53:56Z","receivedAt":"2019-11-25T15:54:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Mon, 25 Nov 2019, Phillip Wood wrote:\n\n> On 25/11/2019 03:00, Junio C Hamano wrote:\n> > Phillip Wood writes:\n> >\n> > > We do actually check that there is a valid HEAD before we try to fixup\n> > > a commit. Though perhaps we should still change this patch as HEAD may\n> > > be changed by another process between that check and re-reading it\n> > > here. If you try to fixup a commit without a valid HEAD you get\n> > >\n> > > error: need a HEAD to fixup\n> > > hint: Could not execute the todo command\n> > > hint:\n> > > hint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P\n> > > hint:\n> > > hint: It has been rescheduled; To edit the command before continuing,\n> > > hint: please\n> > > hint: edit the todo list first:\n> > > hint:\n> > > hint: git rebase --edit-todo\n> > > hint: git rebase --continue\n> > > error: could not copy '.git/rebase-merge/message-squash' to\n> > > '.git/rebase-merge/message'\n> > >\n> > > The last error message is unfortunate but we do exit in an orderly\n> > > manner rather than segfaulting.\n> >\n> > Thanks for thinking about the issue further.\n> >\n> > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> > > index d2f1d5bd23..4f55f0cd1c 100755\n> > > --- a/t/t3404-rebase-interactive.sh\n> > > +++ b/t/t3404-rebase-interactive.sh\n> > > @@ -67,6 +67,21 @@ test_expect_success 'setup' '\n> > > SHELL=\n> > > export SHELL\n> > >\n> > > +test_expect_success 'fixup on orphan branch errors out' '\n> > > +\n> > > + test_when_finished \"git switch master\" &&\n> > > + write_script switch-branch.sh <<-\\EOF &&\n> > > + git symbolic-ref HEAD refs/heads/does-not-exist &&\n> > > + git rm -rf .\n> >\n> > That \"git rm -rf .\" scares me, though.\n>\n> I know I'm not too keen on it my self but we need to empty the worktree and\n> index if we're going to switch to an unborn branch\n\nHow about `git worktree --orphan does-not-exist unborn`?\n\nCiao,\nDscho\n"},{"id":"387005","messageId":"CAPig+cRg1WchC=-4qc51KQLgYewiYtEsu+RTf=_jbw3sZznjew@mail.gmail.com","threadId":"52305","inReplyTo":"nycvar.QRO.7.76.6.1911251652510.31080@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-11-25T16:10:49Z","receivedAt":"2019-11-25T16:11:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 25, 2019 at 10:54 AM Johannes Schindelin\n wrote:\n> On Mon, 25 Nov 2019, Phillip Wood wrote:\n> > On 25/11/2019 03:00, Junio C Hamano wrote:\n> > > That \"git rm -rf .\" scares me, though.\n> >\n> > I know I'm not too keen on it my self but we need to empty the worktree and\n> > index if we're going to switch to an unborn branch\n>\n> How about `git worktree --orphan does-not-exist unborn`?\n\ngit-worktree doesn't presently recognize --orphan, though it would be\nnice if it did. In fact, I clearly was thinking of --orphan (along\nwith -b, -B, and --detach), when I wrote the implementation, as can be\nseen from the commentary in one of the original patches[1]. That\n--orphan never got added was either due to an oversight or it was one\nof those \"we'll add it when someone actually needs it\" deals.\n\n[1]: https://lore.kernel.org/git/1436573146-3893-11-git-send-email-sunshine@sunshineco.com/\n"},{"id":"387009","messageId":"cb0748e9-5eb1-3ed5-a4bd-94eb540076bc@gmail.com","threadId":"52305","inReplyTo":"nycvar.QRO.7.76.6.1911251652510.31080@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-25T16:42:07Z","receivedAt":"2019-11-25T16:42:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 25/11/2019 15:53, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Mon, 25 Nov 2019, Phillip Wood wrote:\n> \n>> On 25/11/2019 03:00, Junio C Hamano wrote:\n>>> Phillip Wood writes:\n>>>\n>>>> We do actually check that there is a valid HEAD before we try to fixup\n>>>> a commit. Though perhaps we should still change this patch as HEAD may\n>>>> be changed by another process between that check and re-reading it\n>>>> here. If you try to fixup a commit without a valid HEAD you get\n>>>>\n>>>> error: need a HEAD to fixup\n>>>> hint: Could not execute the todo command\n>>>> hint:\n>>>> hint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P\n>>>> hint:\n>>>> hint: It has been rescheduled; To edit the command before continuing,\n>>>> hint: please\n>>>> hint: edit the todo list first:\n>>>> hint:\n>>>> hint: git rebase --edit-todo\n>>>> hint: git rebase --continue\n>>>> error: could not copy '.git/rebase-merge/message-squash' to\n>>>> '.git/rebase-merge/message'\n>>>>\n>>>> The last error message is unfortunate but we do exit in an orderly\n>>>> manner rather than segfaulting.\n>>>\n>>> Thanks for thinking about the issue further.\n>>>\n>>>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>>>> index d2f1d5bd23..4f55f0cd1c 100755\n>>>> --- a/t/t3404-rebase-interactive.sh\n>>>> +++ b/t/t3404-rebase-interactive.sh\n>>>> @@ -67,6 +67,21 @@ test_expect_success 'setup' '\n>>>> SHELL=\n>>>> export SHELL\n>>>>\n>>>> +test_expect_success 'fixup on orphan branch errors out' '\n>>>> +\n>>>> + test_when_finished \"git switch master\" &&\n>>>> + write_script switch-branch.sh <<-\\EOF &&\n>>>> + git symbolic-ref HEAD refs/heads/does-not-exist &&\n>>>> + git rm -rf .\n>>>\n>>> That \"git rm -rf .\" scares me, though.\n>>\n>> I know I'm not too keen on it my self but we need to empty the worktree and\n>> index if we're going to switch to an unborn branch\n> \n> How about `git worktree --orphan does-not-exist unborn`?\n\nI'm trying to create the unborn branch in the current worktree as that \nis where the rebase is happening\n\nBest Wishes\n\nPhillip\n\n> \n> Ciao,\n> Dscho\n> \n"},{"id":"387022","messageId":"nycvar.QRO.7.76.6.1911252343350.31080@tvgsbejvaqbjf.bet","threadId":"52305","inReplyTo":"CAPig+cRg1WchC=-4qc51KQLgYewiYtEsu+RTf=_jbw3sZznjew@mail.gmail.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-25T22:52:45Z","receivedAt":"2019-11-25T22:53:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\n\nOn Mon, 25 Nov 2019, Eric Sunshine wrote:\n\n> On Mon, Nov 25, 2019 at 10:54 AM Johannes Schindelin\n> wrote:\n> > On Mon, 25 Nov 2019, Phillip Wood wrote:\n> > > On 25/11/2019 03:00, Junio C Hamano wrote:\n> > > > That \"git rm -rf .\" scares me, though.\n> > >\n> > > I know I'm not too keen on it my self but we need to empty the worktree and\n> > > index if we're going to switch to an unborn branch\n> >\n> > How about `git worktree --orphan does-not-exist unborn`?\n>\n> git-worktree doesn't presently recognize --orphan, though it would be\n> nice if it did. In fact, I clearly was thinking of --orphan (along\n> with -b, -B, and --detach), when I wrote the implementation, as can be\n> seen from the commentary in one of the original patches[1]. That\n> --orphan never got added was either due to an oversight or it was one\n> of those \"we'll add it when someone actually needs it\" deals.\n>\n> [1]: https://lore.kernel.org/git/1436573146-3893-11-git-send-email-sunshine@sunshineco.com/\n\nYou're absolutely correct, of course. I actually had looked at the output\nof `git checkout -h` instead of `git worktree -h`... And `checkout` does\nhave that `--orphan` option.\n\nBut from the documentation at\nhttps://git-scm.com/docs/git-checkout#Documentation/git-checkout.txt---orphanltnewbranchgt\nI see that the command I had in mind does not work as I expected it to:\n`git checkout --orphan new-branch $EMPTY_TREE` will fail with\nfatal: Cannot switch branch to a non-commit '4b825dc642cb6eb9a060e54bf8d69288fbee4904'\n(and the documentation of the `--orphan` option also suggests to use `git\nrm -rf` for the use case under discussion, so there...)\n\nSorry for the noise,\nDscho\n"},{"id":"387033","messageId":"xmqqy2w3wieb.fsf@gitster-ct.c.googlers.com","threadId":"52305","inReplyTo":"43bdadd2-9ea9-4e50-1f47-ec18e0db4794@gmail.com","subject":"Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-26T01:11:56Z","receivedAt":"2019-11-26T01:13:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> +test_expect_success 'fixup on orphan branch errors out' '\n>>> +\n>>> + test_when_finished \"git switch master\" &&\n>>> + write_script switch-branch.sh <<-\\EOF &&\n>>> + git symbolic-ref HEAD refs/heads/does-not-exist &&\n>>> + git rm -rf .\n>>\n>> That \"git rm -rf .\" scares me, though.\n>\n> I know I'm not too keen on it my self but we need to empty the\n> worktree and index if we're going to switch to an unborn branch\n\n\"checkout --orphan\" takes you to an unborn branch. If you need to\nalso be with no contents in the index and nothing in the working\ntree, then \"git rm -r .\" may be needed, but applying a fixup without\nthe target content does sound like asking for a conflict already.\n\n"}]}