threads / patch / 63918

patchrebase -i: permit 'drop' of a merge commit

Subject: [PATCH] rebase -i: permit 'drop' of a merge commit

## tl;dr

4 messages between Aug 6, 2025 and Aug 7, 2025. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Johannes Sixt· Aug 6, 2025, 17:38 UTC · lore

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 <j6t@kdbg.org>
---
 sequencer.c                   | 1 +
 t/t3404-rebase-interactive.sh | 1 +
 2 files changed, 2 insertions(+)

Show changes to 2 files +2 −0

sequencer.c, t/t3404-rebase-interactive.sh

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· Aug 6, 2025, 21:04 UTC · re: Johannes Sixt · lore

Re: [PATCH] rebase -i: permit 'drop' of a merge commit

Johannes Sixt <j6t@kdbg.org> writes:

Show 13 quoted lines

> 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 <j6t@kdbg.org>
> ---
>  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.

Show 28 quoted lines

> 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· Aug 7, 2025, 13:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: permit 'drop' of a merge commit

On 06/08/2025 22:04, Junio C Hamano wrote:

Show 9 quoted lines

> Johannes Sixt <j6t@kdbg.org> 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

Show 29 quoted lines

>> 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· Aug 7, 2025, 13:39 UTC · re: Johannes Sixt · lore

Re: [PATCH] rebase -i: permit 'drop' of a merge commit

Hi Johannes

On 06/08/2025 18:38, Johannes Sixt wrote:

Show 7 quoted lines

> 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

Show 34 quoted lines

> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>   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