Re: [PATCH v2 0/2] Make git rebase work with --rebase-merges and --exec

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Team,

On Mon, 6 Aug 2018, Johannes Schindelin via GitGitGadget wrote:

> It was reported via IRC that the exec lines are inserted in the wrong spots
> when using --rebase-merges.
> 
> The reason is that we used a simple, incorrect implementation that happened
> to work as long as the generated todo list only contains pick, fixup and 
> squash commands. Which is not the case with--rebase-merges.
> 
> Fix this issue by using a correct, if longer and slightly more complex
> implementation instead.

I should have paid more attention to detail, and should have updated this
cover letter. My bad.

The last paragraph of the part quoted above should have read:

	Fix this issue by using a correct implementation instead, that
	even takes into account `merge` commands in the --rebase-merges
	mode.

And the changes since v1:

	- Replaced the "look-ahead" design by a "keep looking" one:
	  instead of having a nested loop that looks for the end of the
	  fixup/squash chain, we continue the loop, delaying the insertion
	  until we know where the fixup/squash chain ends, if any.

One quirk I just noticed is that the new code does not really work
correctly in all circumstances: if the todo list ends in a comment (e.g.
an empty commit being reflected by a commented-out `pick`), we still just
append the final commands to the end.

I should qualify by "correct" in this instance: the `exec` commands are
not inserted in the location that I would have liked to, but they *are*
inserted. So it is more an aesthetic thing than anything else, and it will
probably not even show up all that often in practice.

Given that v2 is easier to understand than v1, in my opinion that slightly
awkward inconsistency in insert location is okay.

Ciao,
Dscho

> Johannes Schindelin (2):
>   t3430: demonstrate what -r, --autosquash & --exec should do
>   rebase --exec: make it work with --rebase-merges
> 
>  sequencer.c              | 37 +++++++++++++++++++++++++++----------
>  t/t3430-rebase-merges.sh | 17 +++++++++++++++++
>  2 files changed, 44 insertions(+), 10 deletions(-)
> 
> 
> base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-13%2Fdscho%2Frebase-merges-and-exec-commands-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-13/dscho/rebase-merges-and-exec-commands-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/13
> 
> Range-diff vs v1:
> 
>  1:  1d82eb450 = 1:  1d82eb450 t3430: demonstrate what -r, --autosquash & --exec should do
>  2:  b29c4d979 ! 2:  7ca441a89 rebase --exec: make it work with --rebase-merges
>      @@ -38,7 +38,7 @@
>        	struct strbuf *buf = &todo_list.buf;
>        	size_t offset = 0, commands_len = strlen(commands);
>       -	int i, first;
>      -+	int i, insert_final_commands;
>      ++	int i, insert;
>        
>        	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
>        		return error(_("could not read '%s'."), todo_file);
>      @@ -52,59 +52,38 @@
>       -		if (item->command == TODO_PICK && !first) {
>       -			strbuf_insert(buf, item->offset_in_buf + offset,
>       -				      commands, commands_len);
>      --			offset += commands_len;
>       +	/*
>       +	 * 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; ) {
>      ++	insert = -1;
>      ++	for (i = 0; i < todo_list.nr; i++) {
>       +		enum todo_command command = todo_list.items[i].command;
>      -+		int j = 0;
>       +
>      -+		if (command != TODO_PICK && command != TODO_MERGE) {
>      -+			i++;
>      -+			continue;
>      -+		}
>      -+
>      -+		/* skip fixup/squash chain, if any */
>      -+		for (i++; i < todo_list.nr; i++, j = 0) {
>      -+			command = todo_list.items[i].command;
>      -+
>      -+			if (is_fixup(command))
>      ++		if (insert >= 0) {
>      ++			/* skip fixup/squash chains */
>      ++			if (command == TODO_COMMENT)
>       +				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;
>      ++			else if (is_fixup(command)) {
>      ++				insert = i + 1;
>       +				continue;
>       +			}
>      -+			break;
>      ++			strbuf_insert(buf,
>      ++				      todo_list.items[insert].offset_in_buf +
>      ++				      offset, commands, commands_len);
>      + 			offset += commands_len;
>      ++			insert = -1;
>        		}
>       -		first = 0;
>       +
>      -+		if (i >= todo_list.nr) {
>      -+			insert_final_commands = 1;
>      -+			break;
>      -+		}
>      -+
>      -+		strbuf_insert(buf, todo_list.items[i].offset_in_buf + offset,
>      -+			      commands, commands_len);
>      -+		offset += commands_len;
>      -+		insert_final_commands = 0;
>      ++		if (command == TODO_PICK || command == TODO_MERGE)
>      ++			insert = i + 1;
>        	}
>        
>        	/* append final <commands> */
>       -	strbuf_add(buf, commands, commands_len);
>      -+	if (insert_final_commands)
>      ++	if (insert >= 0 || !offset)
>       +		strbuf_add(buf, commands, commands_len);
>        
>        	i = write_message(buf->buf, buf->len, todo_file, 0);
> 
> -- 
> gitgitgadget
> 
>