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

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.

Johannes Schindelin (2):
  t3430: demonstrate what -r, --autosquash & --exec should do
  rebase --exec: make it work with --rebase-merges

 sequencer.c              | 59 ++++++++++++++++++++++++++++++++--------
 t/t3430-rebase-merges.sh | 17 ++++++++++++
 2 files changed, 65 insertions(+), 11 deletions(-)


base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-13%2Fdscho%2Frebase-merges-and-exec-commands-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-13/dscho/rebase-merges-and-exec-commands-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/13
-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The --exec option's implementation is not really well-prepared for
--rebase-merges. Demonstrate this.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t3430-rebase-merges.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 9e6229727..0bf5eaa37 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -363,4 +363,21 @@ test_expect_success 'octopus merges' '
 	EOF
 '
 
+test_expect_failure 'with --autosquash and --exec' '
+	git checkout -b with-exec H &&
+	echo Booh >B.t &&
+	test_tick &&
+	git commit --fixup B B.t &&
+	write_script show.sh <<-\EOF &&
+	subject="$(git show -s --format=%s HEAD)"
+	content="$(git diff HEAD^! | tail -n 1)"
+	echo "$subject: $content"
+	EOF
+	test_tick &&
+	git rebase -ir --autosquash --exec ./show.sh A >actual &&
+	grep "B: +Booh" actual &&
+	grep "E: +Booh" actual &&
+	grep "G: +G" actual
+'
+
 test_done

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The idea of `--exec` is to append an `exec` call after each `pick`.

Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.

The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.

With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.

Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.

While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.

Signed-off-by: Johannes Schindelin <redacted>
---
 sequencer.c              | 59 ++++++++++++++++++++++++++++++++--------
 t/t3430-rebase-merges.sh |  2 +-
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 31038472f..dda5cdbba 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
-	struct todo_item *item;
 	struct strbuf *buf = &todo_list.buf;
 	size_t offset = 0, commands_len = strlen(commands);
-	int i, first;
+	int i, insert_final_commands;
 
 	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
 		return error(_("could not read '%s'."), todo_file);

@@ -4257,19 +4256,57 @@ int sequencer_add_exec_commands(const char *commands)
 		return error(_("unusable todo list: '%s'"), todo_file);
 	}
 
-	first = 1;
-	/* insert <commands> before every pick except the first one */
-	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
-		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; ) {
+		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))
+				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;
 		}
-		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;
 	}
 
 	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	if (insert_final_commands)
+		strbuf_add(buf, commands, commands_len);
 
 	i = write_message(buf->buf, buf->len, todo_file, 0);
 	todo_list_release(&todo_list);

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 0bf5eaa37..90ae613e2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -363,7 +363,7 @@ test_expect_success 'octopus merges' '
 	EOF
 '
 
-test_expect_failure 'with --autosquash and --exec' '
+test_expect_success 'with --autosquash and --exec' '
 	git checkout -b with-exec H &&
 	echo Booh >B.t &&
 	test_tick &&

-- 
gitgitgadget

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.

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

From: Johannes Schindelin <redacted>

The --exec option's implementation is not really well-prepared for
--rebase-merges. Demonstrate this.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t3430-rebase-merges.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 9e6229727..0bf5eaa37 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -363,4 +363,21 @@ test_expect_success 'octopus merges' '
 	EOF
 '
 
+test_expect_failure 'with --autosquash and --exec' '
+	git checkout -b with-exec H &&
+	echo Booh >B.t &&
+	test_tick &&
+	git commit --fixup B B.t &&
+	write_script show.sh <<-\EOF &&
+	subject="$(git show -s --format=%s HEAD)"
+	content="$(git diff HEAD^! | tail -n 1)"
+	echo "$subject: $content"
+	EOF
+	test_tick &&
+	git rebase -ir --autosquash --exec ./show.sh A >actual &&
+	grep "B: +Booh" actual &&
+	grep "E: +Booh" actual &&
+	grep "G: +G" actual
+'
+
 test_done

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The idea of `--exec` is to append an `exec` call after each `pick`.

Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.

The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.

With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.

Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.

While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.

Signed-off-by: Johannes Schindelin <redacted>
---
 sequencer.c              | 37 +++++++++++++++++++++++++++----------
 t/t3430-rebase-merges.sh |  2 +-
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 31038472f..ed2e694ff 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
-	struct todo_item *item;
 	struct strbuf *buf = &todo_list.buf;
 	size_t offset = 0, commands_len = strlen(commands);
-	int i, first;
+	int i, insert;
 
 	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
 		return error(_("could not read '%s'."), todo_file);

@@ -4257,19 +4256,37 @@ int sequencer_add_exec_commands(const char *commands)
 		return error(_("unusable todo list: '%s'"), todo_file);
 	}
 
-	first = 1;
-	/* insert <commands> before every pick except the first one */
-	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
-		if (item->command == TODO_PICK && !first) {
-			strbuf_insert(buf, item->offset_in_buf + offset,
-				      commands, 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 = -1;
+	for (i = 0; i < todo_list.nr; i++) {
+		enum todo_command command = todo_list.items[i].command;
+
+		if (insert >= 0) {
+			/* skip fixup/squash chains */
+			if (command == TODO_COMMENT)
+				continue;
+			else if (is_fixup(command)) {
+				insert = i + 1;
+				continue;
+			}
+			strbuf_insert(buf,
+				      todo_list.items[insert].offset_in_buf +
+				      offset, commands, commands_len);
 			offset += commands_len;
+			insert = -1;
 		}
-		first = 0;
+
+		if (command == TODO_PICK || command == TODO_MERGE)
+			insert = i + 1;
 	}
 
 	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	if (insert >= 0 || !offset)
+		strbuf_add(buf, commands, commands_len);
 
 	i = write_message(buf->buf, buf->len, todo_file, 0);
 	todo_list_release(&todo_list);

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 0bf5eaa37..90ae613e2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -363,7 +363,7 @@ test_expect_success 'octopus merges' '
 	EOF
 '
 
-test_expect_failure 'with --autosquash and --exec' '
+test_expect_success 'with --autosquash and --exec' '
 	git checkout -b with-exec H &&
 	echo Booh >B.t &&
 	test_tick &&

-- 
gitgitgadget

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

Hi Johannes
On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:

insert is not updated so if the next command is not a fixup the exec 
line will be inserted before the comment.

+			else if (is_fixup(command)) {
+				insert = i + 1;
+				continue;
+			}
+			strbuf_insert(buf,
+				      todo_list.items[insert].offset_in_buf +
+				      offset, commands, commands_len);
  			offset += commands_len;
+			insert = -1;
  		}
-		first = 0;
+
+		if (command == TODO_PICK || command == TODO_MERGE)
+			insert = i + 1;
  	}
  
  	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	if (insert >= 0 || !offset)
+		strbuf_add(buf, commands, commands_len);

Having read your other message about this patch I think if you wanted to 
fix the position of the final exec in the case where the todo list ends 
with a comment you could do something like

	if (insert >= 0)
		strbuf_insert(buf,
			      todo_list.items[insert].offset_in_buf +
			      offset, commands, commands_len);
	else
		strbuf_add(buf, commands, commands_len);

I'm not sure it matters that much though

The rest of this patch looks fine to me

Best Wishes

Phillip

On 06/08/18 16:23, Phillip Wood wrote:

Hi Johannes
On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

The idea of `--exec` is to append an `exec` call after each `pick`.

Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.

The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.

With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.

Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.

While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.

Signed-off-by: Johannes Schindelin <redacted>
---
  sequencer.c              | 37 +++++++++++++++++++++++++++----------
  t/t3430-rebase-merges.sh |  2 +-
  2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 31038472f..ed2e694ff 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char 

*commands)
  {
      const char *todo_file = rebase_path_todo();
      struct todo_list todo_list = TODO_LIST_INIT;
-    struct todo_item *item;
      struct strbuf *buf = &todo_list.buf;
      size_t offset = 0, commands_len = strlen(commands);
-    int i, first;
+    int i, insert;
      if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
          return error(_("could not read '%s'."), todo_file);

@@ -4257,19 +4256,37 @@ int sequencer_add_exec_commands(const char 

*commands)
          return error(_("unusable todo list: '%s'"), todo_file);
      }
-    first = 1;
-    /* insert <commands> before every pick except the first one */
-    for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
-        if (item->command == TODO_PICK && !first) {
-            strbuf_insert(buf, item->offset_in_buf + offset,
-                      commands, 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 = -1;
+    for (i = 0; i < todo_list.nr; i++) {
+        enum todo_command command = todo_list.items[i].command;
+
+        if (insert >= 0) {
+            /* skip fixup/squash chains */
+            if (command == TODO_COMMENT)
+                continue;

insert is not updated so if the next command is not a fixup the exec 
line will be inserted before the comment.

quoted

+            else if (is_fixup(command)) {
+                insert = i + 1;
+                continue;
+            }
+            strbuf_insert(buf,
+                      todo_list.items[insert].offset_in_buf +
+                      offset, commands, commands_len);
              offset += commands_len;
+            insert = -1;
          }
-        first = 0;
+
+        if (command == TODO_PICK || command == TODO_MERGE)
+            insert = i + 1;
      }
      /* append final <commands> */
-    strbuf_add(buf, commands, commands_len);
+    if (insert >= 0 || !offset)
+        strbuf_add(buf, commands, commands_len);

Having read your other message about this patch I think if you wanted to 
fix the position of the final exec in the case where the todo list ends 
with a comment you could do something like

     if (insert >= 0)
         strbuf_insert(buf,
                   todo_list.items[insert].offset_in_buf +
                   offset, commands, commands_len);
     else

Oops that should be 'else if (!offset)'

         strbuf_add(buf, commands, commands_len);

I'm not sure it matters that much though

The rest of this patch looks fine to me

Best Wishes

Phillip

quoted

      i = write_message(buf->buf, buf->len, todo_file, 0);
      todo_list_release(&todo_list);

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 0bf5eaa37..90ae613e2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -363,7 +363,7 @@ test_expect_success 'octopus merges' '

      EOF
  '
-test_expect_failure 'with --autosquash and --exec' '
+test_expect_success 'with --autosquash and --exec' '
      git checkout -b with-exec H &&
      echo Booh >B.t &&
      test_tick &&

Hi Phillip,

On Mon, 6 Aug 2018, Phillip Wood wrote:

On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

The idea of `--exec` is to append an `exec` call after each `pick`.

Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.

The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.

With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.

Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.

While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.

Signed-off-by: Johannes Schindelin <redacted>
---
  sequencer.c              | 37 +++++++++++++++++++++++++++----------
  t/t3430-rebase-merges.sh |  2 +-
  2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 31038472f..ed2e694ff 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
  {
   const char *todo_file = rebase_path_todo();
   struct todo_list todo_list = TODO_LIST_INIT;
-	struct todo_item *item;
   struct strbuf *buf = &todo_list.buf;
   size_t offset = 0, commands_len = strlen(commands);
-	int i, first;
+	int i, insert;
  
   if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
  		return error(_("could not read '%s'."), todo_file);

@@ -4257,19 +4256,37 @@ int sequencer_add_exec_commands(const char

*commands)
   	return error(_("unusable todo list: '%s'"), todo_file);
   }
  -	first = 1;
-	/* insert <commands> before every pick except the first one */
-	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
-		if (item->command == TODO_PICK && !first) {
-			strbuf_insert(buf, item->offset_in_buf + offset,
-				      commands, 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 = -1;
+	for (i = 0; i < todo_list.nr; i++) {
+		enum todo_command command = todo_list.items[i].command;
+
+		if (insert >= 0) {
+			/* skip fixup/squash chains */
+			if (command == TODO_COMMENT)
+				continue;

insert is not updated so if the next command is not a fixup the exec
line will be inserted before the comment.

Yes, this is very much on purpose. Take this todo list, for example:

	pick 123456 this patch
	# pick 987654 this was an empty commit

You definitely do not want the `exec` to appear after that commented-out
empty commit.

quoted

+			else if (is_fixup(command)) {
+				insert = i + 1;
+				continue;
+			}
+			strbuf_insert(buf,
+				      todo_list.items[insert].offset_in_buf +
+				      offset, commands, commands_len);
  			offset += commands_len;
+			insert = -1;
  		}
-		first = 0;
+
+		if (command == TODO_PICK || command == TODO_MERGE)
+			insert = i + 1;
   }
  
  	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	if (insert >= 0 || !offset)
+		strbuf_add(buf, commands, commands_len);

Having read your other message about this patch I think if you wanted to fix
the position of the final exec in the case where the todo list ends with a
comment you could do something like

	if (insert >= 0)
		strbuf_insert(buf,
			      todo_list.items[insert].offset_in_buf +
			      offset, commands, commands_len);
	else
		strbuf_add(buf, commands, commands_len);

That does not really work, as `insert` can point *after* the last line, in
which case `todo_list.items[insert]` is undefined (and in the worst case,
causes a segmentation fault).

I'm not sure it matters that much though

Well, it does matter to me. After having this in the back of my head, and
after your comment, I think it *is* worth the additional complexity after
all.

Will come up with a new iteration.

Ciao,
Dscho

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 implementation instead, that even takes
into account merge commands in the --rebase-merges mode.

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.

Johannes Schindelin (2):
  t3430: demonstrate what -r, --autosquash & --exec should do
  rebase --exec: make it work with --rebase-merges

 sequencer.c              | 42 +++++++++++++++++++++++++++++-----------
 t/t3430-rebase-merges.sh | 17 ++++++++++++++++
 2 files changed, 48 insertions(+), 11 deletions(-)


base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-13%2Fdscho%2Frebase-merges-and-exec-commands-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-13/dscho/rebase-merges-and-exec-commands-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/13

Range-diff vs v2:

 1:  1d82eb450 = 1:  1d82eb450 t3430: demonstrate what -r, --autosquash & --exec should do
 2:  7ca441a89 ! 2:  b436f67ba rebase --exec: make it work with --rebase-merges
     @@ -22,6 +22,11 @@
          `pick` lines, skip any fixup/squash chains, and then insert the `exec`
          line. Lather, rinse, repeat.
      
     +    Note: we take pains to insert *before* comment lines whenever possible,
     +    as empty commits are represented by commented-out pick lines (and we
     +    want to insert a preceding pick's exec line *before* such a line, not
     +    afterward).
     +
          While at it, also add `exec` lines after `merge` commands, because they
          are similar in spirit to `pick` commands: they add new commits.
      
     @@ -81,9 +86,13 @@
      +			insert = i + 1;
       	}
       
     - 	/* append final <commands> */
     +-	/* append final <commands> */
      -	strbuf_add(buf, commands, commands_len);
     -+	if (insert >= 0 || !offset)
     ++	/* insert or append final <commands> */
     ++	if (insert >= 0 && insert < todo_list.nr)
     ++		strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
     ++			      offset, commands, commands_len);
     ++	else if (insert >= 0 || !offset)
      +		strbuf_add(buf, commands, commands_len);
       
       	i = write_message(buf->buf, buf->len, todo_file, 0);

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The --exec option's implementation is not really well-prepared for
--rebase-merges. Demonstrate this.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t3430-rebase-merges.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 9e6229727..0bf5eaa37 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -363,4 +363,21 @@ test_expect_success 'octopus merges' '
 	EOF
 '
 
+test_expect_failure 'with --autosquash and --exec' '
+	git checkout -b with-exec H &&
+	echo Booh >B.t &&
+	test_tick &&
+	git commit --fixup B B.t &&
+	write_script show.sh <<-\EOF &&
+	subject="$(git show -s --format=%s HEAD)"
+	content="$(git diff HEAD^! | tail -n 1)"
+	echo "$subject: $content"
+	EOF
+	test_tick &&
+	git rebase -ir --autosquash --exec ./show.sh A >actual &&
+	grep "B: +Booh" actual &&
+	grep "E: +Booh" actual &&
+	grep "G: +G" actual
+'
+
 test_done

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The idea of `--exec` is to append an `exec` call after each `pick`.

Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.

The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.

With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.

Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.

Note: we take pains to insert *before* comment lines whenever possible,
as empty commits are represented by commented-out pick lines (and we
want to insert a preceding pick's exec line *before* such a line, not
afterward).

While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.

Signed-off-by: Johannes Schindelin <redacted>
---
 sequencer.c              | 42 +++++++++++++++++++++++++++++-----------
 t/t3430-rebase-merges.sh |  2 +-
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 31038472f..278d34ce9 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
-	struct todo_item *item;
 	struct strbuf *buf = &todo_list.buf;
 	size_t offset = 0, commands_len = strlen(commands);
-	int i, first;
+	int i, insert;
 
 	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
 		return error(_("could not read '%s'."), todo_file);

@@ -4257,19 +4256,40 @@ int sequencer_add_exec_commands(const char *commands)
 		return error(_("unusable todo list: '%s'"), todo_file);
 	}
 
-	first = 1;
-	/* insert <commands> before every pick except the first one */
-	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
-		if (item->command == TODO_PICK && !first) {
-			strbuf_insert(buf, item->offset_in_buf + offset,
-				      commands, 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 = -1;
+	for (i = 0; i < todo_list.nr; i++) {
+		enum todo_command command = todo_list.items[i].command;
+
+		if (insert >= 0) {
+			/* skip fixup/squash chains */
+			if (command == TODO_COMMENT)
+				continue;
+			else if (is_fixup(command)) {
+				insert = i + 1;
+				continue;
+			}
+			strbuf_insert(buf,
+				      todo_list.items[insert].offset_in_buf +
+				      offset, commands, commands_len);
 			offset += commands_len;
+			insert = -1;
 		}
-		first = 0;
+
+		if (command == TODO_PICK || command == TODO_MERGE)
+			insert = i + 1;
 	}
 
-	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	/* insert or append final <commands> */
+	if (insert >= 0 && insert < todo_list.nr)
+		strbuf_insert(buf, todo_list.items[insert].offset_in_buf +
+			      offset, commands, commands_len);
+	else if (insert >= 0 || !offset)
+		strbuf_add(buf, commands, commands_len);
 
 	i = write_message(buf->buf, buf->len, todo_file, 0);
 	todo_list_release(&todo_list);

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 0bf5eaa37..90ae613e2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh

@@ -363,7 +363,7 @@ test_expect_success 'octopus merges' '
 	EOF
 '
 
-test_expect_failure 'with --autosquash and --exec' '
+test_expect_success 'with --autosquash and --exec' '
 	git checkout -b with-exec H &&
 	echo Booh >B.t &&
 	test_tick &&

-- 
gitgitgadget

On 09/08/18 10:22, Johannes Schindelin wrote:

Hi Phillip,

On Mon, 6 Aug 2018, Phillip Wood wrote:

quoted

On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

The idea of `--exec` is to append an `exec` call after each `pick`.

Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.

The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.

With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.

Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.

While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.

Signed-off-by: Johannes Schindelin <redacted>
---
  sequencer.c              | 37 +++++++++++++++++++++++++++----------
  t/t3430-rebase-merges.sh |  2 +-
  2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 31038472f..ed2e694ff 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
  {
   const char *todo_file = rebase_path_todo();
   struct todo_list todo_list = TODO_LIST_INIT;
-	struct todo_item *item;
   struct strbuf *buf = &todo_list.buf;
   size_t offset = 0, commands_len = strlen(commands);
-	int i, first;
+	int i, insert;
  
   if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
  		return error(_("could not read '%s'."), todo_file);

@@ -4257,19 +4256,37 @@ int sequencer_add_exec_commands(const char

*commands)
   	return error(_("unusable todo list: '%s'"), todo_file);
   }
  -	first = 1;
-	/* insert <commands> before every pick except the first one */
-	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
-		if (item->command == TODO_PICK && !first) {
-			strbuf_insert(buf, item->offset_in_buf + offset,
-				      commands, 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 = -1;
+	for (i = 0; i < todo_list.nr; i++) {
+		enum todo_command command = todo_list.items[i].command;
+
+		if (insert >= 0) {
+			/* skip fixup/squash chains */
+			if (command == TODO_COMMENT)
+				continue;

insert is not updated so if the next command is not a fixup the exec
line will be inserted before the comment.

Yes, this is very much on purpose. Take this todo list, for example:

	pick 123456 this patch
	# pick 987654 this was an empty commit

You definitely do not want the `exec` to appear after that commented-out
empty commit.

Yes, I like it, I was just thinking out loud.

quoted

quoted

+			else if (is_fixup(command)) {
+				insert = i + 1;
+				continue;
+			}
+			strbuf_insert(buf,
+				      todo_list.items[insert].offset_in_buf +
+				      offset, commands, commands_len);
  			offset += commands_len;
+			insert = -1;
  		}
-		first = 0;
+
+		if (command == TODO_PICK || command == TODO_MERGE)
+			insert = i + 1;
   }
  
  	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	if (insert >= 0 || !offset)
+		strbuf_add(buf, commands, commands_len);

Having read your other message about this patch I think if you wanted to fix
the position of the final exec in the case where the todo list ends with a
comment you could do something like

	if (insert >= 0)
		strbuf_insert(buf,
			      todo_list.items[insert].offset_in_buf +
			      offset, commands, commands_len);
	else
		strbuf_add(buf, commands, commands_len);

That does not really work, as `insert` can point *after* the last line, in
which case `todo_list.items[insert]` is undefined (and in the worst case,
causes a segmentation fault).

Ah, I'd missed that, does changing the conditions to
if (insert >= 0 && insert < todo.list_nr) and
else if (insert >=0 || !offset) work?

quoted

I'm not sure it matters that much though

Well, it does matter to me. After having this in the back of my head, and
after your comment, I think it *is* worth the additional complexity after
all.

It would definitely be nice to have.

Best Wishes

Phillip

Will come up with a new iteration.

Ciao,
Dscho

Hi Phillip,

On Thu, 9 Aug 2018, Phillip Wood wrote:

On 09/08/18 10:22, Johannes Schindelin wrote:

quoted

On Mon, 6 Aug 2018, Phillip Wood wrote:

quoted

On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:

quoted

+			else if (is_fixup(command)) {
+				insert = i + 1;
+				continue;
+			}
+			strbuf_insert(buf,
+				      todo_list.items[insert].offset_in_buf +
+				      offset, commands, commands_len);
  			offset += commands_len;
+			insert = -1;
  		}
-		first = 0;
+
+		if (command == TODO_PICK || command == TODO_MERGE)
+			insert = i + 1;
   }
  
  	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	if (insert >= 0 || !offset)
+		strbuf_add(buf, commands, commands_len);

Having read your other message about this patch I think if you wanted to fix
the position of the final exec in the case where the todo list ends with a
comment you could do something like

	if (insert >= 0)
		strbuf_insert(buf,
			      todo_list.items[insert].offset_in_buf +
			      offset, commands, commands_len);
	else
		strbuf_add(buf, commands, commands_len);

That does not really work, as `insert` can point *after* the last line, in
which case `todo_list.items[insert]` is undefined (and in the worst case,
causes a segmentation fault).

Ah, I'd missed that, does changing the conditions to
if (insert >= 0 && insert < todo.list_nr) and
else if (insert >=0 || !offset) work?

That's pretty exactly what I did ;-)

Ciao,
Dscho