Re: [PATCH 2/2] rebase --exec: make it work with --rebase-merges

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2018, 20:26 UTC
Message-ID: <xmqqa7q3w61q.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <b29c4d979f147f38bc0a4765a953a748eec0cd6d.1533318123.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> +	/*
> +	 * Insert <commands> after every pick. Here, fixup/squash chains
> +	 * are considered part of the pick, so we insert the commands *after*
> +	 * those chains if there are any.
> +	 */
> +	insert_final_commands = 1;
> +	for (i = 0; i < todo_list.nr; ) {
> +		enum todo_command command = todo_list.items[i].command;
> +		int j = 0;
> + ...
> +		/* skip fixup/squash chain, if any */
> +		for (i++; i < todo_list.nr; i++, j = 0) {

Does 'j' need to be reset to 0 in each iteration? Nobody looks at 'j' after exiting this inner loop, and every referernce to 'j' inside this inner loop happens _after_ it gets assigned "i+1" at the beginning of "skip comment" loop.

For that matter, I wonder if 'j' can be a variable local to this inner loop, not the outer loop that iterates over todo_list.items[].

Show 19 quoted lines

> +			command = todo_list.items[i].command;
> +
> +			if (is_fixup(command))
> +				continue;
> +
> +			if (command != TODO_COMMENT)
> +				break;
> +
> +			/* skip comment if followed by any fixup/squash */
> +			for (j = i + 1; j < todo_list.nr; j++)
> +				if (todo_list.items[j].command != TODO_COMMENT)
> +					break;
> +			if (j < todo_list.nr &&
> +			    is_fixup(todo_list.items[j].command)) {
> +				i = j;
> +				continue;
> +			}
> +			break;
>  		}