diff mbox series

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

Message ID 20190210132648.12821-15-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 Feb. 10, 2019, 1:26 p.m. UTC 
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(-)

Comments

Phillip Wood Feb. 13, 2019, 10:10 a.m. UTC | #1 
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)
>  
>
diff mbox series

Patch

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);
 
-	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);
+
+	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)