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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 23, 2023, 20:32 UTC
Message-ID: <ZBy3aa+7RhnjJUaG@nand.local>
In-Reply-To: <20230323162235.995559-1-oswald.buddenhagen@gmx.de>

On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote:

Show 26 quoted lines

> Make it obvious that the two conditional branches are mutually
> exclusive.
>
> As a drive-by, remove a pair of unnecessary braces.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>  sequencer.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..9169876441 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>  			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) {

I dunno. I think seeing adjacent

    if (item->command == TODO_ABC)

and

    if (item->command == TODO_XYZ)

makes it clear that these two are mutually exclusive, since TODO_ABC != TODO_XYZ.

So I don't mind the unnecessary brace cleanup, but I don't think that this adds additional clarity around these two if-statements.

Specifically: why not combine these two with if-statement that proceeds
it? That might look something like:

    if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
        item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
      ...
    } else if (item->command == TODO_FIXUP) {
      ...
    } else if (item->command == TODO_MERGE) {
      ...
    }

but at that point, you might consider something like:

    switch (item->command) {
    case TODO_EXEC:
    case TODO_LABEL:
    case TODO_RESET:
    case TODO_UPDATE_REF:
      ...
      break;
    case TODO_FIXUP:
      ...
      break;
    case TODO_MERGE:
      ...
      break;
    }

which is arguably clearer, but I have a hard time justifying as worthwhile. TBH, it feels like churn to me, but others may disagree and see it differently.

Thanks, Taylor