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
> 

Reply via email to