diff mbox series

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

Message ID 20181007195418.25752-9-alban.gruin@gmail.com (mailing list archive)
State New, archived
Headers show
Series sequencer: refactor functions working on a todo_list | expand

Commit Message

Alban Gruin Oct. 7, 2018, 7:54 p.m. UTC 
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(-)

Comments

Phillip Wood Oct. 11, 2018, 1:51 p.m. UTC | #1 
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[];
>
Alban Gruin Oct. 11, 2018, 5:06 p.m. UTC | #2 
Le 11/10/2018 à 15:51, Phillip Wood a écrit :
> On 07/10/2018 20:54, Alban Gruin wrote:
>> +    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.

You’re right, I didn’t notice that.

> 
> Best Wishes
> 
> Phillip
> 

Cheers,
Alban
diff mbox series

Patch

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);
-	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);
+	}
+
+	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[];