From: Phillip Wood <phillip.wood123@gmail.com>
To: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>, git@vger.kernel.org
Subject: Re: [PATCH 3/8] sequencer: pass around rebase action explicitly
Date: Thu, 23 Mar 2023 19:27:25 +0000 [thread overview]
Message-ID: <fa584725-52a5-ab7f-3f7b-2fc70fa8fbe1@dunelm.org.uk> (raw)
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:
> ... 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);
>
> /*
next prev parent reply other threads:[~2023-03-23 19:27 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-03-23 19:40 ` Phillip Wood
2023-03-23 20:00 ` Junio C Hamano
2023-03-23 21:08 ` Felipe Contreras
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-08-15 14:01 ` Phillip Wood
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-10-20 21:51 ` Junio C Hamano
2023-10-20 9:36 ` [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
2023-10-23 15:43 ` Phillip Wood
2023-10-23 19:02 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-03-23 19:39 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood [this message]
2023-03-23 21:27 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
2023-03-23 19:31 ` Phillip Wood
2023-03-23 22:38 ` Oswald Buddenhagen
2023-03-24 14:15 ` Phillip Wood
2023-03-24 14:42 ` Oswald Buddenhagen
2023-03-23 20:16 ` Junio C Hamano
2023-03-23 23:23 ` Oswald Buddenhagen
2023-03-24 4:31 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
2023-03-23 19:46 ` Phillip Wood
2023-03-23 22:13 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
2023-03-23 19:34 ` Phillip Wood
2023-03-23 21:36 ` Oswald Buddenhagen
2023-03-24 14:18 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Oswald Buddenhagen
2023-03-26 14:28 ` Phillip Wood
2023-04-26 15:34 ` Oswald Buddenhagen
2023-05-17 12:13 ` Phillip Wood
2023-08-24 16:46 ` Oswald Buddenhagen
2023-03-23 19:38 ` [PATCH 0/8] sequencer refactoring Phillip Wood
2023-03-25 11:08 ` Phillip Wood
2023-04-06 12:09 ` Phillip Wood
2023-05-17 13:10 ` Phillip Wood
Reply instructions:
You may reply publicly 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa584725-52a5-ab7f-3f7b-2fc70fa8fbe1@dunelm.org.uk \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=oswald.buddenhagen@gmx.de \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).