git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/8] sequencer: pass around rebase action explicitly

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Mar 23, 2023, 19:27 UTC
Message-ID
<fa584725-52a5-ab7f-3f7b-2fc70fa8fbe1@dunelm.org.uk>
In-Reply-To
<20230323162235.995574-4-oswald.buddenhagen@gmx.de>
Hi Oswald

I appreciate the sentiment behind this patch, but it looks like an awful lot of churn just to clean up a couple of lines in append_todo_help() and edit_todo_list(). I suspect you may want this change for your --rewind patch, if so I think it would be better to post it in that series where we can better judge the benefit.

Best Wishes
Phillip
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
Show 279 quoted lines
> ... instead of deriving it from other arguments. This is a lot cleaner
> and more extensible.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   builtin/rebase.c     | 25 ++++++++++---------------
>   rebase-interactive.c | 21 +++++++++++----------
>   rebase-interactive.h | 16 ++++++++++++++--
>   sequencer.c          | 16 +++++++++-------
>   sequencer.h          |  8 +++++---
>   5 files changed, 49 insertions(+), 37 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 491759db19..a309addd50 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -58,16 +58,6 @@ enum empty_type {
>   	EMPTY_ASK
>   };
>   
> -enum action {
> -	ACTION_NONE = 0,
> -	ACTION_CONTINUE,
> -	ACTION_SKIP,
> -	ACTION_ABORT,
> -	ACTION_QUIT,
> -	ACTION_EDIT_TODO,
> -	ACTION_SHOW_CURRENT_PATCH
> -};
> -
>   static const char *action_names[] = {
>   	"undefined",
>   	"continue",
> @@ -104,7 +94,7 @@ struct rebase_options {
>   		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
>   	} flags;
>   	struct strvec git_am_opts;
> -	enum action action;
> +	enum rebase_action action;
>   	char *reflog_action;
>   	int signoff;
>   	int allow_rerere_autoupdate;
> @@ -198,9 +188,11 @@ static int edit_todo_file(unsigned flags)
>   		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);
> +	res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags,
> +			     ACTION_EDIT_TODO);
>   	if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
> -					    NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
> +					    NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS),
> +					    ACTION_EDIT_TODO))
>   		res = error_errno(_("could not write '%s'"), todo_file);
>   
>   	todo_list_release(&todo_list);
> @@ -294,7 +286,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   		ret = complete_action(the_repository, &replay, flags,
>   			shortrevisions, opts->onto_name, opts->onto,
>   			&opts->orig_head->object.oid, &opts->exec,
> -			opts->autosquash, opts->update_refs, &todo_list);
> +			opts->autosquash, opts->update_refs, &todo_list,
> +			opts->action);
>   	}
>   
>   cleanup:
> @@ -1246,7 +1239,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		else if (options.exec.nr)
>   			trace2_cmd_mode("interactive-exec");
>   		else
> -			trace2_cmd_mode(action_names[options.action]);
> +			trace2_cmd_mode(
> +				(BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),
> +				 action_names[options.action]));
>   	}
>   
>   	options.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 7407c59319..111a2071ae 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -35,7 +35,7 @@ static enum missing_commit_check_level get_missing_commit_check_level(void)
>   	return MISSING_COMMIT_CHECK_IGNORE;
>   }
>   
> -void append_todo_help(int command_count,
> +void append_todo_help(int command_count, enum rebase_action action,
>   		      const char *shortrevisions, const char *shortonto,
>   		      struct strbuf *buf)
>   {
> @@ -62,9 +62,8 @@ void append_todo_help(int command_count,
>   "                      updated at the end of the rebase\n"
>   "\n"
>   "These lines can be re-ordered; they are executed from top to bottom.\n");
> -	unsigned edit_todo = !(shortrevisions && shortonto);
>   
> -	if (!edit_todo) {
> +	if (action == ACTION_NONE) {
>   		strbuf_addch(buf, '\n');
>   		strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>   					      "Rebase %s onto %s (%d commands)",
> @@ -83,7 +82,7 @@ void append_todo_help(int command_count,
>   
>   	strbuf_add_commented_lines(buf, msg, strlen(msg));
>   
> -	if (edit_todo)
> +	if (action == ACTION_EDIT_TODO)
>   		msg = _("\nYou are editing the todo file "
>   			"of an ongoing interactive rebase.\n"
>   			"To continue rebase after editing, run:\n"
> @@ -97,35 +96,37 @@ void append_todo_help(int command_count,
>   
>   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 *shortonto, unsigned flags,
> +		   enum rebase_action action)
>   {
>   	const char *todo_file = rebase_path_todo(),
>   		*todo_backup = rebase_path_todo_backup();
> -	unsigned initial = shortrevisions && shortonto;
>   	int incorrect = 0;
>   
>   	/* If the user is editing the todo list, we first try to parse
>   	 * it.  If there is an error, we do not return, because the user
>   	 * might want to fix it in the first place. */
> -	if (!initial)
> +	if (action != ACTION_NONE)
>   		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
>   			file_exists(rebase_path_dropped());
>   
>   	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
> -				    -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
> +				    -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP,
> +				    action))
>   		return error_errno(_("could not write '%s'"), todo_file);
>   
>   	if (!incorrect &&
>   	    todo_list_write_to_file(r, todo_list, todo_backup,
>   				    shortrevisions, shortonto, -1,
> -				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
> +				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS,
> +				    action) < 0)
>   		return error(_("could not write '%s'."), 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)
> +	if (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)
>   		return -3;
>   
>   	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 7239c60f79..d9873d3497 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -5,12 +5,24 @@ struct strbuf;
>   struct repository;
>   struct todo_list;
>   
> -void append_todo_help(int command_count,
> +enum rebase_action {
> +	ACTION_NONE = 0,
> +	ACTION_CONTINUE,
> +	ACTION_SKIP,
> +	ACTION_ABORT,
> +	ACTION_QUIT,
> +	ACTION_EDIT_TODO,
> +	ACTION_SHOW_CURRENT_PATCH,
> +	ACTION_LAST
> +};
> +
> +void append_todo_help(int command_count, enum rebase_action action,
>   		      const char *shortrevisions, const char *shortonto,
>   		      struct strbuf *buf);
>   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 *shortonto, unsigned flags,
> +		   enum rebase_action action);
>   
>   int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
>   int todo_list_check_against_backup(struct repository *r,
> diff --git a/sequencer.c b/sequencer.c
> index 7c275c9a65..f05174d151 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5894,14 +5894,15 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>   
>   int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>   			    const char *file, const char *shortrevisions,
> -			    const char *shortonto, int num, unsigned flags)
> +			    const char *shortonto, int num, unsigned flags,
> +			    enum rebase_action action)
>   {
>   	int res;
>   	struct strbuf buf = STRBUF_INIT;
>   
>   	todo_list_to_strbuf(r, todo_list, &buf, num, flags);
>   	if (flags & TODO_LIST_APPEND_TODO_HELP)
> -		append_todo_help(count_commands(todo_list),
> +		append_todo_help(count_commands(todo_list), action,
>   				 shortrevisions, shortonto, &buf);
>   
>   	res = write_message(buf.buf, buf.len, file, 0);
> @@ -5941,7 +5942,8 @@ static int skip_unnecessary_picks(struct repository *r,
>   	if (i > 0) {
>   		const char *done_path = rebase_path_done();
>   
> -		if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) {
> +		if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0,
> +					    ACTION_NONE)) {
>   			error_errno(_("could not write to '%s'"), done_path);
>   			return -1;
>   		}
> @@ -6086,8 +6088,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   		    const char *shortrevisions, const char *onto_name,
>   		    struct commit *onto, const struct object_id *orig_head,
>   		    struct string_list *commands, unsigned autosquash,
> -		    unsigned update_refs,
> -		    struct todo_list *todo_list)
> +		    unsigned update_refs, struct todo_list *todo_list,
> +		    enum rebase_action action)
>   {
>   	char shortonto[GIT_MAX_HEXSZ + 1];
>   	const char *todo_file = rebase_path_todo();
> @@ -6122,7 +6124,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   	}
>   
>   	res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> -			     shortonto, flags);
> +			     shortonto, flags, action);
>   	if (res == -1)
>   		return -1;
>   	else if (res == -2) {
> @@ -6158,7 +6160,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   	}
>   
>   	if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
> -				    flags & ~(TODO_LIST_SHORTEN_IDS))) {
> +				    flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
>   		todo_list_release(&new_todo);
>   		return error_errno(_("could not write '%s'"), todo_file);
>   	}
> diff --git a/sequencer.h b/sequencer.h
> index 33dbaf5b66..1a3e616af2 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -4,6 +4,7 @@
>   #include "strbuf.h"
>   #include "wt-status.h"
>   
> +enum rebase_action;
>   struct commit;
>   struct index_state;
>   struct repository;
> @@ -134,7 +135,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>   				struct todo_list *todo_list);
>   int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>   			    const char *file, const char *shortrevisions,
> -			    const char *shortonto, int num, unsigned flags);
> +			    const char *shortonto, int num, unsigned flags,
> +			    enum rebase_action action);
>   void todo_list_release(struct todo_list *todo_list);
>   const char *todo_item_get_arg(struct todo_list *todo_list,
>   			      struct todo_item *item);
> @@ -187,8 +189,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   		    const char *shortrevisions, const char *onto_name,
>   		    struct commit *onto, const struct object_id *orig_head,
>   		    struct string_list *commands, unsigned autosquash,
> -		    unsigned update_refs,
> -		    struct todo_list *todo_list);
> +		    unsigned update_refs, struct todo_list *todo_list,
> +		    enum rebase_action action);
>   int todo_list_rearrange_squash(struct todo_list *todo_list);
>   
>   /*
Previous: Oswald BuddenhagenNext: Oswald Buddenhagen
Message 37 of 49 in “sequencer refactoring”
  1. 0/8 sequencer refactoringOswald Buddenhagen, Mar 23, 2023
  2. 8/8 rebase: improve resumption from incorrect initial todo listOswald Buddenhagen, Mar 23, 2023
  3. Phillip WoodMar 26, 2023
  4. Oswald BuddenhagenApr 26, 2023
  5. Phillip WoodMay 17, 2023
  6. Oswald BuddenhagenAug 24, 2023
  7. 1/8 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Mar 23, 2023
  8. Phillip WoodMar 23, 2023
  9. Junio C HamanoMar 23, 2023
  10. Felipe ContrerasMar 23, 2023
  11. 0/3 rebase refactoringOswald Buddenhagen, Aug 9, 2023
  12. 1/3 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Aug 9, 2023
  13. 2/3 rebase: handle --strategy via imply_merge() as wellOswald Buddenhagen, Aug 9, 2023
  14. 3/3 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Aug 9, 2023
  15. Phillip WoodAug 15, 2023
  16. 0/3 rebase refactoringOswald Buddenhagen, Oct 20, 2023
  17. 2/3 rebase: handle --strategy via imply_merge() as wellOswald Buddenhagen, Oct 20, 2023
  18. Junio C HamanoOct 20, 2023
  19. 3/3 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Oct 20, 2023
  20. 1/3 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Oct 20, 2023
  21. Junio C HamanoOct 20, 2023
  22. Phillip WoodOct 23, 2023
  23. Junio C HamanoOct 23, 2023
  24. 2/8 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Mar 23, 2023
  25. Phillip WoodMar 23, 2023
  26. 4/8 sequencer: create enum for edit_todo_list() return valueOswald Buddenhagen, Mar 23, 2023
  27. Phillip WoodMar 23, 2023
  28. 5/8 rebase: preserve interactive todo file on checkout failureOswald Buddenhagen, Mar 23, 2023
  29. Phillip WoodMar 23, 2023
  30. Oswald BuddenhagenMar 23, 2023
  31. Phillip WoodMar 24, 2023
  32. Oswald BuddenhagenMar 24, 2023
  33. Junio C HamanoMar 23, 2023
  34. Oswald BuddenhagenMar 23, 2023
  35. Junio C HamanoMar 24, 2023
  36. 3/8 sequencer: pass around rebase action explicitlyOswald Buddenhagen, Mar 23, 2023
  37. Phillip WoodMar 23, 2023
  38. Oswald BuddenhagenMar 23, 2023
  39. 6/8 sequencer: simplify allocation of result array in todo_list_rearrange_squash()Oswald Buddenhagen, Mar 23, 2023
  40. Phillip WoodMar 23, 2023
  41. Oswald BuddenhagenMar 23, 2023
  42. 7/8 sequencer: pass `onto` to complete_action() as object-idOswald Buddenhagen, Mar 23, 2023
  43. Phillip WoodMar 23, 2023
  44. Oswald BuddenhagenMar 23, 2023
  45. Phillip WoodMar 24, 2023
  46. Phillip WoodMar 23, 2023
  47. Phillip WoodMar 25, 2023
  48. Phillip WoodApr 6, 2023
  49. Phillip WoodMay 17, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.