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