Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2023, 20:28 UTC
Message-ID: <xmqqv8dqd2bh.fsf@gitster.g>
In-Reply-To: <20230807170935.2336745-1-oswald.buddenhagen@gmx.de>

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

Show 14 quoted lines

>  			if (item->command == TODO_FIXUP) {
>  				if (item->flags & TODO_EDIT_FIXUP_MSG)
>  					strbuf_addstr(buf, " -c");
> -				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
> +				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
>  					strbuf_addstr(buf, " -C");
> -				}
> -			}
> -
> -			if (item->command == TODO_MERGE) {
> +			} else if (item->command == TODO_MERGE) {
>  				if (item->flags & TODO_EDIT_MERGE_MSG)
>  					strbuf_addstr(buf, " -c");
>  				else

This patch as it stands is a strict Meh at least to me, as we know item->command is not something we will mess with in the loop, so turning two if() into if/elseif does not add all that much value in readability.

Having said that.

The code makes casual readers curious about other things.

 * Are FIXUP and MERGE the only two commands that need to be treated
   differently here?

 * Can item->commit be some other TODO_* command?  What is the
   reason why they can be no-op?

 * When one wants to invent a new kind of TODO_* command, what is
   the right way to deal with it in this if/else cascade?

And that leads me to wonder if this is better rewritten with

	switch (item->command) {
	case TODO_FIXUP:
		...
		break;
	case TODO_MERGE:
		...
		break;
	default:
		/*
		 * all other cases:
		 * we can have a brief explanation on why
		 * they do not need anything done here if we want
		 */
		break;
	}