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

[Junio C Hamano <gitster@pobox.com>]

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.

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.