{"thread":{"id":"63918","subject":"[PATCH] rebase -i: permit 'drop' of a merge commit","startedAt":"2025-08-06T17:38:44Z","lastAt":"2025-08-07T13:50:40Z","messageCount":4,"participants":["Johannes Sixt","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"523668","messageId":"37f6e34c-91aa-4e55-88e1-019d2e042df3@kdbg.org","threadId":"63918","inReplyTo":null,"subject":"[PATCH] rebase -i: permit 'drop' of a merge commit","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-08-06T17:38:35Z","receivedAt":"2025-08-06T17:38:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"4c063c82e9 (rebase -i: improve error message when picking merge,\n2024-05-30) added advice texts for cases when a merge commit is\npassed as argument of sequencer command that cannot operate with\na merge commit. However, it forgot about the 'drop' command, so\nthat in this case the BUG() in the default branch is reached.\n\nHandle 'drop' like 'merge', i.e., permit it without a message.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n sequencer.c                   | 1 +\n t/t3404-rebase-interactive.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex aaf2e4df64..9ae40a91b2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command)\n \tcase TODO_SQUASH:\n \t\treturn error(_(\"cannot squash merge commit into another commit\"));\n \n \tcase TODO_MERGE:\n+\tcase TODO_DROP:\n \t\treturn 0;\n \n \tdefault:\n \t\tBUG(\"unexpected todo_command\");\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 6bac217ed3..34d6ad0770 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2262,8 +2262,9 @@ rebase_setup_and_clean () {\n \treword $oid\n \tedit $oid\n \tfixup $oid\n \tsquash $oid\n+\tdrop $oid # acceptable, no advice\n \tEOF\n \t(\n \t\tset_replace_editor todo &&\n \t\ttest_must_fail git rebase -i HEAD 2>actual\n-- \n2.50.1.837.g8c5950ae16\n\n"},{"id":"523679","messageId":"xmqqjz3gtb4w.fsf@gitster.g","threadId":"63918","inReplyTo":"37f6e34c-91aa-4e55-88e1-019d2e042df3@kdbg.org","subject":"Re: [PATCH] rebase -i: permit 'drop' of a merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-06T21:04:31Z","receivedAt":"2025-08-06T21:04:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> 4c063c82e9 (rebase -i: improve error message when picking merge,\n> 2024-05-30) added advice texts for cases when a merge commit is\n> passed as argument of sequencer command that cannot operate with\n> a merge commit. However, it forgot about the 'drop' command, so\n> that in this case the BUG() in the default branch is reached.\n>\n> Handle 'drop' like 'merge', i.e., permit it without a message.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  sequencer.c                   | 1 +\n>  t/t3404-rebase-interactive.sh | 1 +\n>  2 files changed, 2 insertions(+)\n\nThanks.  Now I understand why some people are sometimes tempted to\nomit the default arm in switch() and allow compilers complain when\nexplicit case arms are not exhaustive.  I am not saying we should do\nso, and I am not convinced that it is a good idea (there are cases\nyou cannot afford to be exhausitive, yet the cases your particular\nswitch must care about are multiple to make an if/else if cascade\nimpractical).  But this is one of the case it might make sense.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index aaf2e4df64..9ae40a91b2 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command)\n>  \tcase TODO_SQUASH:\n>  \t\treturn error(_(\"cannot squash merge commit into another commit\"));\n>  \n>  \tcase TODO_MERGE:\n> +\tcase TODO_DROP:\n>  \t\treturn 0;\n>  \n>  \tdefault:\n>  \t\tBUG(\"unexpected todo_command\");\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 6bac217ed3..34d6ad0770 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -2262,8 +2262,9 @@ rebase_setup_and_clean () {\n>  \treword $oid\n>  \tedit $oid\n>  \tfixup $oid\n>  \tsquash $oid\n> +\tdrop $oid # acceptable, no advice\n>  \tEOF\n>  \t(\n>  \t\tset_replace_editor todo &&\n>  \t\ttest_must_fail git rebase -i HEAD 2>actual\n"},{"id":"523747","messageId":"d55b3745-8471-4c57-aced-7813716a216d@gmail.com","threadId":"63918","inReplyTo":"37f6e34c-91aa-4e55-88e1-019d2e042df3@kdbg.org","subject":"Re: [PATCH] rebase -i: permit 'drop' of a merge commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-07T13:39:47Z","receivedAt":"2025-08-07T13:39:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 06/08/2025 18:38, Johannes Sixt wrote:\n> 4c063c82e9 (rebase -i: improve error message when picking merge,\n> 2024-05-30) added advice texts for cases when a merge commit is\n> passed as argument of sequencer command that cannot operate with\n> a merge commit. However, it forgot about the 'drop' command, so\n> that in this case the BUG() in the default branch is reached.\n> \n> Handle 'drop' like 'merge', i.e., permit it without a message.\n\nThanks for fixing this and also for taking the time to extend the \nregression test.\n\nPhillip\n\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>   sequencer.c                   | 1 +\n>   t/t3404-rebase-interactive.sh | 1 +\n>   2 files changed, 2 insertions(+)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index aaf2e4df64..9ae40a91b2 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command)\n>   \tcase TODO_SQUASH:\n>   \t\treturn error(_(\"cannot squash merge commit into another commit\"));\n>   \n>   \tcase TODO_MERGE:\n> +\tcase TODO_DROP:\n>   \t\treturn 0;\n>   \n>   \tdefault:\n>   \t\tBUG(\"unexpected todo_command\");\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 6bac217ed3..34d6ad0770 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -2262,8 +2262,9 @@ rebase_setup_and_clean () {\n>   \treword $oid\n>   \tedit $oid\n>   \tfixup $oid\n>   \tsquash $oid\n> +\tdrop $oid # acceptable, no advice\n>   \tEOF\n>   \t(\n>   \t\tset_replace_editor todo &&\n>   \t\ttest_must_fail git rebase -i HEAD 2>actual\n\n"},{"id":"523748","messageId":"7c8b1886-e5cb-420a-894a-f0434a766117@gmail.com","threadId":"63918","inReplyTo":"xmqqjz3gtb4w.fsf@gitster.g","subject":"Re: [PATCH] rebase -i: permit 'drop' of a merge commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-07T13:50:30Z","receivedAt":"2025-08-07T13:50:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/08/2025 22:04, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> Thanks.  Now I understand why some people are sometimes tempted to\n> omit the default arm in switch() and allow compilers complain when\n> explicit case arms are not exhaustive.  I am not saying we should do\n> so, and I am not convinced that it is a good idea (there are cases\n> you cannot afford to be exhausitive, yet the cases your particular\n> switch must care about are multiple to make an if/else if cascade\n> impractical).  But this is one of the case it might make sense.\n\nI think there are definitely cases like this where it makes sense to \nrequire the case statements to be exhaustive. Looking at the \ndocumentation for -Wswitch [1] which is enabled by -Wall it only issues \na warning when there is no default arm and the case statements are \nnon-exhaustive. So I think we could start relying on that just by \ndeleting the default arms where we think it makes sense for the case \nstatements to be exhaustive. I've previously worked on a code base that \nenabled -Wswitch-enum which requires the case statements to be \nexhaustive even if there is a default arm and that was a pain in the neck.\n\nThanks\n\nPhillip\n\n[1] \nhttps://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Warning-Options.html#index-Wswitch\n\n>> diff --git a/sequencer.c b/sequencer.c\n>> index aaf2e4df64..9ae40a91b2 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command)\n>>   \tcase TODO_SQUASH:\n>>   \t\treturn error(_(\"cannot squash merge commit into another commit\"));\n>>   \n>>   \tcase TODO_MERGE:\n>> +\tcase TODO_DROP:\n>>   \t\treturn 0;\n>>   \n>>   \tdefault:\n>>   \t\tBUG(\"unexpected todo_command\");\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 6bac217ed3..34d6ad0770 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -2262,8 +2262,9 @@ rebase_setup_and_clean () {\n>>   \treword $oid\n>>   \tedit $oid\n>>   \tfixup $oid\n>>   \tsquash $oid\n>> +\tdrop $oid # acceptable, no advice\n>>   \tEOF\n>>   \t(\n>>   \t\tset_replace_editor todo &&\n>>   \t\ttest_must_fail git rebase -i HEAD 2>actual\n> \n\n"}]}