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

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

[Johannes Schindelin via 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              | 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

[PATCH 1/2] t3430: demonstrate what -r, --autosquash & --exec should do

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

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

[Junio C Hamano]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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.

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

Understandable.

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

Yeah, while reading the "Let's change" paragraph, that exactly was
what came to my mind---the old one was buggy because we didn't
anticipate that 'pick' won't stay to be the first command in the
block that gives us a good anchoring point to find the end of the
previous block, so assuming that 'pick' will stay to be the only
command the users want to add exec after would be making a design
mistake that is quite similar without learning any lesson from the
bug being fixed.

I do agree that it is a good idea to add exec after merge; what I
wanted to say with the above is that I just do not think it is
"While at it"; it would be an integral part of supporting --exec
in rebase-merges mode.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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.
> +	 */

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

> +	insert_final_commands = 1;

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

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

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.

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

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.

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

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

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Fri, 3 Aug 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
> 
> 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.

Yes, it was clever. It is the kind of clever that causes regressions when
new features are introduced that were not anticipated by the cleverness.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Fri, 3 Aug 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > 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.
> > +	 */
> 
> 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.

When I rewrote `rearrange_squash()` in C, I tried to be the kind of clever
that anticipates future features and accommodates for them:

		if (!item->commit || item->command == TODO_DROP) {
			subjects[i] = NULL;
			continue;
		}

This is the snippet of code that will skip entries in the todo list from
being eligible to be fixed up. As you can see, the code is not 100%
future-proof: if somebody wants to introduce another command like `drop`
(i.e. a command that takes a commit as parameter, but does not create a
commit), the clause will have to be extended.

Technically, I could have changed the logic in `add_exec_commands()` to
use the same heuristic. But that would change the behavior, and this patch
series is about fixing a bug, not about changing behavior.

> "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)".
> 
> > +	insert_final_commands = 1;
> 
> We assume, before entering the loop, that we'd need to append
> another exec at the end.

Right. This is what the current code does, and I am not willing to change
that in this bug fix.

(This smells like the exact suggestions I was too happy in the past to
consider, causing regressions like that vexing one where the rock solid,
battle tested hideDotGit support was broken as a consequence of the code
review on this list. It is totally my fault, I should learn from such
experiences and be very wary of potentially-breaking suggestions.)

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

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

ETOOMUCHPYTHON

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

My original reasoning for not doing it that way was to keep the final
commands even in the case that there is no pick.

And that is a hard requirement: it would possibly cause regressions, and I
do not have the time to take care of regressions caused by review these
days.

Another thing your suggestion completely misses is that I went out of my
way to treat comments in the way that "pick, fixup, comment, pick" would
see the "exec" inserted *before* the comment, i.e. comments at the end of
fixup/squash chains would not be considered part of the chain.

But your comments made me look at it again, and I think your idea can be
salvaged. We just have to use an index instead of a Boolean, and extend
the index upon seeing a fixup, and delaying the decision if we see a
comment.

I asked GitGitGadget to submit the second iteration of this patch series.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Ah, yes, it probably makes sense when you have `noop`; even if there
is no picks and merges that change the history from previous state
(which presumably matches the state the user started running the
current "rebase -i" session), after the whole sequence you would
want run it one iteration.

In any case, if the current code without this change works like so,
there is no point in redesigning that part of the semantics at all.

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

[Junio C Hamano]

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

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

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

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

[Johannes Schindelin]

Hi Junio,

On Fri, 3 Aug 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > +	/*
> > +	 * 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[].

I rewrote this code, and the `j` variable is not even there anymore.

Ciao,
Dscho

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

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

[Johannes Schindelin via 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

[PATCH v2 1/2] t3430: demonstrate what -r, --autosquash & --exec should do

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

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

[Phillip Wood]

Hi Johannes
On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:
> 
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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 <johannes.schindelin@gmx.de>
> ---
>   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.

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

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

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

[Phillip Wood]

On 06/08/18 16:23, Phillip Wood wrote:
> 
> Hi Johannes
> On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:
>>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> 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 <johannes.schindelin@gmx.de>
>> ---
>>   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.
> 
>> +            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
> 
>>       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 &&
>>
> 

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

[Johannes Schindelin]

Hi Phillip,

On Mon, 6 Aug 2018, Phillip Wood wrote:

> On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:
> > 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > 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 <johannes.schindelin@gmx.de>
> > ---
> >   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.

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

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

[Phillip Wood]

On 09/08/18 10:22, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Mon, 6 Aug 2018, Phillip Wood wrote:
> 
>> On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:
>>>
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> 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 <johannes.schindelin@gmx.de>
>>> ---
>>>   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.

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

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

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

[Johannes Schindelin]

Hi Phillip,

On Thu, 9 Aug 2018, Phillip Wood wrote:

> On 09/08/18 10:22, Johannes Schindelin wrote:
> > 
> > On Mon, 6 Aug 2018, Phillip Wood wrote:
> > 
> >> On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:
> >>>
> >>> +			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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin via 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 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

[PATCH v3 1/2] t3430: demonstrate what -r, --autosquash & --exec should do

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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