Re: [GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Aug 7, 2018, 14:00 UTC
Message-ID: <325fd5d2-2a93-2270-7d1e-394c265f42d4@talktalk.net>
In-Reply-To: <20180731180003.5421-5-alban.gruin@gmail.com>

On 31/07/18 18:59, Alban Gruin wrote:

Show 108 quoted lines

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

Show 7 quoted lines

> +	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

Show 24 quoted lines

> +	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
>