Re: [PATCH v7 11/16] sequencer: refactor skip_unnecessary_picks() to work on a todo_list

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

Hi Alban

On 10/02/2019 13:26, Alban Gruin wrote:
> This refactors skip_unnecessary_picks() to work on a todo_list.  As this
> function is only called by complete_action() (and thus is not used by
> rebase -p), the file-handling logic is completely dropped here.
> 
> Instead of truncating the todo list’s buffer, the items are moved to
> the beginning of the list, eliminating the need to reparse the list.
> This also means its buffer cannot be directly written to the disk.
> 
> rewrite_file() is then removed, as it is now unused.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 82 +++++++++++++----------------------------------------
>  1 file changed, 19 insertions(+), 63 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index eb8a622af0..eacaf52250 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4717,52 +4717,22 @@ int check_todo_list_from_file(struct repository *r)
>  	return res;
>  }
>  
> -static int rewrite_file(const char *path, const char *buf, size_t len)
> -{
> -	int rc = 0;
> -	int fd = open(path, O_WRONLY | O_TRUNC);
> -	if (fd < 0)
> -		return error_errno(_("could not open '%s' for writing"), path);
> -	if (write_in_full(fd, buf, len) < 0)
> -		rc = error_errno(_("could not write to '%s'"), path);
> -	if (close(fd) && !rc)
> -		rc = error_errno(_("could not close '%s'"), path);
> -	return rc;
> -}
> -
>  /* skip picking commits whose parents are unchanged */
> -static int skip_unnecessary_picks(struct repository *r, struct object_id *output_oid)
> +static int skip_unnecessary_picks(struct repository *r,
> +				  struct todo_list *todo_list,
> +				  struct object_id *base_oid)

Thanks for renaming that parameter, I think it's much clearer.

Best Wishes

Phillip
>  {
> -	const char *todo_file = rebase_path_todo();
> -	struct strbuf buf = STRBUF_INIT;
> -	struct todo_list todo_list = TODO_LIST_INIT;
>  	struct object_id *parent_oid;
> -	int fd, i;
> -
> -	if (!read_oneliner(&buf, rebase_path_onto(), 0))
> -		return error(_("could not read 'onto'"));
> -	if (get_oid(buf.buf, output_oid)) {
> -		strbuf_release(&buf);
> -		return error(_("need a HEAD to fixup"));
> -	}
> -	strbuf_release(&buf);
> -
> -	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
> -		return -1;
> -	if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) {
> -		todo_list_release(&todo_list);
> -		return -1;
> -	}
> +	int i;
>  
> -	for (i = 0; i < todo_list.nr; i++) {
> -		struct todo_item *item = todo_list.items + i;
> +	for (i = 0; i < todo_list->nr; i++) {
> +		struct todo_item *item = todo_list->items + i;
>  
>  		if (item->command >= TODO_NOOP)
>  			continue;
>  		if (item->command != TODO_PICK)
>  			break;
>  		if (parse_commit(item->commit)) {
> -			todo_list_release(&todo_list);
>  			return error(_("could not parse commit '%s'"),
>  				oid_to_hex(&item->commit->object.oid));
>  		}
> @@ -4771,42 +4741,26 @@ static int skip_unnecessary_picks(struct repository *r, struct object_id *output
>  		if (item->commit->parents->next)
>  			break; /* merge commit */
>  		parent_oid = &item->commit->parents->item->object.oid;
> -		if (!oideq(parent_oid, output_oid))
> +		if (!oideq(parent_oid, base_oid))
>  			break;
> -		oidcpy(output_oid, &item->commit->object.oid);
> +		oidcpy(base_oid, &item->commit->object.oid);
>  	}
>  	if (i > 0) {
> -		int offset = get_item_line_offset(&todo_list, i);
>  		const char *done_path = rebase_path_done();
>  
> -		fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
> -		if (fd < 0) {
> -			error_errno(_("could not open '%s' for writing"),
> -				    done_path);
> -			todo_list_release(&todo_list);
> -			return -1;
> -		}
> -		if (write_in_full(fd, todo_list.buf.buf, offset) < 0) {
> +		if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) {
>  			error_errno(_("could not write to '%s'"), done_path);
> -			todo_list_release(&todo_list);
> -			close(fd);
>  			return -1;
>  		}
> -		close(fd);
>  
> -		if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> -				 todo_list.buf.len - offset) < 0) {
> -			todo_list_release(&todo_list);
> -			return -1;
> -		}
> +		MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);
> +		todo_list->nr -= i;
> +		todo_list->current = 0;
>  
> -		todo_list.current = i;
> -		if (is_fixup(peek_command(&todo_list, 0)))
> -			record_in_rewritten(output_oid, peek_command(&todo_list, 0));
> +		if (is_fixup(peek_command(todo_list, 0)))
> +			record_in_rewritten(base_oid, peek_command(todo_list, 0));
>  	}
>  
> -	todo_list_release(&todo_list);
> -
>  	return 0;
>  }
>  
> @@ -4879,6 +4833,11 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  		return -1;
>  	}
>  
> +	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
> +		todo_list_release(&new_todo);
> +		return error(_("could not skip unnecessary pick commands"));
> +	}
> +
>  	if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
>  				    flags & ~(TODO_LIST_SHORTEN_IDS))) {
>  		todo_list_release(&new_todo);
> @@ -4887,9 +4846,6 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  
>  	todo_list_release(&new_todo);
>  
> -	if (opts->allow_ff && skip_unnecessary_picks(r, &oid))
> -		return error(_("could not skip unnecessary pick commands"));
> -
>  	if (checkout_onto(opts, onto_name, oid_to_hex(&oid), orig_head))
>  		return -1;
>  
>