threads / patch / 45805

patchrebase -i: reread the todo list if `exec` touched it

Subject: [PATCH] rebase -i: reread the todo list if `exec` touched it

## tl;dr

3 messages between Apr 26, 2017 and Apr 27, 2017. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Johannes Schindelin· Apr 26, 2017, 19:17 UTC · lore
From: Stephen Hicks <sdh@google.com>

In the scripted version of the interactive rebase, there was no internal representation of the todo list; it was re-read before every command. That allowed the hack that an `exec` command could append (or even completely rewrite) the todo list.

This hack was broken by the partial conversion of the interactive rebase to C, and this patch reinstates it.

We also add a small test to verify that this fix does not regress in the future.

Signed-off-by: Stephen Hicks <sdh@google.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/rebase-reread-todo-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rebase-reread-todo-v1
	Based on https://github.com/git/git/pull/343
 sequencer.c                 | 22 ++++++++++++++++++++++
 t/t3428-rebase-edit-todo.sh | 14 ++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100755 t/t3428-rebase-edit-todo.sh
Show changes to 2 files +36 −0

sequencer.c, t/t3428-rebase-edit-todo.sh

diff --git a/sequencer.c b/sequencer.c
index 77afecaebf0..3fe9fcdab72 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1200,6 +1200,7 @@ struct todo_list {
 	struct todo_item *items;
 	int nr, alloc, current;
 	int done_nr, total_nr;
+	struct stat_data stat;
 };
 
 #define TODO_LIST_INIT { STRBUF_INIT }
@@ -1330,6 +1331,7 @@ static int count_commands(struct todo_list *todo_list)
 static int read_populate_todo(struct todo_list *todo_list,
 			struct replay_opts *opts)
 {
+	struct stat st;
 	const char *todo_file = get_todo_path(opts);
 	int fd, res;
 
@@ -1343,6 +1345,11 @@ static int read_populate_todo(struct todo_list *todo_list,
 	}
 	close(fd);
 
+	res = stat(todo_file, &st);
+	if (res)
+		return error(_("could not stat '%s'"), todo_file);
+	fill_stat_data(&todo_list->stat, &st);
+
 	res = parse_insn_buffer(todo_list->buf.buf, todo_list);
 	if (res) {
 		if (is_rebase_i(opts))
@@ -2028,10 +2035,25 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 		} else if (item->command == TODO_EXEC) {
 			char *end_of_arg = (char *)(item->arg + item->arg_len);
 			int saved = *end_of_arg;
+			struct stat st;
 
 			*end_of_arg = '\0';
 			res = do_exec(item->arg);
 			*end_of_arg = saved;
+
+			/* Reread the todo file if it has changed. */
+			if (res)
+				; /* fall through */
+			else if (stat(get_todo_path(opts), &st))
+				res = error_errno(_("could not stat '%s'"),
+						  get_todo_path(opts));
+			else if (match_stat_data(&todo_list->stat, &st)) {
+				todo_list_release(todo_list);
+				if (read_populate_todo(todo_list, opts))
+					res = -1; /* message was printed */
+				/* `current` will be incremented below */
+				todo_list->current = -1;
+			}
 		} else if (!is_noop(item->command))
 			return error(_("unknown command %d"), item->command);
 
diff --git a/t/t3428-rebase-edit-todo.sh b/t/t3428-rebase-edit-todo.sh
new file mode 100755
index 00000000000..b9292dfc2a3
--- /dev/null
+++ b/t/t3428-rebase-edit-todo.sh
@@ -0,0 +1,14 @@
+#!/bin/sh
+
+test_description='rebase should reread the todo file if an exec modifies it'
+
+. ./test-lib.sh
+
+test_expect_success 'rebase exec modifies rebase-todo' '
+	test_commit initial &&
+	todo=.git/rebase-merge/git-rebase-todo &&
+	git rebase HEAD -x "echo exec touch F >>$todo" &&
+	test -e F
+'
+
+test_done

base-commit: e2cb6ab84c94f147f1259260961513b40c36108a
-- 
2.12.2.windows.2.406.gd14a8f8640f
Steve Hicks· Apr 26, 2017, 21:11 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase -i: reread the todo list if `exec` touched it

On Wed, Apr 26, 2017 at 12:17 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 15 quoted lines
> From: Stephen Hicks <sdh@google.com>
>
> In the scripted version of the interactive rebase, there was no internal
> representation of the todo list; it was re-read before every command.
> That allowed the hack that an `exec` command could append (or even
> completely rewrite) the todo list.
>
> This hack was broken by the partial conversion of the interactive rebase
> to C, and this patch reinstates it.
>
> We also add a small test to verify that this fix does not regress in the
> future.
>
> Signed-off-by: Stephen Hicks <sdh@google.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Thanks for shepherding this through, Johannes!

For context on this "hack", I have a script [1] that allows passing multiple branches at once (or all branches beneath a given root). It rewrites the todo file with some extra operations, like "branch", "push", and "pop", allows editing the modified todo, and then rewrites back to exec's. The "branch" operation, in particular, appends an "exec git checkout $branch; git reset --hard $commit" to the end of the todo, so that no branches are moved until after all rebases are successful. I've found this multi-branch rebase workflow to be very productive, and have been missing it the last few months, so I'm looking forward to it working again soon.

[1] https://github.com/shicks/git-ir
Johannes Schindelin· Apr 27, 2017, 12:43 UTC · re: Steve Hicks · lore

Re: [PATCH] rebase -i: reread the todo list if `exec` touched it

Hi Steve,
On Wed, 26 Apr 2017, Steve Hicks wrote:
Show 20 quoted lines
> On Wed, Apr 26, 2017 at 12:17 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > From: Stephen Hicks <sdh@google.com>
> >
> > In the scripted version of the interactive rebase, there was no
> > internal representation of the todo list; it was re-read before every
> > command.  That allowed the hack that an `exec` command could append
> > (or even completely rewrite) the todo list.
> >
> > This hack was broken by the partial conversion of the interactive
> > rebase to C, and this patch reinstates it.
> >
> > We also add a small test to verify that this fix does not regress in
> > the future.
> 
> For context on this "hack", I have a script [1] that allows passing
> multiple branches at once (or all branches beneath a given root).  It
> rewrites the todo file with some extra operations, like "branch",
> "push", and "pop", allows editing the modified todo, and then rewrites
> back to exec's.

For what it's worth, I used a slightly different approach in the Git garden shears [*1*] (essentially, `git rebase -i -p` Done Right): I override the GIT_SEQUENCE_EDITOR to call the script in a specific mode that rewrites the todo and then launches the editor as per `git var GIT_EDITOR`.

Ciao, Johannes

Footnote *1*: https://github.com/git-for-windows/build-extra/blob/master/shears.sh

← back to recent threads