Make git rebase work with --rebase-merges and --exec

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

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

sequencer.c +1 -4

index 31038472f..dda5cdbba 100644
@@ -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 */

sequencer.c +18 -0

+		}
+
+		/* 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)) {

sequencer.c +18 -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;
 		}

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 9 lines · writes:

From: Johannes Schindelin <[email protected]>

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.

Quoted 4 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.

Ahh, it may be "dirty" but "clever" ;-) As there is no way to say "add exec after only the third one", inserting before 'pick', assuming the lines before that would be a "previous group" that began with a pick and then possibly its amending operations, was sufficient. Makes sense.

"Johannes Schindelin via GitGitGadget" <[email protected]>

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4275

Quoted 12 lines · writes:

sequencer.c +12 -0

+	/*
+	 * 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[].

Quoted 20 lines

sequencer.c +18 -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;
 		}

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 22 lines · writes:

sequencer.c +1 -4

index 31038472f..dda5cdbba 100644
@@ -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 */

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4263

Quoted 10 lines

sequencer.c +5 -5

-	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.
+	 */

This is a tangent, but can a merge be amended with fixup/squash? I am hoping that I can use this machinery to augment Meta/Reintegrate logic someday, and amending merges to resolve semantic conflicts between topiocs in flight is what needs to happen constantly.

It appears the code treats TODO_PICK and TODO_MERGE the same way, so the answer to the question apparently is "yes", which is good.

"after every pick" needs to become "after every pick and merge", or if you prefer "after creating every new commit (i.e. pick and merge)".

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4264

Quoted 1 line

sequencer.c +1 -0

+	insert_final_commands = 1;

We assume, before entering the loop, that we'd need to append another exec at the end.

Quoted 5 lines

sequencer.c +5 -0

+	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) {

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4271

Quoted 2 lines

sequencer.c +2 -0

+			i++;
+			continue;

If we ever see a todo-list without any pick/merge, then insert_final is still 1 when we leave the loop and we will add one single exec at the end. Which may or may not make sense---I dunno, as I do not offhand think of a reason why the user would give us such a sequence in the first place, so it probably may not matter in practice.

Quoted 18 lines

sequencer.c +18 -0

+		}
+
+		/* 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)) {

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4298

Quoted 10 lines

sequencer.c +8 -1

+				i = j;
+				continue;
+			}
+			break;
 		}
-		first = 0;
+
+		if (i >= todo_list.nr) {
+			insert_final_commands = 1;
+			break;

We saw pick or merge and then skipped zero or more fixups. If we reached the end, then we need to append one more to run the command after this last group. Makes sense.

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4304

Quoted 6 lines

sequencer.c +6 -0

+		}
+
+		strbuf_insert(buf, todo_list.items[i].offset_in_buf + offset,
+			      commands, commands_len);
+		offset += commands_len;
+		insert_final_commands = 0;

Otherwise, we finished a group so we insert an exec and move on, after saying that we do not need one more, unless we see pick/merge

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4309

Quoted 6 lines

sequencer.c +2 -1

 	}
 
 	/* append final <commands> */
-	strbuf_add(buf, commands, commands_len);
+	if (insert_final_commands)
+		strbuf_add(buf, commands, commands_len);

When we leave the loop without adding exec to a group we saw, we want to add one more exec, which is done here.

I am not exectly sure if the above loop is what you really want, though. I would have found the flow of the logic simpler to follow if the loop were structured like this:

append_exec = false
for each command:
if append_exec:
add command to execute after the previous block append_exec = false
if command is neither pick or merge:
continue
skip fixup or squash
# at this point we know we are at the end append_exec = true
if append_exec:
add command to execute after the last block

essentially, the loop uses the flag not to keep track of the need to emit only the final command, but the need to emit a command before processing any command in the event stream (or the end of stream).

The above assumes that we do not want any exec if there is no pick/merge, though. As I said already, I am assuming that it does not matter either way in practice, but if it mattered, I'd find it more natural not to run any command if we did not create any commit.

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);

Signature · 1 line

gitgitgadget

Quoted 5 lines

If we ever see a todo-list without any pick/merge, then insert_final is still 1 when we leave the loop and we will add one single exec at the end. Which may or may not make sense---I dunno, as I do not offhand think of a reason why the user would give us such a sequence in the first place, so it probably may not matter in practice.

Think of the noop command. It was introduced specifically to allow rebasing patches interactively to an upstream that already applied the local patches. In that case, an --exec should still run at least once, to avoid negative surprises.

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

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

sequencer.c +4 -8

index 31038472f..ed2e694ff 100644
@@ -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*

sequencer.c +8 -0

+			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;
  		}

sequencer.c +0 -0

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

t/t3430-rebase-merges.sh +1 -1

index 0bf5eaa37..90ae613e2 100755
@@ -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 Johannes

Quoted 62 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
  sequencer.c              | 37 +++++++++++++++++++++++++++---------- t/t3430-rebase-merges.sh |  2 +-

  2 files changed, 28 insertions(+), 11 deletions(-)

sequencer.c +4 -8

index 31038472f..ed2e694ff 100644
@@ -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*

sequencer.c int sequencer_add_exec_commands(const char · line 4272

Quoted 10 lines

sequencer.c +10 -0

+     * 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 9 lines

sequencer.c +8 -0

+            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;

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4289

Quoted 10 lines

sequencer.c +5 -2

           }
-        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

Quoted 15 lines

sequencer.c +0 -0

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

t/t3430-rebase-merges.sh +0 -0

index 0bf5eaa37..90ae613e2 100755
@@ -363,7 +363,7 @@ test_expect_success 'octopus merges' '

      EOF
  '

t/t3430-rebase-merges.sh +1 -1

-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 &&

Quoted 66 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

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

sequencer.c +9 -7

index 31038472f..ed2e694ff 100644
@@ -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;

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4271

Quoted 5 lines

sequencer.c +5 -0

+
+		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 10 lines

sequencer.c +8 -0

+			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;
  		}

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4289

Quoted 10 lines

sequencer.c +5 -2

-		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

Hi Phillip,

Quoted 67 lines · Phillip Wood

Quoted 66 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

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

sequencer.c +9 -7

index 31038472f..ed2e694ff 100644
@@ -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;

Quoted 8 lines

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4271

Quoted 5 lines

sequencer.c +5 -0

+
+		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 25 lines

Quoted 10 lines

sequencer.c +8 -0

+			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;
  		}

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4289

Quoted 10 lines

sequencer.c +5 -2

-		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

Quoted 6 lines

	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).

Quoted 1 line

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

Quoted 37 lines · Johannes Schindelin

Quoted 33 lines · Phillip Wood

Quoted 11 lines · Johannes Schindelin via GitGitGadget

sequencer.c +8 -0

+			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;
  		}

sequencer.c int sequencer_add_exec_commands(const char *commands) · line 4289

Quoted 10 lines

sequencer.c +5 -2

-		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);

Quoted 3 lines

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?