From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 02 Jun 2025 13:59:31 GMT
Subject: Re: [PATCH v2] sequencer: replace error() with BUG() in update_squash_messages()
Message-ID: <eba62aad-a396-46d9-9a37-3521e4e7a23e@gmail.com>
In-Reply-To: <xmqq1ps6iuw0.fsf@gitster.g>

On 30/05/2025 05:29, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> 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.

I guess it cuts both ways. As you say it is a small function and I think 
someone adding support for a new command would need to do more that just 
change the precondition to get their code to work unless they're calling 
this when they don't need to.

> 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.

Agreed

Phillip