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.gruin@xxxxxxxxx>
> ---
> 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
>
