Re: [PATCH v7 14/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit

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

Hi Alban

I think the error handling is fine now, I've got a couple of small
comments below.

On 10/02/2019 13:26, Alban Gruin wrote:
> edit_todo_list() is changed to work on a todo_list, and to handle the
> initial edition of the todo list (ie. making a backup of the todo
> list).
> 
> It does not check for dropped commits yet, as todo_list_check() does not
> take the commits that have already been processed by the rebase (ie. the
> todo list is edited in the middle of a rebase session).
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  builtin/rebase--interactive.c | 24 +++++++++++++++++-
>  rebase-interactive.c          | 48 +++++++++++++++++------------------
>  rebase-interactive.h          |  4 ++-
>  sequencer.c                   |  3 +--
>  sequencer.h                   |  1 +
>  5 files changed, 52 insertions(+), 28 deletions(-)
> 
> diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> index 4f2949922f..370d584683 100644
> --- a/builtin/rebase--interactive.c
> +++ b/builtin/rebase--interactive.c
> @@ -13,6 +13,28 @@ static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")
>  static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
>  static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
>  
> +static int edit_todo_file(unsigned flags)
> +{
> +	const char *todo_file = rebase_path_todo();
> +	struct todo_list todo_list = TODO_LIST_INIT,
> +		new_todo = TODO_LIST_INIT;
> +	int res = 0;
> +
> +	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
> +		return error_errno(_("could not read '%s'."), todo_file);
> +
> +	strbuf_stripspace(&todo_list.buf, 1);
> +	res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);
> +	if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
> +					    NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
> +		res = error_errno(_("could not write '%s'"), todo_file);
> +
> +	todo_list_release(&todo_list);
> +	todo_list_release(&new_todo);
> +
> +	return res;
> +}
> +
>  static int get_revision_ranges(const char *upstream, const char *onto,
>  			       const char **head_hash,
>  			       char **revisions, char **shortrevisions)
> @@ -241,7 +263,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>  		break;
>  	}
>  	case EDIT_TODO:
> -		ret = edit_todo_list(the_repository, flags);
> +		ret = edit_todo_file(flags);
>  		break;
>  	case SHOW_CURRENT_PATCH: {
>  		struct child_process cmd = CHILD_PROCESS_INIT;
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 807f8370db..96c70d1959 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -87,35 +87,35 @@ void append_todo_help(unsigned keep_empty, int command_count,
>  	}
>  }
>  
> -int edit_todo_list(struct repository *r, unsigned flags)
> +int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> +		   struct todo_list *new_todo, const char *shortrevisions,
> +		   const char *shortonto, unsigned flags)
>  {
>  	const char *todo_file = rebase_path_todo();
> -	struct todo_list todo_list = TODO_LIST_INIT;
> -	int res = 0;
> -
> -	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
> -		return error_errno(_("could not read '%s'."), todo_file);
> -
> -	strbuf_stripspace(&todo_list.buf, 1);
> -	todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list);
> -	if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1,
> -				    flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) {
> -		todo_list_release(&todo_list);
> -		return -1;
> -	}
> +	unsigned initial = shortrevisions && shortonto;
>  
> -	strbuf_reset(&todo_list.buf);
> -	if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) {
> -		todo_list_release(&todo_list);
> -		return -1;
> -	}
> +	if (!initial)
> +		todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);

It might be worth a comment to explain that we always want to edit the
file so we deliberately ignore any parsing errors (I'm not sure what we
end writing to the disc if we can't parse the file though) and that the
todo list is already parsed in the initial case.

>  
> -	if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list))
> -		res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1,
> -					      flags & ~(TODO_LIST_SHORTEN_IDS));
> +	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
> +				    -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
> +		return error_errno(_("could not write '%s'"), todo_file);
>  
> -	todo_list_release(&todo_list);
> -	return res;
> +	if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))
> +		return error(_("could not copy '%s' to '%s'."), todo_file,
> +			     rebase_path_todo_backup());
> +
> +	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
> +		return -2;
> +
> +	strbuf_stripspace(&new_todo->buf, 1);
> +	if (initial && new_todo->buf.len == 0)
> +		return -3;
> +
> +	if (!initial)
> +		return todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo);

It might be worth a comment explaining where the list gets parsed for
the initial edit.

Best Wishes

Phillip

> +
> +	return 0;
>  }
>  
>  define_commit_slab(commit_seen, unsigned char);
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 0e5925e3aa..44dbb06311 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -8,7 +8,9 @@ struct todo_list;
>  void append_todo_help(unsigned keep_empty, int command_count,
>  		      const char *shortrevisions, const char *shortonto,
>  		      struct strbuf *buf);
> -int edit_todo_list(struct repository *r, unsigned flags);
> +int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> +		   struct todo_list *new_todo, const char *shortrevisions,
> +		   const char *shortonto, unsigned flags);
>  int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
>  
>  #endif
> diff --git a/sequencer.c b/sequencer.c
> index 64d698032c..f6ed4e21e9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -55,8 +55,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
>   * file and written to the tail of 'done'.
>   */
>  GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
> -static GIT_PATH_FUNC(rebase_path_todo_backup,
> -		     "rebase-merge/git-rebase-todo.backup")
> +GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup")
>  
>  /*
>   * The rebase command lines that have already been processed. A line
> diff --git a/sequencer.h b/sequencer.h
> index c80990659c..b0688ba2a1 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -10,6 +10,7 @@ struct repository;
>  const char *git_path_commit_editmsg(void);
>  const char *git_path_seq_dir(void);
>  const char *rebase_path_todo(void);
> +const char *rebase_path_todo_backup(void);
>  
>  #define APPEND_SIGNOFF_DEDUP (1u << 0)
>  
>