From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 26 Jun 2020 16:03:40 GMT
Subject: Re: [PATCH v5 1/5] rebase -i: add --ignore-whitespace flag
Message-ID: <xmqqk0zthl0j.fsf@gitster.c.googlers.com>
In-Reply-To: <78c32f2d-3af6-1514-51a3-1110531cbb88@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

>>>   +	if (options.type == REBASE_APPLY) {
>>> +		if (ignore_whitespace)
>>> +			argv_array_push (&options.git_am_opts,
>>> +					 "--ignore-whitespace");
>>> +	} else if (ignore_whitespace) {
>>> +			string_list_append (&strategy_options,
>>> +					    "ignore-space-change");
>>> +	}
>>> +
>> ...
> I wanted to keep the subsequent patches as simple as possible. Having
> to rewrite the if statement in the next patch just clutters it up and
> makes the real changes introduced by that patch less obvious

A set of different behaviour depending on .type is OK, but then at
least the above should be more like this:

	if (options.type == REBASE_APPLY) {
		if (ignore_whitespace)
			argv_array_push(...);
	} else {
		/* REBASE_MERGE and PRESERVE_MERGES */
		if (ignore_whitespace)
			string_list_append(...);
	}

or even

	switch (options.type) {
	case REBASE_APPLY:
		...
		break;
	case REBASE_MERGE:
	case REBASE_PRESERVE_MERGES:
		...
		break;
	default:
		BUG("unhandled rebase type %d", options.type);
	}

That would clarify the flow of the logic better.

Thanks.