# [PATCH] rebase -i: permit 'drop' of a merge commit 4 messages from 2025-08-06 to 2025-08-07. Participants: Johannes Sixt, Junio C Hamano, Phillip Wood. Thread: https://gitlist.dev/t/63918 ## Johannes Sixt, 2025-08-06 17:38 Subject: [PATCH] rebase -i: permit 'drop' of a merge commit Message-ID: <37f6e34c-91aa-4e55-88e1-019d2e042df3@kdbg.org> URL: https://gitlist.dev/e/37f6e34c-91aa-4e55-88e1-019d2e042df3%40kdbg.org ``` 4c063c82e9 (rebase -i: improve error message when picking merge, 2024-05-30) added advice texts for cases when a merge commit is passed as argument of sequencer command that cannot operate with a merge commit. However, it forgot about the 'drop' command, so that in this case the BUG() in the default branch is reached. Handle 'drop' like 'merge', i.e., permit it without a message. Signed-off-by: Johannes Sixt --- sequencer.c | 1 + t/t3404-rebase-interactive.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/sequencer.c b/sequencer.c index aaf2e4df64..9ae40a91b2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command) case TODO_SQUASH: return error(_("cannot squash merge commit into another commit")); case TODO_MERGE: + case TODO_DROP: return 0; default: BUG("unexpected todo_command"); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 6bac217ed3..34d6ad0770 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -2262,8 +2262,9 @@ rebase_setup_and_clean () { reword $oid edit $oid fixup $oid squash $oid + drop $oid # acceptable, no advice EOF ( set_replace_editor todo && test_must_fail git rebase -i HEAD 2>actual -- 2.50.1.837.g8c5950ae16 ``` ## Junio C Hamano, 2025-08-06 21:04 Subject: Re: [PATCH] rebase -i: permit 'drop' of a merge commit Message-ID: URL: https://gitlist.dev/e/xmqqjz3gtb4w.fsf%40gitster.g In-Reply-To: <37f6e34c-91aa-4e55-88e1-019d2e042df3@kdbg.org> ``` Johannes Sixt writes: > 4c063c82e9 (rebase -i: improve error message when picking merge, > 2024-05-30) added advice texts for cases when a merge commit is > passed as argument of sequencer command that cannot operate with > a merge commit. However, it forgot about the 'drop' command, so > that in this case the BUG() in the default branch is reached. > > Handle 'drop' like 'merge', i.e., permit it without a message. > > Signed-off-by: Johannes Sixt > --- > sequencer.c | 1 + > t/t3404-rebase-interactive.sh | 1 + > 2 files changed, 2 insertions(+) Thanks. Now I understand why some people are sometimes tempted to omit the default arm in switch() and allow compilers complain when explicit case arms are not exhaustive. I am not saying we should do so, and I am not convinced that it is a good idea (there are cases you cannot afford to be exhausitive, yet the cases your particular switch must care about are multiple to make an if/else if cascade impractical). But this is one of the case it might make sense. > diff --git a/sequencer.c b/sequencer.c > index aaf2e4df64..9ae40a91b2 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command) > case TODO_SQUASH: > return error(_("cannot squash merge commit into another commit")); > > case TODO_MERGE: > + case TODO_DROP: > return 0; > > default: > BUG("unexpected todo_command"); > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 6bac217ed3..34d6ad0770 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -2262,8 +2262,9 @@ rebase_setup_and_clean () { > reword $oid > edit $oid > fixup $oid > squash $oid > + drop $oid # acceptable, no advice > EOF > ( > set_replace_editor todo && > test_must_fail git rebase -i HEAD 2>actual ``` ## Phillip Wood, 2025-08-07 13:39 Subject: Re: [PATCH] rebase -i: permit 'drop' of a merge commit Message-ID: URL: https://gitlist.dev/e/d55b3745-8471-4c57-aced-7813716a216d%40gmail.com In-Reply-To: <37f6e34c-91aa-4e55-88e1-019d2e042df3@kdbg.org> ``` Hi Johannes On 06/08/2025 18:38, Johannes Sixt wrote: > 4c063c82e9 (rebase -i: improve error message when picking merge, > 2024-05-30) added advice texts for cases when a merge commit is > passed as argument of sequencer command that cannot operate with > a merge commit. However, it forgot about the 'drop' command, so > that in this case the BUG() in the default branch is reached. > > Handle 'drop' like 'merge', i.e., permit it without a message. Thanks for fixing this and also for taking the time to extend the regression test. Phillip > Signed-off-by: Johannes Sixt > --- > sequencer.c | 1 + > t/t3404-rebase-interactive.sh | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/sequencer.c b/sequencer.c > index aaf2e4df64..9ae40a91b2 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command) > case TODO_SQUASH: > return error(_("cannot squash merge commit into another commit")); > > case TODO_MERGE: > + case TODO_DROP: > return 0; > > default: > BUG("unexpected todo_command"); > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 6bac217ed3..34d6ad0770 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -2262,8 +2262,9 @@ rebase_setup_and_clean () { > reword $oid > edit $oid > fixup $oid > squash $oid > + drop $oid # acceptable, no advice > EOF > ( > set_replace_editor todo && > test_must_fail git rebase -i HEAD 2>actual ``` ## Phillip Wood, 2025-08-07 13:50 Subject: Re: [PATCH] rebase -i: permit 'drop' of a merge commit Message-ID: <7c8b1886-e5cb-420a-894a-f0434a766117@gmail.com> URL: https://gitlist.dev/e/7c8b1886-e5cb-420a-894a-f0434a766117%40gmail.com In-Reply-To: ``` On 06/08/2025 22:04, Junio C Hamano wrote: > Johannes Sixt writes: > > Thanks. Now I understand why some people are sometimes tempted to > omit the default arm in switch() and allow compilers complain when > explicit case arms are not exhaustive. I am not saying we should do > so, and I am not convinced that it is a good idea (there are cases > you cannot afford to be exhausitive, yet the cases your particular > switch must care about are multiple to make an if/else if cascade > impractical). But this is one of the case it might make sense. I think there are definitely cases like this where it makes sense to require the case statements to be exhaustive. Looking at the documentation for -Wswitch [1] which is enabled by -Wall it only issues a warning when there is no default arm and the case statements are non-exhaustive. So I think we could start relying on that just by deleting the default arms where we think it makes sense for the case statements to be exhaustive. I've previously worked on a code base that enabled -Wswitch-enum which requires the case statements to be exhaustive even if there is a default arm and that was a pain in the neck. Thanks Phillip [1] https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Warning-Options.html#index-Wswitch >> diff --git a/sequencer.c b/sequencer.c >> index aaf2e4df64..9ae40a91b2 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2720,8 +2720,9 @@ static int check_merge_commit_insn(enum todo_command command) >> case TODO_SQUASH: >> return error(_("cannot squash merge commit into another commit")); >> >> case TODO_MERGE: >> + case TODO_DROP: >> return 0; >> >> default: >> BUG("unexpected todo_command"); >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 6bac217ed3..34d6ad0770 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -2262,8 +2262,9 @@ rebase_setup_and_clean () { >> reword $oid >> edit $oid >> fixup $oid >> squash $oid >> + drop $oid # acceptable, no advice >> EOF >> ( >> set_replace_editor todo && >> test_must_fail git rebase -i HEAD 2>actual > ```