Re: [PATCH v2] sequencer: replace error() with BUG() in update_squash_messages()

From: Junio C Hamano <gitster@pobox.com>
Date: May 30, 2025, 04:29 UTC
Message-ID: <xmqq1ps6iuw0.fsf@gitster.g>
In-Reply-To: <edd4d392-f124-4e97-b4a9-6cf3fde1e804@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 7 quoted lines

>> OK.  Or
>> 	if (!is_fixup(command))
>> 		BUG("not a FIXUP or SQUASH %d", command);
>> at the very beginning of the function?
>
> Asserting the precondition at the start of the function sounds like a
> good idea

I am of two minds.

I do not know if this is better for longer-term maintainability or not. Such a message at the beginning of the function declares that the current implementation does not want to receive a command that is not either of these two. Such a message tells the next developer who adds another caller to the function to make sure not to pass other commands, which is good.

But I do not know what it tells to the next developer who wants to add support for another command. After opening that initial gate a bit wider, perhaps to

	if (!is_filxup(command) && !is_my_new_command(command))
		BUT("not a FIXUP or SQUASH or MYCOMMAND %d", command);

would they make sure to handle their new command correctly in the if/elseif cascade, from which we are removing the "error()" with such a change, and would our reviewers notice if they forget to do so? I dunno.

In any case, such a future change will have to be done with a good understanding of the entirety of this small function anyway, so I guess either way is OK.

Thanks.