{"thread":{"id":"45805","subject":"[PATCH] rebase -i: reread the todo list if `exec` touched it","startedAt":"2017-04-26T19:18:03Z","lastAt":"2017-04-27T12:43:51Z","messageCount":3,"participants":["Johannes Schindelin","Steve Hicks"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"317952","messageId":"ffb8b1ddb7a8a22643d569565e1a4929b883fba5.1493234134.git.johannes.schindelin@gmx.de","threadId":"45805","inReplyTo":null,"subject":"[PATCH] rebase -i: reread the todo list if `exec` touched it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-26T19:17:40Z","receivedAt":"2017-04-26T19:18:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Stephen Hicks \n\nIn the scripted version of the interactive rebase, there was no internal\nrepresentation of the todo list; it was re-read before every command.\nThat allowed the hack that an `exec` command could append (or even\ncompletely rewrite) the todo list.\n\nThis hack was broken by the partial conversion of the interactive rebase\nto C, and this patch reinstates it.\n\nWe also add a small test to verify that this fix does not regress in the\nfuture.\n\nSigned-off-by: Stephen Hicks \nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/rebase-reread-todo-v1\nFetch-It-Via: git fetch https://github.com/dscho/git rebase-reread-todo-v1\n\n\tBased on https://github.com/git/git/pull/343\n\n sequencer.c | 22 ++++++++++++++++++++++\n t/t3428-rebase-edit-todo.sh | 14 ++++++++++++++\n 2 files changed, 36 insertions(+)\n create mode 100755 t/t3428-rebase-edit-todo.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 77afecaebf0..3fe9fcdab72 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1200,6 +1200,7 @@ struct todo_list {\n \tstruct todo_item *items;\n \tint nr, alloc, current;\n \tint done_nr, total_nr;\n+\tstruct stat_data stat;\n };\n \n #define TODO_LIST_INIT { STRBUF_INIT }\n@@ -1330,6 +1331,7 @@ static int count_commands(struct todo_list *todo_list)\n static int read_populate_todo(struct todo_list *todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n+\tstruct stat st;\n \tconst char *todo_file = get_todo_path(opts);\n \tint fd, res;\n \n@@ -1343,6 +1345,11 @@ static int read_populate_todo(struct todo_list *todo_list,\n \t}\n \tclose(fd);\n \n+\tres = stat(todo_file, &st);\n+\tif (res)\n+\t\treturn error(_(\"could not stat '%s'\"), todo_file);\n+\tfill_stat_data(&todo_list->stat, &st);\n+\n \tres = parse_insn_buffer(todo_list->buf.buf, todo_list);\n \tif (res) {\n \t\tif (is_rebase_i(opts))\n@@ -2028,10 +2035,25 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n \t\t} else if (item->command == TODO_EXEC) {\n \t\t\tchar *end_of_arg = (char *)(item->arg + item->arg_len);\n \t\t\tint saved = *end_of_arg;\n+\t\t\tstruct stat st;\n \n \t\t\t*end_of_arg = '\\0';\n \t\t\tres = do_exec(item->arg);\n \t\t\t*end_of_arg = saved;\n+\n+\t\t\t/* Reread the todo file if it has changed. */\n+\t\t\tif (res)\n+\t\t\t\t; /* fall through */\n+\t\t\telse if (stat(get_todo_path(opts), &st))\n+\t\t\t\tres = error_errno(_(\"could not stat '%s'\"),\n+\t\t\t\t\t\t get_todo_path(opts));\n+\t\t\telse if (match_stat_data(&todo_list->stat, &st)) {\n+\t\t\t\ttodo_list_release(todo_list);\n+\t\t\t\tif (read_populate_todo(todo_list, opts))\n+\t\t\t\t\tres = -1; /* message was printed */\n+\t\t\t\t/* `current` will be incremented below */\n+\t\t\t\ttodo_list->current = -1;\n+\t\t\t}\n \t\t} else if (!is_noop(item->command))\n \t\t\treturn error(_(\"unknown command %d\"), item->command);\n \ndiff --git a/t/t3428-rebase-edit-todo.sh b/t/t3428-rebase-edit-todo.sh\nnew file mode 100755\nindex 00000000000..b9292dfc2a3\n--- /dev/null\n+++ b/t/t3428-rebase-edit-todo.sh\n@@ -0,0 +1,14 @@\n+#!/bin/sh\n+\n+test_description='rebase should reread the todo file if an exec modifies it'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'rebase exec modifies rebase-todo' '\n+\ttest_commit initial &&\n+\ttodo=.git/rebase-merge/git-rebase-todo &&\n+\tgit rebase HEAD -x \"echo exec touch F >>$todo\" &&\n+\ttest -e F\n+'\n+\n+test_done\n\nbase-commit: e2cb6ab84c94f147f1259260961513b40c36108a\n-- \n2.12.2.windows.2.406.gd14a8f8640f\n"},{"id":"318000","messageId":"CAABDHWZdsbP_NYW36nXc7WwjZf53PeEQrfaCUQrDpU4Zd9R2ew@mail.gmail.com","threadId":"45805","inReplyTo":"ffb8b1ddb7a8a22643d569565e1a4929b883fba5.1493234134.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] rebase -i: reread the todo list if `exec` touched it","fromName":"Steve Hicks","fromEmail":"sdh@google.com","sentAt":"2017-04-26T21:11:05Z","receivedAt":"2017-04-26T21:11:11Z","isPatch":true,"sender":{"key":"sdh@google.com","avatar":null},"body":"On Wed, Apr 26, 2017 at 12:17 PM, Johannes Schindelin\n wrote:\n> From: Stephen Hicks \n>\n> In the scripted version of the interactive rebase, there was no internal\n> representation of the todo list; it was re-read before every command.\n> That allowed the hack that an `exec` command could append (or even\n> completely rewrite) the todo list.\n>\n> This hack was broken by the partial conversion of the interactive rebase\n> to C, and this patch reinstates it.\n>\n> We also add a small test to verify that this fix does not regress in the\n> future.\n>\n> Signed-off-by: Stephen Hicks \n> Signed-off-by: Johannes Schindelin \n\nThanks for shepherding this through, Johannes!\n\nFor context on this \"hack\", I have a script [1] that allows passing\nmultiple branches at once (or all branches beneath a given root). It\nrewrites the todo file with some extra operations, like \"branch\",\n\"push\", and \"pop\", allows editing the modified todo, and then rewrites\nback to exec's. The \"branch\" operation, in particular, appends an\n\"exec git checkout $branch; git reset --hard $commit\" to the end of\nthe todo, so that no branches are moved until after all rebases are\nsuccessful. I've found this multi-branch rebase workflow to be very\nproductive, and have been missing it the last few months, so I'm\nlooking forward to it working again soon.\n\n[1] https://github.com/shicks/git-ir\n"},{"id":"318067","messageId":"alpine.DEB.2.20.1704271438290.3480@virtualbox","threadId":"45805","inReplyTo":"CAABDHWZdsbP_NYW36nXc7WwjZf53PeEQrfaCUQrDpU4Zd9R2ew@mail.gmail.com","subject":"Re: [PATCH] rebase -i: reread the todo list if `exec` touched it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-27T12:43:15Z","receivedAt":"2017-04-27T12:43:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Steve,\n\nOn Wed, 26 Apr 2017, Steve Hicks wrote:\n\n> On Wed, Apr 26, 2017 at 12:17 PM, Johannes Schindelin\n> wrote:\n> > From: Stephen Hicks \n> >\n> > In the scripted version of the interactive rebase, there was no\n> > internal representation of the todo list; it was re-read before every\n> > command. That allowed the hack that an `exec` command could append\n> > (or even completely rewrite) the todo list.\n> >\n> > This hack was broken by the partial conversion of the interactive\n> > rebase to C, and this patch reinstates it.\n> >\n> > We also add a small test to verify that this fix does not regress in\n> > the future.\n> \n> For context on this \"hack\", I have a script [1] that allows passing\n> multiple branches at once (or all branches beneath a given root). It\n> rewrites the todo file with some extra operations, like \"branch\",\n> \"push\", and \"pop\", allows editing the modified todo, and then rewrites\n> back to exec's.\n\nFor what it's worth, I used a slightly different approach in the Git\ngarden shears [*1*] (essentially, `git rebase -i -p` Done Right): I\noverride the GIT_SEQUENCE_EDITOR to call the script in a specific mode\nthat rewrites the todo and then launches the editor as per `git var\nGIT_EDITOR`.\n\nCiao,\nJohannes\n\nFootnote *1*:\nhttps://github.com/git-for-windows/build-extra/blob/master/shears.sh\n"}]}