git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Alban Gruin <alban.gruin@gmail.com>, git@vger.kernel.org
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 08/15] sequencer: change complete_action() to use the refactored functions
Date: Thu, 11 Oct 2018 14:51:11 +0100
Message-ID: <d0adf861-3046-f0b6-3217-c89d92319e43@talktalk.net> (raw)
In-Reply-To: <20181007195418.25752-9-alban.gruin@gmail.com>

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[];
> 


  reply index

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-07 19:54 [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
2018-10-07 19:54 ` [PATCH 01/15] sequencer: clear the number of items of a todo_list before parsing Alban Gruin
2018-10-07 19:54 ` [PATCH 02/15] sequencer: make the todo_list structure public Alban Gruin
2018-10-07 19:54 ` [PATCH 03/15] sequencer: refactor check_todo_list() to work on a todo_list Alban Gruin
2018-10-11 11:23   ` Phillip Wood
2018-10-07 19:54 ` [PATCH 04/15] sequencer: refactor sequencer_add_exec_commands() " Alban Gruin
2018-10-11 11:25   ` Phillip Wood
2018-10-11 16:57     ` Alban Gruin
2018-10-12  9:54       ` Phillip Wood
2018-10-12 12:23         ` Alban Gruin
2018-10-07 19:54 ` [PATCH 05/15] sequencer: refactor rearrange_squash() " Alban Gruin
2018-10-07 19:54 ` [PATCH 06/15] sequencer: refactor transform_todos() " Alban Gruin
2018-10-07 19:54 ` [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf Alban Gruin
2018-10-12 10:01   ` SZEDER Gábor
2018-10-19  8:16     ` Junio C Hamano
2018-10-19  9:27       ` SZEDER Gábor
2018-10-07 19:54 ` [PATCH 08/15] sequencer: change complete_action() to use the refactored functions Alban Gruin
2018-10-11 13:51   ` Phillip Wood [this message]
2018-10-11 17:06     ` Alban Gruin
2018-10-07 19:54 ` [PATCH 09/15] sequencer: refactor skip_unnecessary_picks() to work on a todo_list Alban Gruin
2018-10-07 19:54 ` [PATCH 10/15] rebase-interactive: use todo_list_transform() in edit_todo_list() Alban Gruin
2018-10-11 15:16   ` Phillip Wood
2018-10-11 19:58     ` Alban Gruin
2018-10-07 19:54 ` [PATCH 11/15] rebase-interactive: append_todo_help() changes Alban Gruin
2018-10-07 19:54 ` [PATCH 12/15] rebase-interactive: rewrite edit_todo_list() to handle the initial edit Alban Gruin
2018-10-07 19:54 ` [PATCH 13/15] sequencer: use edit_todo_list() in complete_action() Alban Gruin
2018-10-07 19:54 ` [PATCH 14/15] sequencer: fix a call to error() in transform_todo_file() Alban Gruin
2018-10-07 19:54 ` [PATCH 15/15] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
2018-10-07 20:51 ` [PATCH 00/15] sequencer: refactor functions working on a todo_list Alban Gruin
2018-10-27 21:29 ` [PATCH v2 00/16] " Alban Gruin
2018-10-27 21:29   ` [PATCH v2 01/16] sequencer: changes in parse_insn_buffer() Alban Gruin
2018-10-27 21:29   ` [PATCH v2 02/16] sequencer: make the todo_list structure public Alban Gruin
2018-10-27 21:29   ` [PATCH v2 03/16] sequencer: refactor transform_todos() to work on a todo_list Alban Gruin
2018-10-27 21:29   ` [PATCH v2 04/16] sequencer: introduce todo_list_write_to_file() Alban Gruin
2018-10-30 16:28     ` Phillip Wood
2018-11-01 23:31       ` Alban Gruin
2018-10-27 21:29   ` [PATCH v2 05/16] sequencer: refactor check_todo_list() to work on a todo_list Alban Gruin
2018-10-27 21:29   ` [PATCH v2 06/16] sequencer: refactor sequencer_add_exec_commands() " Alban Gruin
2018-10-30 16:47     ` Phillip Wood
2018-11-01 23:31       ` Alban Gruin
2018-11-02 10:09         ` Phillip Wood
2018-11-02 16:26           ` Alban Gruin
2018-11-02 17:09             ` Phillip Wood
2018-10-27 21:29   ` [PATCH v2 07/16] sequencer: refactor rearrange_squash() " Alban Gruin
2018-10-27 21:29   ` [PATCH v2 08/16] sequencer: make sequencer_make_script() write its script to a strbuf Alban Gruin
2018-10-27 21:29   ` [PATCH v2 09/16] sequencer: change complete_action() to use the refactored functions Alban Gruin
2018-10-27 21:29   ` [PATCH v2 10/16] sequencer: refactor skip_unnecessary_picks() to work on a todo_list Alban Gruin
2018-10-27 21:29   ` [PATCH v2 11/16] rebase-interactive: use todo_list_write_to_file() in edit_todo_list() Alban Gruin
2018-10-27 21:29   ` [PATCH v2 12/16] rebase-interactive: append_todo_help() changes Alban Gruin
2018-10-27 21:29   ` [PATCH v2 13/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit Alban Gruin
2018-10-27 21:29   ` [PATCH v2 14/16] sequencer: use edit_todo_list() in complete_action() Alban Gruin
2018-10-27 21:29   ` [PATCH v2 15/16] sequencer: fix a call to error() in transform_todo_file() Alban Gruin
2018-10-27 21:29   ` [PATCH v2 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin
2018-10-29  3:05   ` [PATCH v2 00/16] sequencer: refactor functions working on a todo_list Junio C Hamano
2018-10-29 15:34     ` Alban Gruin
2018-11-09  8:07   ` [PATCH v3 " Alban Gruin
2018-11-09  8:07     ` [PATCH v3 01/16] sequencer: changes in parse_insn_buffer() Alban Gruin
2018-11-09  8:07     ` [PATCH v3 02/16] sequencer: make the todo_list structure public Alban Gruin
2018-11-09  8:07     ` [PATCH v3 03/16] sequencer: refactor transform_todos() to work on a todo_list Alban Gruin
2018-11-09  8:07     ` [PATCH v3 04/16] sequencer: introduce todo_list_write_to_file() Alban Gruin
2018-11-09  8:07     ` [PATCH v3 05/16] sequencer: refactor check_todo_list() to work on a todo_list Alban Gruin
2018-11-09  8:07     ` [PATCH v3 06/16] sequencer: refactor sequencer_add_exec_commands() " Alban Gruin
2018-11-09  8:07     ` [PATCH v3 07/16] sequencer: refactor rearrange_squash() " Alban Gruin
2018-11-09  8:07     ` [PATCH v3 08/16] sequencer: make sequencer_make_script() write its script to a strbuf Alban Gruin
2018-11-09  8:07     ` [PATCH v3 09/16] sequencer: change complete_action() to use the refactored functions Alban Gruin
2018-11-09  8:07     ` [PATCH v3 10/16] sequencer: refactor skip_unnecessary_picks() to work on a todo_list Alban Gruin
2018-11-09  8:08     ` [PATCH v3 11/16] rebase-interactive: use todo_list_write_to_file() in edit_todo_list() Alban Gruin
2018-11-09  8:08     ` [PATCH v3 12/16] rebase-interactive: append_todo_help() changes Alban Gruin
2018-11-09  8:08     ` [PATCH v3 13/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit Alban Gruin
2018-11-09  8:08     ` [PATCH v3 14/16] sequencer: use edit_todo_list() in complete_action() Alban Gruin
2018-11-09  8:08     ` [PATCH v3 15/16] sequencer: fix a call to error() in transform_todo_file() Alban Gruin
2018-11-09  8:08     ` [PATCH v3 16/16] rebase--interactive: move transform_todo_file() to rebase--interactive.c Alban Gruin

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d0adf861-3046-f0b6-3217-c89d92319e43@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=alban.gruin@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox