Re: [PATCH 08/15] sequencer: change complete_action() to use the refactored functions

[Phillip Wood <phillip.wood@talktalk.net>]

On 07/10/2018 20:54, Alban Gruin wrote:
> complete_action() used functions that read the todo-list file, made some
> changes to it, and wrote it back to the disk.
> 
> The previous commits were dedicated to separate the part that deals with
> the file from the actual logic of these functions.  Now that this is
> done, we can call directly the "logic" functions to avoid useless file
> access.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   builtin/rebase--interactive.c | 13 +-----
>   sequencer.c                   | 76 +++++++++++++++++------------------
>   sequencer.h                   |  2 +-
>   3 files changed, 38 insertions(+), 53 deletions(-)
> 
> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> index eef1ff2e83..0700339f90 100644
> --- a/builtin/rebase--interactive.c
> +++ b/builtin/rebase--interactive.c
> @@ -71,7 +71,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
>   	const char *head_hash = NULL;
>   	char *revisions = NULL, *shortrevisions = NULL;
>   	struct argv_array make_script_args = ARGV_ARRAY_INIT;
> -	FILE *todo_list_file;
>   	struct todo_list todo_list = TODO_LIST_INIT;
>   
>   	if (prepare_branch_to_be_rebased(opts, switch_to))
> @@ -94,14 +93,6 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
>   	if (!upstream && squash_onto)
>   		write_file(path_squash_onto(), "%s\n", squash_onto);
>   
> -	todo_list_file = fopen(rebase_path_todo(), "w");
> -	if (!todo_list_file) {
> -		free(revisions);
> -		free(shortrevisions);
> -
> -		return error_errno(_("could not open %s"), rebase_path_todo());
> -	}
> -
>   	argv_array_pushl(&make_script_args, "", revisions, NULL);
>   	if (restrict_revision)
>   		argv_array_push(&make_script_args, restrict_revision);
> @@ -109,15 +100,13 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
>   	ret = sequencer_make_script(&todo_list.buf,
>   				    make_script_args.argc, make_script_args.argv,
>   				    flags);

I think it would be clearer to parse the todo list here explicitly 
rather than doing it implicitly in complete_action()

> -	fputs(todo_list.buf.buf, todo_list_file);
> -	fclose(todo_list_file);
>   
>   	if (ret)
>   		error(_("could not generate todo list"));
>   	else {
>   		discard_cache() >   		ret = complete_action(opts, flags, shortrevisions, onto_name, onto,
> -				      head_hash, cmd, autosquash);
> +				      head_hash, cmd, autosquash, &todo_list);
>   	}
>   
>   	free(revisions);
> diff --git a/sequencer.c b/sequencer.c
> index dfb8d1c974..b37935e5ab 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4624,93 +4624,89 @@ static int skip_unnecessary_picks(struct object_id *output_oid)
>   	return 0;
>   }
>   
> +static int todo_list_rearrange_squash(struct todo_list *todo_list);
> +
>   int complete_action(struct replay_opts *opts, unsigned flags,
>   		    const char *shortrevisions, const char *onto_name,
>   		    const char *onto, const char *orig_head, const char *cmd,
> -		    unsigned autosquash)
> +		    unsigned autosquash, struct todo_list *todo_list)
>   {
>   	const char *shortonto, *todo_file = rebase_path_todo();
> -	struct todo_list todo_list = TODO_LIST_INIT;
> -	struct strbuf *buf = &(todo_list.buf);
> +	struct todo_list new_todo = TODO_LIST_INIT;
> +	struct strbuf *buf = &todo_list->buf;
>   	struct object_id oid;
> -	struct stat st;
> +	int command_count;
>   
>   	get_oid(onto, &oid);
>   	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
>   
> -	if (!lstat(todo_file, &st) && st.st_size == 0 &&
> -	    write_message("noop\n", 5, todo_file, 0))
> -		return -1;
> +	if (buf->len == 0)
> +		strbuf_add(buf, "noop\n", 5);
> +
> +	if (todo_list_parse_insn_buffer(buf->buf, todo_list))
> +		BUG("unusable todo list");
>   
> -	if (autosquash && rearrange_squash_in_todo_file())
> +	if (autosquash && todo_list_rearrange_squash(todo_list))
>   		return -1;
>   
>   	if (cmd && *cmd)
> -		sequencer_add_exec_commands(cmd);
> +		todo_list_add_exec_commands(todo_list, cmd);
>   
> -	if (strbuf_read_file(buf, todo_file, 0) < 0)
> -		return error_errno(_("could not read '%s'."), todo_file);
> -
> -	if (todo_list_parse_insn_buffer(buf->buf, &todo_list)) {
> -		todo_list_release(&todo_list);
> -		return error(_("unusable todo list: '%s'"), todo_file);
> -	}
> -
> -	if (count_commands(&todo_list) == 0) {
> +	command_count = count_commands(todo_list);
> +	if (command_count == 0) {
>   		apply_autostash(opts);
>   		sequencer_remove_state(opts);
> -		todo_list_release(&todo_list);
>   
>   		return error(_("nothing to do"));
>   	}
>   
> +	todo_list_transform(todo_list, flags | TODO_LIST_SHORTEN_IDS);
> +
>   	strbuf_addch(buf, '\n');
>   	strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>   				      "Rebase %s onto %s (%d commands)",
> -				      count_commands(&todo_list)),
> -			      shortrevisions, shortonto, count_commands(&todo_list));
> +				      command_count),
> +			      shortrevisions, shortonto, command_count);
>   	append_todo_help(0, flags & TODO_LIST_KEEP_EMPTY, buf);
>   
> -	if (write_message(buf->buf, buf->len, todo_file, 0)) {
> -		todo_list_release(&todo_list);
> -		return -1;
> -	}
> +	if (write_message(buf->buf, buf->len, todo_file, 0))
> +		return error_errno(_("could not write '%s'"), todo_file);
>   
>   	if (copy_file(rebase_path_todo_backup(), todo_file, 0666))
>   		return error(_("could not copy '%s' to '%s'."), todo_file,
>   			     rebase_path_todo_backup());
>   
> -	if (transform_todo_file(flags | TODO_LIST_SHORTEN_IDS))
> -		return error(_("could not transform the todo list"));
> -
> -	strbuf_reset(buf);
> -
> -	if (launch_sequence_editor(todo_file, buf, NULL)) {
> +	if (launch_sequence_editor(todo_file, &new_todo.buf, NULL)) {
>   		apply_autostash(opts);
>   		sequencer_remove_state(opts);
> -		todo_list_release(&todo_list);
>   
>   		return -1;
>   	}
>   
> -	strbuf_stripspace(buf, 1);
> -	if (buf->len == 0) {
> +	strbuf_stripspace(&new_todo.buf, 1);
> +	if (new_todo.buf.len == 0) {
>   		apply_autostash(opts);
>   		sequencer_remove_state(opts);
> -		todo_list_release(&todo_list);
> +		todo_list_release(&new_todo);
>   
>   		return error(_("nothing to do"));
>   	}
>   
> -	todo_list_release(&todo_list);
> -
> -	if (check_todo_list_from_file()) {
> +	if (todo_list_check(todo_list, &new_todo)) {
>   		checkout_onto(opts, onto_name, onto, orig_head);
> +		todo_list_release(&new_todo);
> +
>   		return -1;
>   	}
>   
> -	if (transform_todo_file(flags & ~(TODO_LIST_SHORTEN_IDS)))
> -		return error(_("could not transform the todo list"));
> +	todo_list_transform(&new_todo, flags & ~(TODO_LIST_SHORTEN_IDS));
> +
> +	if (rewrite_file(todo_file, new_todo.buf.buf, new_todo.buf.len) < 0) {
> +		todo_list_release(&new_todo);
> +		return error_errno(_("could not write '%s'"), todo_file);
> +	}

rewrite_file() can truncate the old version of the file if there is an 
error when writing the new version, I think it would be better to use 
write_message() instead as that atomically updates the file. The same 
applies to patch 5 (refactor rearrange_squash()) after which I think 
there will be no callers to rewrite_file() so it can be deleted.

Best Wishes

Phillip

> +
> +	todo_list_release(&new_todo);
>   
>   	if (opts->allow_ff && skip_unnecessary_picks(&oid))
>   		return error(_("could not skip unnecessary pick commands"));
> diff --git a/sequencer.h b/sequencer.h
> index 21d9ba09ab..5bd3b79282 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -141,7 +141,7 @@ int check_todo_list_from_file(void);
>   int complete_action(struct replay_opts *opts, unsigned flags,
>   		    const char *shortrevisions, const char *onto_name,
>   		    const char *onto, const char *orig_head, const char *cmd,
> -		    unsigned autosquash);
> +		    unsigned autosquash, struct todo_list *todo_list);
>   int rearrange_squash_in_todo_file(void);
>   
>   extern const char sign_off_header[];
>