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

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

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> In sequencer.c, caller only pass TODO_SQUASH or TODO_FIXUP to
> update_squash_messages(), any other command passed in should be
> considered as BUG. Thus I think `return error('unknown command')`
> should be replaced as `BUG('unknown command')`.

Yup.  The only caller has

	else if (is_fixup(command)) {
	     if (update_squash_messages(r, command, ...) {

and is_fixup() is confusingly [*] defined to return true only when
the command is one of these two values.

[Footnote]

 * And a similarly named is_fixup_flag() only accepts TODO_FIXUP and
   never TODO_SQUASH.  Both of these helpers probably need to be
   renamed.

> diff --git a/sequencer.c b/sequencer.c
> index b5c4043757e..3cd0dd3434e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2139,7 +2139,7 @@ static int update_squash_messages(struct repository *r,
>  		strbuf_add_commented_lines(&buf, body, strlen(body),
>  					   comment_line_str);
>  	} else
> -		return error(_("unknown command: %d"), command);
> +		BUG(_("unknown command: %d"), command);
>  	repo_unuse_commit_buffer(r, commit, message);

BUG() is not end-user facing but programmer facing, and we do not
use _("...") in them.  I see a few existing violators that need to
be corrected.

OK.  Or

	if (!is_fixup(command))
		BUG("not a FIXUP or SQUASH %d", command);

at the very beginning of the function?