On 31/07/18 18:59, Alban Gruin wrote: > This rewrites the edit-todo functionality from shell to C. > > To achieve that, a new command mode, `edit-todo`, is added, and the > `write-edit-todo` flag is removed, as the shell script does not need to > write the edit todo help message to the todo list anymore. > > The shell version is then stripped in favour of a call to the helper. > > Signed-off-by: Alban Gruin <alban.gr...@gmail.com> > --- > No changes since v4. > > builtin/rebase--helper.c | 13 ++++++++----- > git-rebase--interactive.sh | 11 +---------- > rebase-interactive.c | 31 +++++++++++++++++++++++++++++++ > rebase-interactive.h | 1 + > 4 files changed, 41 insertions(+), 15 deletions(-) > > diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c > index 05e73e71d4..731a64971d 100644 > --- a/builtin/rebase--helper.c > +++ b/builtin/rebase--helper.c > @@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = > { > int cmd_rebase__helper(int argc, const char **argv, const char *prefix) > { > struct replay_opts opts = REPLAY_OPTS_INIT; > - unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo > = 0; > + unsigned flags = 0, keep_empty = 0, rebase_merges = 0; > int abbreviate_commands = 0, rebase_cousins = -1; > enum { > CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS, > CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH, > - ADD_EXEC, APPEND_TODO_HELP > + ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO > } command = 0; > struct option options[] = { > OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")), > @@ -28,8 +28,6 @@ int cmd_rebase__helper(int argc, const char **argv, const > char *prefix) > OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge > commits")), > OPT_BOOL(0, "rebase-cousins", &rebase_cousins, > N_("keep original branch points of cousins")), > - OPT_BOOL(0, "write-edit-todo", &write_edit_todo, > - N_("append the edit-todo message to the todo-list")), > OPT_CMDMODE(0, "continue", &command, N_("continue rebase"), > CONTINUE), > OPT_CMDMODE(0, "abort", &command, N_("abort rebase"), > @@ -50,6 +48,9 @@ int cmd_rebase__helper(int argc, const char **argv, const > char *prefix) > N_("insert exec commands in todo list"), ADD_EXEC), > OPT_CMDMODE(0, "append-todo-help", &command, > N_("insert the help in the todo list"), > APPEND_TODO_HELP), > + OPT_CMDMODE(0, "edit-todo", &command, > + N_("edit the todo list during an interactive > rebase"), > + EDIT_TODO), > OPT_END() > }; > > @@ -90,6 +91,8 @@ int cmd_rebase__helper(int argc, const char **argv, const > char *prefix) > if (command == ADD_EXEC && argc == 2) > return !!sequencer_add_exec_commands(argv[1]); > if (command == APPEND_TODO_HELP && argc == 1) > - return !!append_todo_help(write_edit_todo, keep_empty); > + return !!append_todo_help(0, keep_empty); > + if (command == EDIT_TODO && argc == 1) > + return !!edit_todo_list(flags); > usage_with_options(builtin_rebase_helper_usage, options); > } > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index 94c23a7af2..2defe607f4 100644 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -108,16 +108,7 @@ initiate_action () { > --continue > ;; > edit-todo) > - git stripspace --strip-comments <"$todo" >"$todo".new > - mv -f "$todo".new "$todo" > - collapse_todo_ids > - git rebase--helper --append-todo-help --write-edit-todo > - > - git_sequence_editor "$todo" || > - die "$(gettext "Could not execute editor")" > - expand_todo_ids > - > - exit > + exec git rebase--helper --edit-todo > ;; > show-current-patch) > exec git show REBASE_HEAD -- > diff --git a/rebase-interactive.c b/rebase-interactive.c > index d7996bc8d9..403ecbf3c9 100644 > --- a/rebase-interactive.c > +++ b/rebase-interactive.c > @@ -66,3 +66,34 @@ int append_todo_help(unsigned edit_todo, unsigned > keep_empty) > > return ret; > } > + > +int edit_todo_list(unsigned flags) > +{ > + struct strbuf buf = STRBUF_INIT; > + const char *todo_file = rebase_path_todo(); > + FILE *todo; > + > + if (strbuf_read_file(&buf, todo_file, 0) < 0) > + return error_errno(_("could not read '%s'."), todo_file); > + > + strbuf_stripspace(&buf, 1); > + todo = fopen_or_warn(todo_file, "w");
This truncates the existing file, if there are any errors writing the new one then the user has lost the old one. write_message() in sequencer.c avoids this problem by writing a new file and then renaming it if the write is successful, maybe it is worth exporting it so it can be used elsewhere. > + if (!todo) { > + strbuf_release(&buf); > + return 1; > + } > + > + strbuf_write(&buf, todo); > + fclose(todo); There needs to be some error checking after the write and the close (using write_message() would mean you only have to check for errors in one place) Best Wishes Phillip > + strbuf_release(&buf); > + > + transform_todos(flags | TODO_LIST_SHORTEN_IDS); > + append_todo_help(1, 0); > + > + if (launch_sequence_editor(todo_file, NULL, NULL)) > + return 1; > + > + transform_todos(flags & ~(TODO_LIST_SHORTEN_IDS)); > + > + return 0; > +} > diff --git a/rebase-interactive.h b/rebase-interactive.h > index 47372624e0..155219e742 100644 > --- a/rebase-interactive.h > +++ b/rebase-interactive.h > @@ -2,5 +2,6 @@ > #define REBASE_INTERACTIVE_H > > int append_todo_help(unsigned edit_todo, unsigned keep_empty); > +int edit_todo_list(unsigned flags); > > #endif >