threads / rfc / 50780

RFC patch, 11 partsrebase -i run without forking rebase--interactive

Subject: [RFC PATCH 00/11] rebase -i run without forking rebase--interactive

## tl;dr

40 messages between Mar 19, 2019 and Apr 25, 2019. Diffs are folded; open one to read it.

replies: 39people: 6as markdown or json

Phillip Wood· Mar 19, 2019, 19:03 UTC · lore
From: Phillip Wood <phillip.wood@dunelm.org.uk>

When the builtin rebase starts an interactive rebase it parses the options and then repackages them and forks `rebase--interactive`. This series refactors rebase--interactive so that interactive rebases can be started by the builtin rebase without forking. My motivation was to make it easier to debug the sequencer but this should help future maintainability.

This series involves some code movement so viewing the diffs with --color-moved is recommended.

These patches are based on a merge of master [e902e9bcae ("The second batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841 ("rebase--interactive: move transform_todo_file()", 2019-03-05). They can be fetched from the tag rebase-i-no-fork/rfc at https://github.com/phillipwood/git.git

Phillip Wood (11):
  sequencer: always discard index after checkout
  rebase: rename write_basic_state()
  rebase: use OPT_RERERE_AUTOUPDATE()
  rebase -i: combine rebase--interactive.c with rebase.c
  rebase -i: remove duplication
  rebase -i: use struct commit when parsing options
  rebase -i: use struct object_id for squash_onto
  rebase -i: use struct rebase_options to parse args
  rebase -i: use struct rebase_options in do_interactive_rebase()
  rebase: use a common action enum
  rebase -i: run without forking rebase--interactive
 Makefile                      |   1 -
 builtin/rebase--interactive.c | 377 --------------------
 builtin/rebase.c              | 625 ++++++++++++++++++++++++++--------
 parse-options-cb.c            |  34 ++
 parse-options.h               |   4 +
 sequencer.c                   |  42 ++-
 sequencer.h                   |   7 +-
 7 files changed, 556 insertions(+), 534 deletions(-)
 delete mode 100644 builtin/rebase--interactive.c
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 02/11] rebase: rename write_basic_state()

From: Phillip Wood <phillip.wood@dunelm.org.uk>
This clashes with a function in sequencer.c
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/rebase.c +2 −2
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 52114cbf0d..b634879c5b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -245,7 +245,7 @@ static int read_basic_state(struct rebase_options *opts)
 	return 0;
 }
 
-static int write_basic_state(struct rebase_options *opts)
+static int rebase_write_basic_state(struct rebase_options *opts)
 {
 	write_file(state_dir_path("head-name", opts), "%s",
 		   opts->head_name ? opts->head_name : "detached HEAD");
@@ -640,7 +640,7 @@ static int run_am(struct rebase_options *opts)
 	}
 
 	if (is_directory(opts->state_dir))
-		write_basic_state(opts);
+		rebase_write_basic_state(opts);
 
 	return status;
 }
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 01/11] sequencer: always discard index after checkout

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As the checkout runs in a separate process our index will be out of date so it should be discarded. The existing callers are not doing this consistently so do it here to avoid the callers having to worry about it.

This fixes some test failures that happen if do_interactive_rebase() is called without forking rebase--interactive which we will implement shortly. Running

  git rebase -i master topic

starting on master created empty todo lists because all the commits in topic were marked as cherry-picks. After topic was checked out in prepare_branch_to_be_rebased() the working tree contained the contents from topic but the index contained master and the cache entries were still valid. This meant that diff_populate_filespec() which loads the blobs when calculating patch-id's ended up reading the contents for master from the working tree which actually contained topic.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
Notes:
    It would perhaps be better to pass around the_index rather than
    the_repository
 builtin/rebase--interactive.c |  2 +-
 sequencer.c                   | 27 +++++++++++++++++----------
 sequencer.h                   |  3 ++-
 3 files changed, 20 insertions(+), 12 deletions(-)
Show changes to 3 files +20 −12

builtin/rebase--interactive.c, sequencer.c, sequencer.h

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 4535523bf5..d1a4ac1b84 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -171,7 +171,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 	struct argv_array make_script_args = ARGV_ARRAY_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
 
-	if (prepare_branch_to_be_rebased(opts, switch_to))
+	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
 		return -1;
 
 	if (get_revision_ranges(upstream, onto, &head_hash,
diff --git a/sequencer.c b/sequencer.c
index 281a8ade19..ccc0160800 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3418,10 +3418,11 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
-static int run_git_checkout(struct replay_opts *opts, const char *commit,
-			    const char *action)
+static int run_git_checkout(struct repository *r, struct replay_opts *opts,
+			    const char *commit, const char *action)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	int ret;
 
 	cmd.git_cmd = 1;
 
@@ -3430,25 +3431,31 @@ static int run_git_checkout(struct replay_opts *opts, const char *commit,
 	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
 
 	if (opts->verbose)
-		return run_command(&cmd);
+		ret = run_command(&cmd);
 	else
-		return run_command_silent_on_success(&cmd);
+		ret = run_command_silent_on_success(&cmd);
+
+	if (!ret)
+		discard_index(r->index);
+
+	return ret;
 }
 
-int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit)
+int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
+				 const char *commit)
 {
 	const char *action;
 
 	if (commit && *commit) {
 		action = reflog_message(opts, "start", "checkout %s", commit);
-		if (run_git_checkout(opts, commit, action))
+		if (run_git_checkout(r, opts, commit, action))
 			return error(_("could not checkout %s"), commit);
 	}
 
 	return 0;
 }
 
-static int checkout_onto(struct replay_opts *opts,
+static int checkout_onto(struct repository *r, struct replay_opts *opts,
 			 const char *onto_name, const char *onto,
 			 const char *orig_head)
 {
@@ -3458,7 +3465,7 @@ static int checkout_onto(struct replay_opts *opts,
 	if (get_oid(orig_head, &oid))
 		return error(_("%s: not a valid OID"), orig_head);
 
-	if (run_git_checkout(opts, onto, action)) {
+	if (run_git_checkout(r, opts, onto, action)) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
 		return error(_("could not detach HEAD"));
@@ -4786,7 +4793,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
 	    todo_list_check(todo_list, &new_todo)) {
 		fprintf(stderr, _(edit_todo_list_advice));
-		checkout_onto(opts, onto_name, onto, orig_head);
+		checkout_onto(r, opts, onto_name, onto, orig_head);
 		todo_list_release(&new_todo);
 
 		return -1;
@@ -4805,7 +4812,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 
 	todo_list_release(&new_todo);
 
-	if (checkout_onto(opts, onto_name, oid_to_hex(&oid), orig_head))
+	if (checkout_onto(r, opts, onto_name, oid_to_hex(&oid), orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
diff --git a/sequencer.h b/sequencer.h
index a515ee4457..6c55aa4200 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -175,7 +175,8 @@ void commit_post_rewrite(struct repository *r,
 			 const struct commit *current_head,
 			 const struct object_id *new_head);
 
-int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit);
+int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
+				 const char *commit);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
-- 
2.21.0
Duy Nguyen· Mar 20, 2019, 01:50 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 01/11] sequencer: always discard index after checkout

On Wed, Mar 20, 2019 at 2:04 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>     It would perhaps be better to pass around the_index rather than
>     the_repository

Not by a large margin. For sequencer.c most operations require more than just the index and passing 'struct repository *' around has been the norm. And as soon as you need to load the index back (not sure if you should do it here btw, after discard_index, since we have the index loaded before) you need 'struct repository' not 'struct index_state'.

>  builtin/rebase--interactive.c |  2 +-
>  sequencer.c                   | 27 +++++++++++++++++----------
>  sequencer.h                   |  3 ++-
>  3 files changed, 20 insertions(+), 12 deletions(-)
-- 
Duy
Phillip Wood· Mar 21, 2019, 14:35 UTC · re: Duy Nguyen · lore

Re: [RFC PATCH 01/11] sequencer: always discard index after checkout

On 20/03/2019 01:50, Duy Nguyen wrote:
Show 10 quoted lines
> On Wed, Mar 20, 2019 at 2:04 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>      It would perhaps be better to pass around the_index rather than
>>      the_repository
> 
> Not by a large margin. For sequencer.c most operations require more
> than just the index and passing 'struct repository *' around has been
> the norm. And as soon as you need to load the index back (not sure if
> you should do it here btw, after discard_index, since we have the
> index loaded before) you need 'struct repository' not 'struct
> index_state'.

Thanks, I think I'll leave it as it is then. After we checkout the new base we reload the index in the loop that picks the commits. For 'rebase <upstream> <branch>' after we checkout <branch> we create the todo-list which involves a revision walk and then checkout the new base. I'm not entirely sure if it needs reloading before we create the todo list but I think it probably not as I don't think rebase--interactive.c loads the index (which would explain why this only becomes an issue when we stop forking rebase--interactive from rebase because then rebase.c has loaded the index to check there are no uncommitted changes)

Best Wishes
Phillip
Show 5 quoted lines
> 
>>   builtin/rebase--interactive.c |  2 +-
>>   sequencer.c                   | 27 +++++++++++++++++----------
>>   sequencer.h                   |  3 ++-
>>   3 files changed, 20 insertions(+), 12 deletions(-)
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 03/11] rebase: use OPT_RERERE_AUTOUPDATE()

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As we have a macro for this it makes sense to use it. Having cmd_rebase() and cmd_rebase__interactive() use the same values for this option will be helpful when we start running interactive rebases without forking rebase--interactive.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)
Show changes to builtin/rebase.c +13 −17
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b634879c5b..ccf178326c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -206,14 +206,13 @@ static int read_basic_state(struct rebase_options *opts)
 			    &buf))
 			return -1;
 		if (!strcmp(buf.buf, "--rerere-autoupdate"))
-			opts->allow_rerere_autoupdate = 1;
+			opts->allow_rerere_autoupdate = RERERE_AUTOUPDATE;
 		else if (!strcmp(buf.buf, "--no-rerere-autoupdate"))
-			opts->allow_rerere_autoupdate = 0;
+			opts->allow_rerere_autoupdate = RERERE_NOAUTOUPDATE;
 		else
 			warning(_("ignoring invalid allow_rerere_autoupdate: "
 				  "'%s'"), buf.buf);
-	} else
-		opts->allow_rerere_autoupdate = -1;
+	}
 
 	if (file_exists(state_dir_path("gpg_sign_opt", opts))) {
 		strbuf_reset(&buf);
@@ -263,10 +262,11 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	if (opts->strategy_opts)
 		write_file(state_dir_path("strategy_opts", opts), "%s",
 			   opts->strategy_opts);
-	if (opts->allow_rerere_autoupdate >= 0)
+	if (opts->allow_rerere_autoupdate > 0)
 		write_file(state_dir_path("allow_rerere_autoupdate", opts),
 			   "-%s-rerere-autoupdate",
-			   opts->allow_rerere_autoupdate ? "" : "-no");
+			   opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?
+				"" : "-no");
 	if (opts->gpg_sign_opt)
 		write_file(state_dir_path("gpg_sign_opt", opts), "%s",
 			   opts->gpg_sign_opt);
@@ -625,9 +625,9 @@ static int run_am(struct rebase_options *opts)
 	argv_array_push(&am.args, "--rebasing");
 	argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 	argv_array_push(&am.args, "--patch-format=mboxrd");
-	if (opts->allow_rerere_autoupdate > 0)
+	if (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)
 		argv_array_push(&am.args, "--rerere-autoupdate");
-	else if (opts->allow_rerere_autoupdate == 0)
+	else if (opts->allow_rerere_autoupdate == RERERE_NOAUTOUPDATE)
 		argv_array_push(&am.args, "--no-rerere-autoupdate");
 	if (opts->gpg_sign_opt)
 		argv_array_push(&am.args, opts->gpg_sign_opt);
@@ -713,9 +713,9 @@ static int run_specific_rebase(struct rebase_options *opts)
 			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
 		if (opts->allow_empty_message)
 			argv_array_push(&child.args, "--allow-empty-message");
-		if (opts->allow_rerere_autoupdate > 0)
+		if (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)
 			argv_array_push(&child.args, "--rerere-autoupdate");
-		else if (opts->allow_rerere_autoupdate == 0)
+		else if (opts->allow_rerere_autoupdate == RERERE_NOAUTOUPDATE)
 			argv_array_push(&child.args, "--no-rerere-autoupdate");
 		if (opts->gpg_sign_opt)
 			argv_array_push(&child.args, opts->gpg_sign_opt);
@@ -764,9 +764,9 @@ static int run_specific_rebase(struct rebase_options *opts)
 	add_var(&script_snippet, "action", opts->action ? opts->action : "");
 	add_var(&script_snippet, "signoff", opts->signoff ? "--signoff" : "");
 	add_var(&script_snippet, "allow_rerere_autoupdate",
-		opts->allow_rerere_autoupdate < 0 ? "" :
 		opts->allow_rerere_autoupdate ?
-		"--rerere-autoupdate" : "--no-rerere-autoupdate");
+			opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?
+			"--rerere-autoupdate" : "--no-rerere-autoupdate" : "");
 	add_var(&script_snippet, "keep_empty", opts->keep_empty ? "yes" : "");
 	add_var(&script_snippet, "autosquash", opts->autosquash ? "t" : "");
 	add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt);
@@ -1007,7 +1007,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		.type = REBASE_UNSPECIFIED,
 		.flags = REBASE_NO_QUIET,
 		.git_am_opts = ARGV_ARRAY_INIT,
-		.allow_rerere_autoupdate  = -1,
 		.allow_empty_message = 1,
 		.git_format_patch_opt = STRBUF_INIT,
 	};
@@ -1102,10 +1101,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('p', "preserve-merges", &options.type,
 			    N_("try to recreate merges instead of ignoring "
 			       "them"), REBASE_PRESERVE_MERGES),
-		OPT_BOOL(0, "rerere-autoupdate",
-			 &options.allow_rerere_autoupdate,
-			 N_("allow rerere to update index with resolved "
-			    "conflict")),
+		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
 		OPT_BOOL('k', "keep-empty", &options.keep_empty,
 			 N_("preserve empty commits during rebase")),
 		OPT_BOOL(0, "autosquash", &options.autosquash,
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 05/11] rebase -i: remove duplication

From: Phillip Wood <phillip.wood@dunelm.org.uk>

path_state_dir() and merge_dir() refer to the same path so remove one of them.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to builtin/rebase.c +2 −3
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 80252d72df..e55bd0348a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -37,7 +37,6 @@ static char const * const builtin_rebase_usage[] = {
 	NULL
 };
 
-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 GIT_PATH_FUNC(apply_dir, "rebase-apply")
@@ -182,8 +181,8 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 {
 	FILE *interactive;
 
-	if (!is_directory(path_state_dir()) && mkdir_in_gitdir(path_state_dir()))
-		return error_errno(_("could not create temporary %s"), path_state_dir());
+	if (!is_directory(merge_dir()) && mkdir_in_gitdir(merge_dir()))
+		return error_errno(_("could not create temporary %s"), merge_dir());
 
 	delete_reflog("REBASE_HEAD");
 
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 06/11] rebase -i: use struct commit when parsing options

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This is in preparation for using `struct rebase_options` when parsing options in cmd_rebase__interactive(). Using a string for onto, restrict_revision and upstream, was a hangover from the scripted version of rebase. The functions that use these variables are updated to take a `struct commit`.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
Notes:
    I'm not sure if parse_opt_commit() should be in parse-options-cb.c or
    not, it depends if it is going to be useful elsewhere.
 builtin/rebase.c   | 42 ++++++++++++++++++++++++------------------
 parse-options-cb.c | 17 +++++++++++++++++
 parse-options.h    |  1 +
 sequencer.c        | 21 +++++++++++----------
 sequencer.h        |  4 ++--
 5 files changed, 55 insertions(+), 30 deletions(-)
Show changes to 5 files +55 −30

builtin/rebase.c, parse-options-cb.c, parse-options.h, sequencer.c, sequencer.h

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e55bd0348a..0584b331bf 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -147,27 +147,28 @@ static int edit_todo_file(unsigned flags)
 	return res;
 }
 
-static int get_revision_ranges(const char *upstream, const char *onto,
+static int get_revision_ranges(struct commit *upstream, struct commit *onto,
 			       const char **head_hash,
 			       char **revisions, char **shortrevisions)
 {
-	const char *base_rev = upstream ? upstream : onto, *shorthead;
+	struct commit *base_rev = upstream ? upstream : onto;
+	const char *shorthead;
 	struct object_id orig_head;
 
 	if (get_oid("HEAD", &orig_head))
 		return error(_("no HEAD?"));
 
 	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
-	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+	*revisions = xstrfmt("%s...%s", oid_to_hex(&base_rev->object.oid),
+						   *head_hash);
 
 	shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);
 
 	if (upstream) {
 		const char *shortrev;
-		struct object_id rev_oid;
 
-		get_oid(base_rev, &rev_oid);
-		shortrev = find_unique_abbrev(&rev_oid, DEFAULT_ABBREV);
+		shortrev = find_unique_abbrev(&base_rev->object.oid,
+					      DEFAULT_ABBREV);
 
 		*shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
 	} else
@@ -177,7 +178,7 @@ static int get_revision_ranges(const char *upstream, const char *onto,
 }
 
 static int init_basic_state(struct replay_opts *opts, const char *head_name,
-			    const char *onto, const char *orig_head)
+			    struct commit *onto, const char *orig_head)
 {
 	FILE *interactive;
 
@@ -195,10 +196,10 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 }
 
 static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
-				 const char *switch_to, const char *upstream,
-				 const char *onto, const char *onto_name,
+				 const char *switch_to, struct commit *upstream,
+				 struct commit *onto, const char *onto_name,
 				 const char *squash_onto, const char *head_name,
-				 const char *restrict_revision, char *raw_strategies,
+				 struct commit *restrict_revision, char *raw_strategies,
 				 struct string_list *commands, unsigned autosquash)
 {
 	int ret;
@@ -229,7 +230,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 
 	argv_array_pushl(&make_script_args, "", revisions, NULL);
 	if (restrict_revision)
-		argv_array_push(&make_script_args, restrict_revision);
+		argv_array_push(&make_script_args,
+				oid_to_hex(&restrict_revision->object.oid));
 
 	ret = sequencer_make_script(the_repository, &todo_list.buf,
 				    make_script_args.argc, make_script_args.argv,
@@ -265,9 +267,10 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
-		*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
+	const char *onto_name = NULL,
+		*squash_onto = NULL, *head_name = NULL,
 		*switch_to = NULL, *cmd = NULL;
+	struct commit *onto = NULL, *upstream = NULL, *restrict_revision = NULL;
 	struct string_list commands = STRING_LIST_INIT_DUP;
 	char *raw_strategies = NULL;
 	enum {
@@ -303,13 +306,16 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			N_("insert exec commands in todo list"), ADD_EXEC),
-		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
-		OPT_STRING(0, "restrict-revision", &restrict_revision,
-			   N_("restrict-revision"), N_("restrict revision")),
+		{ OPTION_CALLBACK, 0, "onto", &onto, N_("onto"), N_("onto"),
+		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
+		{ OPTION_CALLBACK, 0, "restrict-revision", &restrict_revision,
+		  N_("restrict-revision"), N_("restrict revision"),
+		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
 		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
 			   N_("squash onto")),
-		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
-			   N_("the upstream commit")),
+		{ OPTION_CALLBACK, 0, "upstream", &upstream, N_("upstream"),
+		  N_("the upstream commit"), PARSE_OPT_NONEG, parse_opt_commit,
+		  0 },
 		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
 		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
 			N_("GPG-sign commits"),
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 2733393546..2206eb763c 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -96,6 +96,23 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_commit(const struct option *opt, const char *arg, int unset)
+{
+	struct object_id oid;
+	struct commit *commit;
+	struct commit **target = opt->value;
+
+	if (!arg)
+		return -1;
+	if (get_oid(arg, &oid))
+		return error("malformed object name %s", arg);
+	commit = lookup_commit_reference(the_repository, &oid);
+	if (!commit)
+		return error("no such commit %s", arg);
+	*target = commit;
+	return 0;
+}
+
 int parse_opt_object_name(const struct option *opt, const char *arg, int unset)
 {
 	struct object_id oid;
diff --git a/parse-options.h b/parse-options.h
index 7d83e2971d..5a75646618 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -266,6 +266,7 @@ int parse_opt_color_flag_cb(const struct option *, const char *, int);
 int parse_opt_verbosity_cb(const struct option *, const char *, int);
 int parse_opt_object_name(const struct option *, const char *, int);
 int parse_opt_commits(const struct option *, const char *, int);
+int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
diff --git a/sequencer.c b/sequencer.c
index ccc0160800..610b7ece14 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2418,14 +2418,15 @@ static void write_strategy_opts(struct replay_opts *opts)
 }
 
 int write_basic_state(struct replay_opts *opts, const char *head_name,
-		      const char *onto, const char *orig_head)
+		      struct commit *onto, const char *orig_head)
 {
 	const char *quiet = getenv("GIT_QUIET");
 
 	if (head_name)
 		write_file(rebase_path_head_name(), "%s\n", head_name);
 	if (onto)
-		write_file(rebase_path_onto(), "%s\n", onto);
+		write_file(rebase_path_onto(), "%s\n",
+			   oid_to_hex(&onto->object.oid));
 	if (orig_head)
 		write_file(rebase_path_orig_head(), "%s\n", orig_head);
 
@@ -3456,7 +3457,7 @@ int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
 }
 
 static int checkout_onto(struct repository *r, struct replay_opts *opts,
-			 const char *onto_name, const char *onto,
+			 const char *onto_name, const struct object_id *onto,
 			 const char *orig_head)
 {
 	struct object_id oid;
@@ -3465,7 +3466,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
 	if (get_oid(orig_head, &oid))
 		return error(_("%s: not a valid OID"), orig_head);
 
-	if (run_git_checkout(r, opts, onto, action)) {
+	if (run_git_checkout(r, opts, oid_to_hex(onto), action)) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
 		return error(_("could not detach HEAD"));
@@ -4741,16 +4742,16 @@ static int skip_unnecessary_picks(struct repository *r,
 
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
-		    const char *onto, const char *orig_head, struct string_list *commands,
-		    unsigned autosquash, struct todo_list *todo_list)
+		    struct commit *onto, const char *orig_head,
+		    struct string_list *commands, unsigned autosquash,
+		    struct todo_list *todo_list)
 {
 	const char *shortonto, *todo_file = rebase_path_todo();
 	struct todo_list new_todo = TODO_LIST_INIT;
 	struct strbuf *buf = &todo_list->buf;
-	struct object_id oid;
+	struct object_id oid = onto->object.oid;
 	int res;
 
-	get_oid(onto, &oid);
 	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
 
 	if (buf->len == 0) {
@@ -4793,7 +4794,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
 	    todo_list_check(todo_list, &new_todo)) {
 		fprintf(stderr, _(edit_todo_list_advice));
-		checkout_onto(r, opts, onto_name, onto, orig_head);
+		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
 		todo_list_release(&new_todo);
 
 		return -1;
@@ -4812,7 +4813,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 
 	todo_list_release(&new_todo);
 
-	if (checkout_onto(r, opts, onto_name, oid_to_hex(&oid), orig_head))
+	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
diff --git a/sequencer.h b/sequencer.h
index 6c55aa4200..e640ca21f2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -150,7 +150,7 @@ void todo_list_add_exec_commands(struct todo_list *todo_list,
 int check_todo_list_from_file(struct repository *r);
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
-		    const char *onto, const char *orig_head, struct string_list *commands,
+		    struct commit *onto, const char *orig_head, struct string_list *commands,
 		    unsigned autosquash, struct todo_list *todo_list);
 int todo_list_rearrange_squash(struct todo_list *todo_list);
 
@@ -191,4 +191,4 @@ int read_author_script(const char *path, char **name, char **email, char **date,
 
 void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
 int write_basic_state(struct replay_opts *opts, const char *head_name,
-		      const char *onto, const char *orig_head);
+		      struct commit *onto, const char *orig_head);
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 04/11] rebase -i: combine rebase--interactive.c with rebase.c

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In order to run `rebase -i` without forking `rebase--interactive` it will be convenient to have all the code from rebase--interactive.c in rebase.c. This is a straight forward copy of the code from rebase--interactive.c, it will be simplified slightly in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Makefile                      |   1 -
 builtin/rebase--interactive.c | 377 ----------------------------------
 builtin/rebase.c              | 367 +++++++++++++++++++++++++++++++++
 3 files changed, 367 insertions(+), 378 deletions(-)
 delete mode 100644 builtin/rebase--interactive.c
Show changes to 3 files +367 −378

Makefile, builtin/rebase--interactive.c, builtin/rebase.c

diff --git a/Makefile b/Makefile
index 537493822b..c644089244 100644
--- a/Makefile
+++ b/Makefile
@@ -1130,7 +1130,6 @@ BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/range-diff.o
 BUILTIN_OBJS += builtin/read-tree.o
 BUILTIN_OBJS += builtin/rebase.o
-BUILTIN_OBJS += builtin/rebase--interactive.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
deleted file mode 100644
index d1a4ac1b84..0000000000
--- a/builtin/rebase--interactive.c
+++ /dev/null
@@ -1,377 +0,0 @@
-#define USE_THE_INDEX_COMPATIBILITY_MACROS
-#include "builtin.h"
-#include "cache.h"
-#include "config.h"
-#include "parse-options.h"
-#include "sequencer.h"
-#include "rebase-interactive.h"
-#include "argv-array.h"
-#include "refs.h"
-#include "rerere.h"
-#include "run-command.h"
-
-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 add_exec_commands(struct string_list *commands)
-{
-	const char *todo_file = rebase_path_todo();
-	struct todo_list todo_list = TODO_LIST_INIT;
-	int res;
-
-	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
-		return error_errno(_("could not read '%s'."), todo_file);
-
-	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-					&todo_list)) {
-		todo_list_release(&todo_list);
-		return error(_("unusable todo list: '%s'"), todo_file);
-	}
-
-	todo_list_add_exec_commands(&todo_list, commands);
-	res = todo_list_write_to_file(the_repository, &todo_list,
-				      todo_file, NULL, NULL, -1, 0);
-	todo_list_release(&todo_list);
-
-	if (res)
-		return error_errno(_("could not write '%s'."), todo_file);
-	return 0;
-}
-
-static int rearrange_squash_in_todo_file(void)
-{
-	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);
-	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-					&todo_list)) {
-		todo_list_release(&todo_list);
-		return error(_("unusable todo list: '%s'"), todo_file);
-	}
-
-	res = todo_list_rearrange_squash(&todo_list);
-	if (!res)
-		res = todo_list_write_to_file(the_repository, &todo_list,
-					      todo_file, NULL, NULL, -1, 0);
-
-	todo_list_release(&todo_list);
-
-	if (res)
-		return error_errno(_("could not write '%s'."), todo_file);
-	return 0;
-}
-
-static int transform_todo_file(unsigned flags)
-{
-	const char *todo_file = rebase_path_todo();
-	struct todo_list todo_list = TODO_LIST_INIT;
-	int res;
-
-	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
-		return error_errno(_("could not read '%s'."), todo_file);
-
-	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-					&todo_list)) {
-		todo_list_release(&todo_list);
-		return error(_("unusable todo list: '%s'"), todo_file);
-	}
-
-	res = todo_list_write_to_file(the_repository, &todo_list, todo_file,
-				      NULL, NULL, -1, flags);
-	todo_list_release(&todo_list);
-
-	if (res)
-		return error_errno(_("could not write '%s'."), todo_file);
-	return 0;
-}
-
-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)
-{
-	const char *base_rev = upstream ? upstream : onto, *shorthead;
-	struct object_id orig_head;
-
-	if (get_oid("HEAD", &orig_head))
-		return error(_("no HEAD?"));
-
-	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
-	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
-
-	shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);
-
-	if (upstream) {
-		const char *shortrev;
-		struct object_id rev_oid;
-
-		get_oid(base_rev, &rev_oid);
-		shortrev = find_unique_abbrev(&rev_oid, DEFAULT_ABBREV);
-
-		*shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
-	} else
-		*shortrevisions = xstrdup(shorthead);
-
-	return 0;
-}
-
-static int init_basic_state(struct replay_opts *opts, const char *head_name,
-			    const char *onto, const char *orig_head)
-{
-	FILE *interactive;
-
-	if (!is_directory(path_state_dir()) && mkdir_in_gitdir(path_state_dir()))
-		return error_errno(_("could not create temporary %s"), path_state_dir());
-
-	delete_reflog("REBASE_HEAD");
-
-	interactive = fopen(path_interactive(), "w");
-	if (!interactive)
-		return error_errno(_("could not mark as interactive"));
-	fclose(interactive);
-
-	return write_basic_state(opts, head_name, onto, orig_head);
-}
-
-static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
-				 const char *switch_to, const char *upstream,
-				 const char *onto, const char *onto_name,
-				 const char *squash_onto, const char *head_name,
-				 const char *restrict_revision, char *raw_strategies,
-				 struct string_list *commands, unsigned autosquash)
-{
-	int ret;
-	const char *head_hash = NULL;
-	char *revisions = NULL, *shortrevisions = NULL;
-	struct argv_array make_script_args = ARGV_ARRAY_INIT;
-	struct todo_list todo_list = TODO_LIST_INIT;
-
-	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
-		return -1;
-
-	if (get_revision_ranges(upstream, onto, &head_hash,
-				&revisions, &shortrevisions))
-		return -1;
-
-	if (raw_strategies)
-		parse_strategy_opts(opts, raw_strategies);
-
-	if (init_basic_state(opts, head_name, onto, head_hash)) {
-		free(revisions);
-		free(shortrevisions);
-
-		return -1;
-	}
-
-	if (!upstream && squash_onto)
-		write_file(path_squash_onto(), "%s\n", squash_onto);
-
-	argv_array_pushl(&make_script_args, "", revisions, NULL);
-	if (restrict_revision)
-		argv_array_push(&make_script_args, restrict_revision);
-
-	ret = sequencer_make_script(the_repository, &todo_list.buf,
-				    make_script_args.argc, make_script_args.argv,
-				    flags);
-
-	if (ret)
-		error(_("could not generate todo list"));
-	else {
-		discard_cache();
-		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-						&todo_list))
-			BUG("unusable todo list");
-
-		ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name,
-				      onto, head_hash, commands, autosquash, &todo_list);
-	}
-
-	free(revisions);
-	free(shortrevisions);
-	todo_list_release(&todo_list);
-	argv_array_clear(&make_script_args);
-
-	return ret;
-}
-
-static const char * const builtin_rebase_interactive_usage[] = {
-	N_("git rebase--interactive [<options>]"),
-	NULL
-};
-
-int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
-{
-	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
-	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
-		*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
-		*switch_to = NULL, *cmd = NULL;
-	struct string_list commands = STRING_LIST_INIT_DUP;
-	char *raw_strategies = NULL;
-	enum {
-		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
-		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
-	} command = 0;
-	struct option options[] = {
-		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
-		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
-		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
-			 N_("allow commits with empty messages")),
-		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
-		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
-			 N_("keep original branch points of cousins")),
-		OPT_BOOL(0, "autosquash", &autosquash,
-			 N_("move commits that begin with squash!/fixup!")),
-		OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")),
-		OPT__VERBOSE(&opts.verbose, N_("be verbose")),
-		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
-			    CONTINUE),
-		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
-		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
-			    EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
-			    SHOW_CURRENT_PATCH),
-		OPT_CMDMODE(0, "shorten-ids", &command,
-			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
-		OPT_CMDMODE(0, "expand-ids", &command,
-			N_("expand commit ids in the todo list"), EXPAND_OIDS),
-		OPT_CMDMODE(0, "check-todo-list", &command,
-			N_("check the todo list"), CHECK_TODO_LIST),
-		OPT_CMDMODE(0, "rearrange-squash", &command,
-			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
-		OPT_CMDMODE(0, "add-exec-commands", &command,
-			N_("insert exec commands in todo list"), ADD_EXEC),
-		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
-		OPT_STRING(0, "restrict-revision", &restrict_revision,
-			   N_("restrict-revision"), N_("restrict revision")),
-		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
-			   N_("squash onto")),
-		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
-			   N_("the upstream commit")),
-		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
-		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
-			N_("GPG-sign commits"),
-			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
-		OPT_STRING(0, "strategy", &opts.strategy, N_("strategy"),
-			   N_("rebase strategy")),
-		OPT_STRING(0, "strategy-opts", &raw_strategies, N_("strategy-opts"),
-			   N_("strategy options")),
-		OPT_STRING(0, "switch-to", &switch_to, N_("switch-to"),
-			   N_("the branch or commit to checkout")),
-		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
-		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
-		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
-		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
-			 N_("automatically re-schedule any `exec` that fails")),
-		OPT_END()
-	};
-
-	sequencer_init_config(&opts);
-	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
-
-	opts.action = REPLAY_INTERACTIVE_REBASE;
-	opts.allow_ff = 1;
-	opts.allow_empty = 1;
-
-	if (argc == 1)
-		usage_with_options(builtin_rebase_interactive_usage, options);
-
-	argc = parse_options(argc, argv, NULL, options,
-			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
-
-	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
-
-	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
-	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
-	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
-	flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
-	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
-
-	if (rebase_cousins >= 0 && !rebase_merges)
-		warning(_("--[no-]rebase-cousins has no effect without "
-			  "--rebase-merges"));
-
-	if (cmd && *cmd) {
-		string_list_split(&commands, cmd, '\n', -1);
-
-		/* rebase.c adds a new line to cmd after every command,
-		 * so here the last command is always empty */
-		string_list_remove_empty_items(&commands, 0);
-	}
-
-	switch (command) {
-	case NONE:
-		if (!onto && !upstream)
-			die(_("a base commit must be provided with --upstream or --onto"));
-
-		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
-					    onto_name, squash_onto, head_name, restrict_revision,
-					    raw_strategies, &commands, autosquash);
-		break;
-	case SKIP: {
-		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-
-		rerere_clear(the_repository, &merge_rr);
-		/* fallthrough */
-	case CONTINUE:
-		ret = sequencer_continue(the_repository, &opts);
-		break;
-	}
-	case EDIT_TODO:
-		ret = edit_todo_file(flags);
-		break;
-	case SHOW_CURRENT_PATCH: {
-		struct child_process cmd = CHILD_PROCESS_INIT;
-
-		cmd.git_cmd = 1;
-		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
-		ret = run_command(&cmd);
-
-		break;
-	}
-	case SHORTEN_OIDS:
-	case EXPAND_OIDS:
-		ret = transform_todo_file(flags);
-		break;
-	case CHECK_TODO_LIST:
-		ret = check_todo_list_from_file(the_repository);
-		break;
-	case REARRANGE_SQUASH:
-		ret = rearrange_squash_in_todo_file();
-		break;
-	case ADD_EXEC:
-		ret = add_exec_commands(&commands);
-		break;
-	default:
-		BUG("invalid command '%d'", command);
-	}
-
-	string_list_clear(&commands, 0);
-	return !!ret;
-}
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ccf178326c..80252d72df 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -25,6 +25,8 @@
 #include "commit-reach.h"
 #include "rerere.h"
 #include "branch.h"
+#include "sequencer.h"
+#include "rebase-interactive.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
@@ -35,6 +37,9 @@ static char const * const builtin_rebase_usage[] = {
 	NULL
 };
 
+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 GIT_PATH_FUNC(apply_dir, "rebase-apply")
 static GIT_PATH_FUNC(merge_dir, "rebase-merge")
 
@@ -46,6 +51,368 @@ enum rebase_type {
 	REBASE_PRESERVE_MERGES
 };
 
+static int add_exec_commands(struct string_list *commands)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	int res;
+
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+		return error_errno(_("could not read '%s'."), todo_file);
+
+	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+					&todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	todo_list_add_exec_commands(&todo_list, commands);
+	res = todo_list_write_to_file(the_repository, &todo_list,
+				      todo_file, NULL, NULL, -1, 0);
+	todo_list_release(&todo_list);
+
+	if (res)
+		return error_errno(_("could not write '%s'."), todo_file);
+	return 0;
+}
+
+static int rearrange_squash_in_todo_file(void)
+{
+	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);
+	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+					&todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	res = todo_list_rearrange_squash(&todo_list);
+	if (!res)
+		res = todo_list_write_to_file(the_repository, &todo_list,
+					      todo_file, NULL, NULL, -1, 0);
+
+	todo_list_release(&todo_list);
+
+	if (res)
+		return error_errno(_("could not write '%s'."), todo_file);
+	return 0;
+}
+
+static int transform_todo_file(unsigned flags)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	int res;
+
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+		return error_errno(_("could not read '%s'."), todo_file);
+
+	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+					&todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	res = todo_list_write_to_file(the_repository, &todo_list, todo_file,
+				      NULL, NULL, -1, flags);
+	todo_list_release(&todo_list);
+
+	if (res)
+		return error_errno(_("could not write '%s'."), todo_file);
+	return 0;
+}
+
+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)
+{
+	const char *base_rev = upstream ? upstream : onto, *shorthead;
+	struct object_id orig_head;
+
+	if (get_oid("HEAD", &orig_head))
+		return error(_("no HEAD?"));
+
+	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
+	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+	shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);
+
+	if (upstream) {
+		const char *shortrev;
+		struct object_id rev_oid;
+
+		get_oid(base_rev, &rev_oid);
+		shortrev = find_unique_abbrev(&rev_oid, DEFAULT_ABBREV);
+
+		*shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
+	} else
+		*shortrevisions = xstrdup(shorthead);
+
+	return 0;
+}
+
+static int init_basic_state(struct replay_opts *opts, const char *head_name,
+			    const char *onto, const char *orig_head)
+{
+	FILE *interactive;
+
+	if (!is_directory(path_state_dir()) && mkdir_in_gitdir(path_state_dir()))
+		return error_errno(_("could not create temporary %s"), path_state_dir());
+
+	delete_reflog("REBASE_HEAD");
+
+	interactive = fopen(path_interactive(), "w");
+	if (!interactive)
+		return error_errno(_("could not mark as interactive"));
+	fclose(interactive);
+
+	return write_basic_state(opts, head_name, onto, orig_head);
+}
+
+static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
+				 const char *switch_to, const char *upstream,
+				 const char *onto, const char *onto_name,
+				 const char *squash_onto, const char *head_name,
+				 const char *restrict_revision, char *raw_strategies,
+				 struct string_list *commands, unsigned autosquash)
+{
+	int ret;
+	const char *head_hash = NULL;
+	char *revisions = NULL, *shortrevisions = NULL;
+	struct argv_array make_script_args = ARGV_ARRAY_INIT;
+	struct todo_list todo_list = TODO_LIST_INIT;
+
+	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
+		return -1;
+
+	if (get_revision_ranges(upstream, onto, &head_hash,
+				&revisions, &shortrevisions))
+		return -1;
+
+	if (raw_strategies)
+		parse_strategy_opts(opts, raw_strategies);
+
+	if (init_basic_state(opts, head_name, onto, head_hash)) {
+		free(revisions);
+		free(shortrevisions);
+
+		return -1;
+	}
+
+	if (!upstream && squash_onto)
+		write_file(path_squash_onto(), "%s\n", squash_onto);
+
+	argv_array_pushl(&make_script_args, "", revisions, NULL);
+	if (restrict_revision)
+		argv_array_push(&make_script_args, restrict_revision);
+
+	ret = sequencer_make_script(the_repository, &todo_list.buf,
+				    make_script_args.argc, make_script_args.argv,
+				    flags);
+
+	if (ret)
+		error(_("could not generate todo list"));
+	else {
+		discard_cache();
+		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+						&todo_list))
+			BUG("unusable todo list");
+
+		ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name,
+				      onto, head_hash, commands, autosquash, &todo_list);
+	}
+
+	free(revisions);
+	free(shortrevisions);
+	todo_list_release(&todo_list);
+	argv_array_clear(&make_script_args);
+
+	return ret;
+}
+
+static const char * const builtin_rebase_interactive_usage[] = {
+	N_("git rebase--interactive [<options>]"),
+	NULL
+};
+
+int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
+{
+	struct replay_opts opts = REPLAY_OPTS_INIT;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
+	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
+	const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
+		*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
+		*switch_to = NULL, *cmd = NULL;
+	struct string_list commands = STRING_LIST_INIT_DUP;
+	char *raw_strategies = NULL;
+	enum {
+		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
+		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
+	} command = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
+		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
+		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
+			 N_("allow commits with empty messages")),
+		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
+		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
+			 N_("keep original branch points of cousins")),
+		OPT_BOOL(0, "autosquash", &autosquash,
+			 N_("move commits that begin with squash!/fixup!")),
+		OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")),
+		OPT__VERBOSE(&opts.verbose, N_("be verbose")),
+		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
+			    CONTINUE),
+		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
+		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
+			    EDIT_TODO),
+		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
+			    SHOW_CURRENT_PATCH),
+		OPT_CMDMODE(0, "shorten-ids", &command,
+			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
+		OPT_CMDMODE(0, "expand-ids", &command,
+			N_("expand commit ids in the todo list"), EXPAND_OIDS),
+		OPT_CMDMODE(0, "check-todo-list", &command,
+			N_("check the todo list"), CHECK_TODO_LIST),
+		OPT_CMDMODE(0, "rearrange-squash", &command,
+			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+		OPT_CMDMODE(0, "add-exec-commands", &command,
+			N_("insert exec commands in todo list"), ADD_EXEC),
+		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
+		OPT_STRING(0, "restrict-revision", &restrict_revision,
+			   N_("restrict-revision"), N_("restrict revision")),
+		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
+			   N_("squash onto")),
+		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
+			   N_("the upstream commit")),
+		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
+		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
+			N_("GPG-sign commits"),
+			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+		OPT_STRING(0, "strategy", &opts.strategy, N_("strategy"),
+			   N_("rebase strategy")),
+		OPT_STRING(0, "strategy-opts", &raw_strategies, N_("strategy-opts"),
+			   N_("strategy options")),
+		OPT_STRING(0, "switch-to", &switch_to, N_("switch-to"),
+			   N_("the branch or commit to checkout")),
+		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
+		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
+		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
+			 N_("automatically re-schedule any `exec` that fails")),
+		OPT_END()
+	};
+
+	sequencer_init_config(&opts);
+	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
+
+	opts.action = REPLAY_INTERACTIVE_REBASE;
+	opts.allow_ff = 1;
+	opts.allow_empty = 1;
+
+	if (argc == 1)
+		usage_with_options(builtin_rebase_interactive_usage, options);
+
+	argc = parse_options(argc, argv, NULL, options,
+			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
+
+	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
+
+	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
+	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
+	flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+
+	if (rebase_cousins >= 0 && !rebase_merges)
+		warning(_("--[no-]rebase-cousins has no effect without "
+			  "--rebase-merges"));
+
+	if (cmd && *cmd) {
+		string_list_split(&commands, cmd, '\n', -1);
+
+		/* rebase.c adds a new line to cmd after every command,
+		 * so here the last command is always empty */
+		string_list_remove_empty_items(&commands, 0);
+	}
+
+	switch (command) {
+	case NONE:
+		if (!onto && !upstream)
+			die(_("a base commit must be provided with --upstream or --onto"));
+
+		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
+					    onto_name, squash_onto, head_name, restrict_revision,
+					    raw_strategies, &commands, autosquash);
+		break;
+	case SKIP: {
+		struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+		rerere_clear(the_repository, &merge_rr);
+		/* fallthrough */
+	case CONTINUE:
+		ret = sequencer_continue(the_repository, &opts);
+		break;
+	}
+	case EDIT_TODO:
+		ret = edit_todo_file(flags);
+		break;
+	case SHOW_CURRENT_PATCH: {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
+		ret = run_command(&cmd);
+
+		break;
+	}
+	case SHORTEN_OIDS:
+	case EXPAND_OIDS:
+		ret = transform_todo_file(flags);
+		break;
+	case CHECK_TODO_LIST:
+		ret = check_todo_list_from_file(the_repository);
+		break;
+	case REARRANGE_SQUASH:
+		ret = rearrange_squash_in_todo_file();
+		break;
+	case ADD_EXEC:
+		ret = add_exec_commands(&commands);
+		break;
+	default:
+		BUG("invalid command '%d'", command);
+	}
+
+	string_list_clear(&commands, 0);
+	return !!ret;
+}
+
 static int use_builtin_rebase(void)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 07/11] rebase -i: use struct object_id for squash_onto

From: Phillip Wood <phillip.wood@dunelm.org.uk>

More preparation for using `struct rebase_options` in cmd_rebase__interactive(). Using a string was a hangover from the scripted version of rebase, update the functions that use `squash_onto` to take a `sturct object_id`.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c   | 21 +++++++++++++--------
 parse-options-cb.c | 17 +++++++++++++++++
 parse-options.h    |  3 +++
 3 files changed, 33 insertions(+), 8 deletions(-)
Show changes to 3 files +33 −8

builtin/rebase.c, parse-options-cb.c, parse-options.h

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0584b331bf..c93f2aa629 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -198,7 +198,7 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 				 const char *switch_to, struct commit *upstream,
 				 struct commit *onto, const char *onto_name,
-				 const char *squash_onto, const char *head_name,
+				 struct object_id *squash_onto, const char *head_name,
 				 struct commit *restrict_revision, char *raw_strategies,
 				 struct string_list *commands, unsigned autosquash)
 {
@@ -226,7 +226,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 	}
 
 	if (!upstream && squash_onto)
-		write_file(path_squash_onto(), "%s\n", squash_onto);
+		write_file(path_squash_onto(), "%s\n",
+			   oid_to_hex(squash_onto));
 
 	argv_array_pushl(&make_script_args, "", revisions, NULL);
 	if (restrict_revision)
@@ -267,10 +268,11 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto_name = NULL,
-		*squash_onto = NULL, *head_name = NULL,
-		*switch_to = NULL, *cmd = NULL;
+	const char *onto_name = NULL, *head_name = NULL, *switch_to = NULL,
+		*cmd = NULL;
 	struct commit *onto = NULL, *upstream = NULL, *restrict_revision = NULL;
+	struct object_id squash_onto = null_oid;
+	struct object_id *squash_onto_opt = NULL;
 	struct string_list commands = STRING_LIST_INIT_DUP;
 	char *raw_strategies = NULL;
 	enum {
@@ -311,8 +313,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "restrict-revision", &restrict_revision,
 		  N_("restrict-revision"), N_("restrict revision"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
-		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
-			   N_("squash onto")),
+		{ OPTION_CALLBACK, 0, "squash-onto", &squash_onto, N_("squash-onto"),
+		  N_("squash onto"), PARSE_OPT_NONEG, parse_opt_object_id, 0 },
 		{ OPTION_CALLBACK, 0, "upstream", &upstream, N_("upstream"),
 		  N_("the upstream commit"), PARSE_OPT_NONEG, parse_opt_commit,
 		  0 },
@@ -349,6 +351,9 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 
 	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
 
+	if (!is_null_oid(&squash_onto))
+		squash_onto_opt = &squash_onto;
+
 	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
@@ -373,7 +378,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
 		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
-					    onto_name, squash_onto, head_name, restrict_revision,
+					    onto_name, squash_onto_opt, head_name, restrict_revision,
 					    raw_strategies, &commands, autosquash);
 		break;
 	case SKIP: {
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 2206eb763c..28ad5cd94b 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -129,6 +129,23 @@ int parse_opt_object_name(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_object_id(const struct option *opt, const char *arg, int unset)
+{
+	struct object_id oid;
+	struct object_id *target = opt->value;
+
+	if (unset) {
+		*target = null_oid;
+		return 0;
+	}
+	if (!arg)
+		return -1;
+	if (get_oid(arg, &oid))
+		return error(_("malformed object name '%s'"), arg);
+	*target = oid;
+	return 0;
+}
+
 int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
 {
 	int *target = opt->value;
diff --git a/parse-options.h b/parse-options.h
index 5a75646618..0ab1103bc7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -264,7 +264,10 @@ int parse_opt_abbrev_cb(const struct option *, const char *, int);
 int parse_opt_expiry_date_cb(const struct option *, const char *, int);
 int parse_opt_color_flag_cb(const struct option *, const char *, int);
 int parse_opt_verbosity_cb(const struct option *, const char *, int);
+/* value is struct oid_array* */
 int parse_opt_object_name(const struct option *, const char *, int);
+/* value is struct object_id* */
+int parse_opt_object_id(const struct option *, const char *, int);
 int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 08/11] rebase -i: use struct rebase_options to parse args

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In order to run `rebase -i` without forking `rebase--interactive` it will be convenient to use the same structure when parsing the options in cmd_rebase() and cmd_rebase__interactive().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 203 ++++++++++++++++++++++++++---------------------
 1 file changed, 112 insertions(+), 91 deletions(-)
Show changes to builtin/rebase.c +112 −91
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c93f2aa629..33a2495032 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -50,6 +50,73 @@ enum rebase_type {
 	REBASE_PRESERVE_MERGES
 };
 
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	const char *upstream_arg;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	const char *switch_to;
+	int root;
+	struct object_id *squash_onto;
+	struct commit *restrict_revision;
+	int dont_finish_rebase;
+	enum {
+		REBASE_NO_QUIET = 1<<0,
+		REBASE_VERBOSE = 1<<1,
+		REBASE_DIFFSTAT = 1<<2,
+		REBASE_FORCE = 1<<3,
+		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
+	} flags;
+	struct argv_array git_am_opts;
+	const char *action;
+	int signoff;
+	int allow_rerere_autoupdate;
+	int keep_empty;
+	int autosquash;
+	char *gpg_sign_opt;
+	int autostash;
+	char *cmd;
+	int allow_empty_message;
+	int rebase_merges, rebase_cousins;
+	char *strategy, *strategy_opts;
+	struct strbuf git_format_patch_opt;
+	int reschedule_failed_exec;
+};
+
+#define REBASE_OPTIONS_INIT {			  	\
+		.type = REBASE_UNSPECIFIED,	  	\
+		.flags = REBASE_NO_QUIET, 		\
+		.git_am_opts = ARGV_ARRAY_INIT,		\
+		.git_format_patch_opt = STRBUF_INIT	\
+	}
+
+static struct replay_opts get_replay_opts(const struct rebase_options *opts)
+{
+	struct replay_opts replay = REPLAY_OPTS_INIT;
+
+	sequencer_init_config(&replay);
+
+	replay.action = REPLAY_INTERACTIVE_REBASE;
+	replay.signoff = opts->signoff;
+	replay.allow_ff = !(opts->flags & REBASE_FORCE);
+	if (opts->allow_rerere_autoupdate)
+		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
+	replay.allow_empty = 1;
+	replay.allow_empty_message = opts->allow_empty_message;
+	replay.verbose = opts->flags & REBASE_VERBOSE;
+	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
+	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
+	replay.strategy = opts->strategy;
+
+	return replay;
+}
+
 static int add_exec_commands(struct string_list *commands)
 {
 	const char *todo_file = rebase_path_todo();
@@ -265,32 +332,30 @@ static const char * const builtin_rebase_interactive_usage[] = {
 
 int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 {
-	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
-	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto_name = NULL, *head_name = NULL, *switch_to = NULL,
-		*cmd = NULL;
-	struct commit *onto = NULL, *upstream = NULL, *restrict_revision = NULL;
+	struct rebase_options opts = REBASE_OPTIONS_INIT;
+	unsigned flags = 0;
+	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
-	struct object_id *squash_onto_opt = NULL;
 	struct string_list commands = STRING_LIST_INIT_DUP;
-	char *raw_strategies = NULL;
 	enum {
 		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
 		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
 	} command = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
-		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
+		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
+			   REBASE_FORCE),
+		OPT_BOOL(0, "keep-empty", &opts.keep_empty, N_("keep empty commits")),
 		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
 			 N_("allow commits with empty messages")),
-		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
-		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
+		OPT_BOOL(0, "rebase-merges", &opts.rebase_merges, N_("rebase merge commits")),
+		OPT_BOOL(0, "rebase-cousins", &opts.rebase_cousins,
 			 N_("keep original branch points of cousins")),
-		OPT_BOOL(0, "autosquash", &autosquash,
+		OPT_BOOL(0, "autosquash", &opts.autosquash,
 			 N_("move commits that begin with squash!/fixup!")),
 		OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")),
-		OPT__VERBOSE(&opts.verbose, N_("be verbose")),
+		OPT_BIT('v', "verbose", &opts.flags,
+			N_("display a diffstat of what changed upstream"),
+			REBASE_NO_QUIET | REBASE_VERBOSE | REBASE_DIFFSTAT),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 			    CONTINUE),
 		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
@@ -308,86 +373,86 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			N_("insert exec commands in todo list"), ADD_EXEC),
-		{ OPTION_CALLBACK, 0, "onto", &onto, N_("onto"), N_("onto"),
+		{ OPTION_CALLBACK, 0, "onto", &opts.onto, N_("onto"), N_("onto"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
-		{ OPTION_CALLBACK, 0, "restrict-revision", &restrict_revision,
+		{ OPTION_CALLBACK, 0, "restrict-revision", &opts.restrict_revision,
 		  N_("restrict-revision"), N_("restrict revision"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
 		{ OPTION_CALLBACK, 0, "squash-onto", &squash_onto, N_("squash-onto"),
 		  N_("squash onto"), PARSE_OPT_NONEG, parse_opt_object_id, 0 },
-		{ OPTION_CALLBACK, 0, "upstream", &upstream, N_("upstream"),
+		{ OPTION_CALLBACK, 0, "upstream", &opts.upstream, N_("upstream"),
 		  N_("the upstream commit"), PARSE_OPT_NONEG, parse_opt_commit,
 		  0 },
-		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
-		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
+		OPT_STRING(0, "head-name", &opts.head_name, N_("head-name"), N_("head name")),
+		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign_opt, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_STRING(0, "strategy", &opts.strategy, N_("strategy"),
 			   N_("rebase strategy")),
-		OPT_STRING(0, "strategy-opts", &raw_strategies, N_("strategy-opts"),
+		OPT_STRING(0, "strategy-opts", &opts.strategy_opts, N_("strategy-opts"),
 			   N_("strategy options")),
-		OPT_STRING(0, "switch-to", &switch_to, N_("switch-to"),
+		OPT_STRING(0, "switch-to", &opts.switch_to, N_("switch-to"),
 			   N_("the branch or commit to checkout")),
-		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
-		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
-		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+		OPT_STRING(0, "onto-name", &opts.onto_name, N_("onto-name"), N_("onto name")),
+		OPT_STRING(0, "cmd", &opts.cmd, N_("cmd"), N_("the command to run")),
+		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_autoupdate),
 		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
 			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END()
 	};
 
-	sequencer_init_config(&opts);
+	opts.rebase_cousins = -1;
+
 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
-	opts.action = REPLAY_INTERACTIVE_REBASE;
-	opts.allow_ff = 1;
-	opts.allow_empty = 1;
-
 	if (argc == 1)
 		usage_with_options(builtin_rebase_interactive_usage, options);
 
 	argc = parse_options(argc, argv, NULL, options,
 			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
 
-	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
-
 	if (!is_null_oid(&squash_onto))
-		squash_onto_opt = &squash_onto;
+		opts.squash_onto = &squash_onto;
 
-	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= opts.keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
-	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
-	flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
+	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
 	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
-	if (rebase_cousins >= 0 && !rebase_merges)
+	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
-	if (cmd && *cmd) {
-		string_list_split(&commands, cmd, '\n', -1);
+	if (opts.cmd && *opts.cmd) {
+		string_list_split(&commands, opts.cmd, '\n', -1);
 
 		/* rebase.c adds a new line to cmd after every command,
 		 * so here the last command is always empty */
 		string_list_remove_empty_items(&commands, 0);
 	}
 
 	switch (command) {
-	case NONE:
-		if (!onto && !upstream)
+	case NONE: {
+		struct replay_opts replay_opts = get_replay_opts(&opts);
+		if (!opts.onto && !opts.upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
-		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
-					    onto_name, squash_onto_opt, head_name, restrict_revision,
-					    raw_strategies, &commands, autosquash);
+		ret = do_interactive_rebase(&replay_opts, flags, opts.switch_to, opts.upstream, opts.onto,
+					    opts.onto_name, opts.squash_onto, opts.head_name, opts.restrict_revision,
+					    opts.strategy_opts, &commands, opts.autosquash);
 		break;
+	}
 	case SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		rerere_clear(the_repository, &merge_rr);
+	}
 		/* fallthrough */
-	case CONTINUE:
-		ret = sequencer_continue(the_repository, &opts);
+	case CONTINUE: {
+		struct replay_opts replay_opts = get_replay_opts(&opts);
+
+		ret = sequencer_continue(the_repository, &replay_opts);
 		break;
 	}
 	case EDIT_TODO:
@@ -446,45 +511,6 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
-struct rebase_options {
-	enum rebase_type type;
-	const char *state_dir;
-	struct commit *upstream;
-	const char *upstream_name;
-	const char *upstream_arg;
-	char *head_name;
-	struct object_id orig_head;
-	struct commit *onto;
-	const char *onto_name;
-	const char *revisions;
-	const char *switch_to;
-	int root;
-	struct object_id *squash_onto;
-	struct commit *restrict_revision;
-	int dont_finish_rebase;
-	enum {
-		REBASE_NO_QUIET = 1<<0,
-		REBASE_VERBOSE = 1<<1,
-		REBASE_DIFFSTAT = 1<<2,
-		REBASE_FORCE = 1<<3,
-		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
-	} flags;
-	struct argv_array git_am_opts;
-	const char *action;
-	int signoff;
-	int allow_rerere_autoupdate;
-	int keep_empty;
-	int autosquash;
-	char *gpg_sign_opt;
-	int autostash;
-	char *cmd;
-	int allow_empty_message;
-	int rebase_merges, rebase_cousins;
-	char *strategy, *strategy_opts;
-	struct strbuf git_format_patch_opt;
-	int reschedule_failed_exec;
-};
-
 static int is_interactive(struct rebase_options *opts)
 {
 	return opts->type == REBASE_INTERACTIVE ||
@@ -1380,13 +1406,7 @@ static int check_exec_cmd(const char *cmd)
 
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
-	struct rebase_options options = {
-		.type = REBASE_UNSPECIFIED,
-		.flags = REBASE_NO_QUIET,
-		.git_am_opts = ARGV_ARRAY_INIT,
-		.allow_empty_message = 1,
-		.git_format_patch_opt = STRBUF_INIT,
-	};
+	struct rebase_options options = REBASE_OPTIONS_INIT;
 	const char *branch_name;
 	int ret, flags, total_argc, in_progress = 0;
 	int ok_to_skip_pre_rebase = 0;
@@ -1540,6 +1560,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
+	options.allow_empty_message = 1;
 	git_config(rebase_config, &options);
 
 	strbuf_reset(&buf);
-- 
2.21.0
Junio C Hamano· Mar 21, 2019, 04:21 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 08/11] rebase -i: use struct rebase_options to parse args

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 20 quoted lines
> +static struct replay_opts get_replay_opts(const struct rebase_options *opts)
> +{
> +	struct replay_opts replay = REPLAY_OPTS_INIT;
> +
> +	sequencer_init_config(&replay);
> +
> +	replay.action = REPLAY_INTERACTIVE_REBASE;
> +	replay.signoff = opts->signoff;
> +	replay.allow_ff = !(opts->flags & REBASE_FORCE);
> +	if (opts->allow_rerere_autoupdate)
> +		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
> +	replay.allow_empty = 1;
> +	replay.allow_empty_message = opts->allow_empty_message;
> +	replay.verbose = opts->flags & REBASE_VERBOSE;
> +	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
> +	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
> +	replay.strategy = opts->strategy;
> +
> +	return replay;
> +}

This calls init_config() and then sets .action; does it revert to what dl/merge-cleanup-scissors-fix wants to do, which flipped the order to fix some bug? It is a bit hard to tell.

Unfortunately because of the earlier huge code movement the changes to _this_ file does not conflict and cleanly merges, but because the other file is removed by this series while a topic in flight updates it, the semantic conflict like this luckily gets discovered.

Especially since this is still an RFC, I'd preferred to see it without moving around the code too much (instead, exporting some symbols that need to be visible with each other after renaming them to more appropriate names that are fit in the global namespace).

Phillip Wood· Mar 21, 2019, 14:59 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 08/11] rebase -i: use struct rebase_options to parse args

Hi Junio
On 21/03/2019 04:21, Junio C Hamano wrote:
Show 26 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> +static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>> +{
>> +	struct replay_opts replay = REPLAY_OPTS_INIT;
>> +
>> +	sequencer_init_config(&replay);
>> +
>> +	replay.action = REPLAY_INTERACTIVE_REBASE;
>> +	replay.signoff = opts->signoff;
>> +	replay.allow_ff = !(opts->flags & REBASE_FORCE);
>> +	if (opts->allow_rerere_autoupdate)
>> +		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
>> +	replay.allow_empty = 1;
>> +	replay.allow_empty_message = opts->allow_empty_message;
>> +	replay.verbose = opts->flags & REBASE_VERBOSE;
>> +	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>> +	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>> +	replay.strategy = opts->strategy;
>> +
>> +	return replay;
>> +}
> 
> This calls init_config() and then sets .action; does it revert to
> what dl/merge-cleanup-scissors-fix wants to do, which flipped the
> order to fix some bug?  It is a bit hard to tell.

dl/merge-cleanup-scissors-fix changes sequencer_init_config() to depend on the value of action so action should to be set first. What is in pu at the moment in not quite right (though I'm not sure what the practical implications are as it looks like the rebase tests are passing[1]).

[1] https://travis-ci.org/git/git/jobs/509332448
Show 9 quoted lines
> Unfortunately because of the earlier huge code movement the changes
> to _this_ file does not conflict and cleanly merges, but because the
> other file is removed by this series while a topic in flight updates
> it, the semantic conflict like this luckily gets discovered.
> 
> Especially since this is still an RFC, I'd preferred to see it
> without moving around the code too much (instead, exporting some
> symbols that need to be visible with each other after renaming them
> to more appropriate names that are fit in the global namespace).

I'm happy to try doing that, though the textual conflict would be in a different place in the file to the semantic conflict but at least they'd both be in the same file. I think it would only need to share the definitions of struct rebase_options, enum rebase_action and the declaration of run_rebase_interactive(). Would you be happy with the addition of builtin/rebase.h (there don't seem to be another headers in that directory). We could leave rebase--interactive.c around until rebase--preserve-merges.sh is finally removed.

Best Wishes
Phillip
Junio C Hamano· Mar 22, 2019, 03:34 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 08/11] rebase -i: use struct rebase_options to parse args

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 37 quoted lines
> Hi Junio
>
> On 21/03/2019 04:21, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> +static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>>> +{
>>> +	struct replay_opts replay = REPLAY_OPTS_INIT;
>>> +
>>> +	sequencer_init_config(&replay);
>>> +
>>> +	replay.action = REPLAY_INTERACTIVE_REBASE;
>>> +	replay.signoff = opts->signoff;
>>> +	replay.allow_ff = !(opts->flags & REBASE_FORCE);
>>> +	if (opts->allow_rerere_autoupdate)
>>> +		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
>>> +	replay.allow_empty = 1;
>>> +	replay.allow_empty_message = opts->allow_empty_message;
>>> +	replay.verbose = opts->flags & REBASE_VERBOSE;
>>> +	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>>> +	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>>> +	replay.strategy = opts->strategy;
>>> +
>>> +	return replay;
>>> +}
>>
>> This calls init_config() and then sets .action; does it revert to
>> what dl/merge-cleanup-scissors-fix wants to do, which flipped the
>> order to fix some bug?  It is a bit hard to tell.
>
> dl/merge-cleanup-scissors-fix changes sequencer_init_config() to
> depend on the value of action so action should to be set first. What
> is in pu at the moment in not quite right (though I'm not sure what
> the practical implications are as it looks like the rebase tests are
> passing[1]).
>
> [1] https://travis-ci.org/git/git/jobs/509332448

Yes, it actually is the reason why I left the seemingly-wrong merge in 'pu' on purpose to see if we have enough test coverage.

Show 6 quoted lines
> they'd both be in the same file. I think it would only need to share
> the definitions of struct rebase_options, enum rebase_action and the
> declaration of run_rebase_interactive(). Would you be happy with the
> addition of builtin/rebase.h (there don't seem to be another headers
> in that directory). We could leave rebase--interactive.c around until
> rebase--preserve-merges.sh is finally removed.

I think dl/merge-cleanup-scissors-fix is getting solid enough, so a better alternative may be to base this on top of

	reset e902e9bcae ;# The second batch
	merge ag/sequencer-reduce-rewriting-todo
	merge dl/merge-cleanup-scissors-fix
instead of basing it only on ag/sequencer-reduce-rewriting-todo.
Thanks.
Alban Gruin· Mar 21, 2019, 21:13 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 08/11] rebase -i: use struct rebase_options to parse args

Hi Phillip,
It’s nice to see your work on this on the list.
Le 19/03/2019 à 20:03, Phillip Wood a écrit :
Show 86 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> In order to run `rebase -i` without forking `rebase--interactive` it
> will be convenient to use the same structure when parsing the options in
> cmd_rebase() and cmd_rebase__interactive().
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 203 ++++++++++++++++++++++++++---------------------
>  1 file changed, 112 insertions(+), 91 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index c93f2aa629..33a2495032 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -50,6 +50,73 @@ enum rebase_type {
>  	REBASE_PRESERVE_MERGES
>  };
>  
> +struct rebase_options {
> +	enum rebase_type type;
> +	const char *state_dir;
> +	struct commit *upstream;
> +	const char *upstream_name;
> +	const char *upstream_arg;
> +	char *head_name;
> +	struct object_id orig_head;
> +	struct commit *onto;
> +	const char *onto_name;
> +	const char *revisions;
> +	const char *switch_to;
> +	int root;
> +	struct object_id *squash_onto;
> +	struct commit *restrict_revision;
> +	int dont_finish_rebase;
> +	enum {
> +		REBASE_NO_QUIET = 1<<0,
> +		REBASE_VERBOSE = 1<<1,
> +		REBASE_DIFFSTAT = 1<<2,
> +		REBASE_FORCE = 1<<3,
> +		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> +	} flags;
> +	struct argv_array git_am_opts;
> +	const char *action;
> +	int signoff;
> +	int allow_rerere_autoupdate;
> +	int keep_empty;
> +	int autosquash;
> +	char *gpg_sign_opt;
> +	int autostash;
> +	char *cmd;
> +	int allow_empty_message;
> +	int rebase_merges, rebase_cousins;
> +	char *strategy, *strategy_opts;
> +	struct strbuf git_format_patch_opt;
> +	int reschedule_failed_exec;
> +};
> +
> +#define REBASE_OPTIONS_INIT {			  	\
> +		.type = REBASE_UNSPECIFIED,	  	\
> +		.flags = REBASE_NO_QUIET, 		\
> +		.git_am_opts = ARGV_ARRAY_INIT,		\
> +		.git_format_patch_opt = STRBUF_INIT	\
> +	}
> +
> +static struct replay_opts get_replay_opts(const struct rebase_options *opts)
> +{
> +	struct replay_opts replay = REPLAY_OPTS_INIT;
> +
> +	sequencer_init_config(&replay);
> +
> +	replay.action = REPLAY_INTERACTIVE_REBASE;
> +	replay.signoff = opts->signoff;
> +	replay.allow_ff = !(opts->flags & REBASE_FORCE);
> +	if (opts->allow_rerere_autoupdate)
> +		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
> +	replay.allow_empty = 1;
> +	replay.allow_empty_message = opts->allow_empty_message;
> +	replay.verbose = opts->flags & REBASE_VERBOSE;
> +	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
> +	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
> +	replay.strategy = opts->strategy;
> +
> +	return replay;
> +}
> +

I wonder if `struct rebase_options` and `struct replay_options` could be merged, or at least have `replay_options` used in `rebase_options`, instead of converting one to the other. I think it would make things simpler and cleaner, but I don’t know how hard it would be, or if my assumption is correct.

Cheers, Alban

Phillip Wood· Apr 10, 2019, 19:16 UTC · re: Alban Gruin · lore

Re: [RFC PATCH 08/11] rebase -i: use struct rebase_options to parse args

Hi Alban

sorry for the slow reply, I think you're probably off-list for a while by now

On 21/03/2019 21:13, Alban Gruin wrote:
Show 97 quoted lines
> Hi Phillip,
> 
> It’s nice to see your work on this on the list.
> 
> Le 19/03/2019 à 20:03, Phillip Wood a écrit :
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> In order to run `rebase -i` without forking `rebase--interactive` it
>> will be convenient to use the same structure when parsing the options in
>> cmd_rebase() and cmd_rebase__interactive().
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/rebase.c | 203 ++++++++++++++++++++++++++---------------------
>>   1 file changed, 112 insertions(+), 91 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index c93f2aa629..33a2495032 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -50,6 +50,73 @@ enum rebase_type {
>>   	REBASE_PRESERVE_MERGES
>>   };
>>   
>> +struct rebase_options {
>> +	enum rebase_type type;
>> +	const char *state_dir;
>> +	struct commit *upstream;
>> +	const char *upstream_name;
>> +	const char *upstream_arg;
>> +	char *head_name;
>> +	struct object_id orig_head;
>> +	struct commit *onto;
>> +	const char *onto_name;
>> +	const char *revisions;
>> +	const char *switch_to;
>> +	int root;
>> +	struct object_id *squash_onto;
>> +	struct commit *restrict_revision;
>> +	int dont_finish_rebase;
>> +	enum {
>> +		REBASE_NO_QUIET = 1<<0,
>> +		REBASE_VERBOSE = 1<<1,
>> +		REBASE_DIFFSTAT = 1<<2,
>> +		REBASE_FORCE = 1<<3,
>> +		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
>> +	} flags;
>> +	struct argv_array git_am_opts;
>> +	const char *action;
>> +	int signoff;
>> +	int allow_rerere_autoupdate;
>> +	int keep_empty;
>> +	int autosquash;
>> +	char *gpg_sign_opt;
>> +	int autostash;
>> +	char *cmd;
>> +	int allow_empty_message;
>> +	int rebase_merges, rebase_cousins;
>> +	char *strategy, *strategy_opts;
>> +	struct strbuf git_format_patch_opt;
>> +	int reschedule_failed_exec;
>> +};
>> +
>> +#define REBASE_OPTIONS_INIT {			  	\
>> +		.type = REBASE_UNSPECIFIED,	  	\
>> +		.flags = REBASE_NO_QUIET, 		\
>> +		.git_am_opts = ARGV_ARRAY_INIT,		\
>> +		.git_format_patch_opt = STRBUF_INIT	\
>> +	}
>> +
>> +static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>> +{
>> +	struct replay_opts replay = REPLAY_OPTS_INIT;
>> +
>> +	sequencer_init_config(&replay);
>> +
>> +	replay.action = REPLAY_INTERACTIVE_REBASE;
>> +	replay.signoff = opts->signoff;
>> +	replay.allow_ff = !(opts->flags & REBASE_FORCE);
>> +	if (opts->allow_rerere_autoupdate)
>> +		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
>> +	replay.allow_empty = 1;
>> +	replay.allow_empty_message = opts->allow_empty_message;
>> +	replay.verbose = opts->flags & REBASE_VERBOSE;
>> +	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>> +	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>> +	replay.strategy = opts->strategy;
>> +
>> +	return replay;
>> +}
>> +
> 
> I wonder if `struct rebase_options` and `struct replay_options` could be
> merged, or at least have `replay_options` used in `rebase_options`,
> instead of converting one to the other.  I think it would make things
> simpler and cleaner, but I don’t know how hard it would be, or if my
> assumption is correct.

I did consider doing that, but there are a few subtle differences in the way the options are stored in each struct (eg the gpg option has a leading -S in struct rebase_options but not struct replay_options) and I think it would be a bit of a faff to align them as it would mean messing with the code uses them and the code that reads/writes the various state files (we cannot change the on-disk format without breaking things - sometimes users start a rebase with one version of git bundled with something like tig and then continue on the command-line with a different version). I'd prefer to leave it as a future cleanup once this series has been merged.

Best Wishes
Phillip
> 
> Cheers,
> Alban
> 
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 10/11] rebase: use a common action enum

From: Phillip Wood <phillip.wood@dunelm.org.uk>

cmd_rebase() and cmd_rebase__interactive() used different enums to hold the current action. Change to using a common enum so the values are the same when we change `rebase -i` to avoid forking `rebase--interactive`.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
Notes:
    I'm not sure what to do with action_names, I moved it to keep it near
    the definition of the enum. The extra commands in the enum are not
    traced so they don't need to be in action_names but it looks odd to
    leave them out. Also I don't understand why action_names in NULL
    terminated or the names are marked for translation as it is used in
    	trace2_cmd_mode(action_names[action]);
    so we don't use the NULL termination or translate the names (I'd be
    surprised if we did for tracing)
 builtin/rebase.c | 93 +++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 44 deletions(-)
Show changes to builtin/rebase.c +49 −44
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 21ccb6495e..26d25a5242 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -119,6 +119,30 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	return replay;
 }
 
+enum action {
+	ACTION_NONE = 0,
+	ACTION_CONTINUE,
+	ACTION_SKIP,
+	ACTION_ABORT,
+	ACTION_QUIT,
+	ACTION_EDIT_TODO,
+	ACTION_SHOW_CURRENT_PATCH,
+	ACTION_SHORTEN_OIDS,
+	ACTION_EXPAND_OIDS,
+	ACTION_CHECK_TODO_LIST,
+	ACTION_REARRANGE_SQUASH,
+	ACTION_ADD_EXEC
+};
+
+static const char *action_names[] = { N_("undefined"),
+				      N_("continue"),
+				      N_("skip"),
+				      N_("abort"),
+				      N_("quit"),
+				      N_("edit_todo"),
+				      N_("show_current_patch"),
+				      NULL };
+
 static int add_exec_commands(struct string_list *commands)
 {
 	const char *todo_file = rebase_path_todo();
@@ -347,10 +371,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
-	enum {
-		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
-		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
-	} command = 0;
+	enum action command = ACTION_NONE;
 	struct option options[] = {
 		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
 			   REBASE_FORCE),
@@ -367,22 +388,22 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			N_("display a diffstat of what changed upstream"),
 			REBASE_NO_QUIET | REBASE_VERBOSE | REBASE_DIFFSTAT),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
-			    CONTINUE),
-		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
+			    ACTION_CONTINUE),
+		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), ACTION_SKIP),
 		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
-			    EDIT_TODO),
+			    ACTION_EDIT_TODO),
 		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
-			    SHOW_CURRENT_PATCH),
+			    ACTION_SHOW_CURRENT_PATCH),
 		OPT_CMDMODE(0, "shorten-ids", &command,
-			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
+			N_("shorten commit ids in the todo list"), ACTION_SHORTEN_OIDS),
 		OPT_CMDMODE(0, "expand-ids", &command,
-			N_("expand commit ids in the todo list"), EXPAND_OIDS),
+			N_("expand commit ids in the todo list"), ACTION_EXPAND_OIDS),
 		OPT_CMDMODE(0, "check-todo-list", &command,
-			N_("check the todo list"), CHECK_TODO_LIST),
+			N_("check the todo list"), ACTION_CHECK_TODO_LIST),
 		OPT_CMDMODE(0, "rearrange-squash", &command,
-			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+			N_("rearrange fixup/squash lines"), ACTION_REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
-			N_("insert exec commands in todo list"), ADD_EXEC),
+			N_("insert exec commands in todo list"), ACTION_ADD_EXEC),
 		{ OPTION_CALLBACK, 0, "onto", &opts.onto, N_("onto"), N_("onto"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
 		{ OPTION_CALLBACK, 0, "restrict-revision", &opts.restrict_revision,
@@ -428,36 +449,36 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
-	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
 	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
 	switch (command) {
-	case NONE: {
+	case ACTION_NONE: {
 		if (!opts.onto && !opts.upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
 		ret = do_interactive_rebase(&opts, flags);
 		break;
 	}
-	case SKIP: {
+	case ACTION_SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		rerere_clear(the_repository, &merge_rr);
 	}
 		/* fallthrough */
-	case CONTINUE: {
+	case ACTION_CONTINUE: {
 		struct replay_opts replay_opts = get_replay_opts(&opts);
 
 		ret = sequencer_continue(the_repository, &replay_opts);
 		break;
 	}
-	case EDIT_TODO:
+	case ACTION_EDIT_TODO:
 		ret = edit_todo_file(flags);
 		break;
-	case SHOW_CURRENT_PATCH: {
+	case ACTION_SHOW_CURRENT_PATCH: {
 		struct child_process cmd = CHILD_PROCESS_INIT;
 
 		cmd.git_cmd = 1;
@@ -466,17 +487,17 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 
 		break;
 	}
-	case SHORTEN_OIDS:
-	case EXPAND_OIDS:
+	case ACTION_SHORTEN_OIDS:
+	case ACTION_EXPAND_OIDS:
 		ret = transform_todo_file(flags);
 		break;
-	case CHECK_TODO_LIST:
+	case ACTION_CHECK_TODO_LIST:
 		ret = check_todo_list_from_file(the_repository);
 		break;
-	case REARRANGE_SQUASH:
+	case ACTION_REARRANGE_SQUASH:
 		ret = rearrange_squash_in_todo_file();
 		break;
-	case ADD_EXEC: {
+	case ACTION_ADD_EXEC: {
 		struct string_list commands = STRING_LIST_INIT_DUP;
 
 		split_exec_commands(opts.cmd, &commands);
@@ -1417,23 +1438,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id merge_base;
-	enum {
-		NO_ACTION,
-		ACTION_CONTINUE,
-		ACTION_SKIP,
-		ACTION_ABORT,
-		ACTION_QUIT,
-		ACTION_EDIT_TODO,
-		ACTION_SHOW_CURRENT_PATCH,
-	} action = NO_ACTION;
-	static const char *action_names[] = { N_("undefined"),
-					      N_("continue"),
-					      N_("skip"),
-					      N_("abort"),
-					      N_("quit"),
-					      N_("edit_todo"),
-					      N_("show_current_patch"),
-					      NULL };
+	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
@@ -1600,7 +1605,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
 
-	if (action != NO_ACTION && total_argc != 2) {
+	if (action != ACTION_NONE && total_argc != 2) {
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 	}
@@ -1609,7 +1614,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 
-	if (action != NO_ACTION && !in_progress)
+	if (action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
 	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
@@ -1709,7 +1714,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.action = "show-current-patch";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
-	case NO_ACTION:
+	case ACTION_NONE:
 		break;
 	default:
 		BUG("action: %d", action);
-- 
2.21.0
Ævar Arnfjörð Bjarmason· Mar 19, 2019, 20:24 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 10/11] rebase: use a common action enum

On Tue, Mar 19 2019, Phillip Wood wrote:
Show 18 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> cmd_rebase() and cmd_rebase__interactive() used different enums to hold
> the current action. Change to using a common enum so the values are the
> same when we change `rebase -i` to avoid forking `rebase--interactive`.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>
> Notes:
>     I'm not sure what to do with action_names, I moved it to keep it near
>     the definition of the enum. The extra commands in the enum are not
>     traced so they don't need to be in action_names but it looks odd to
>     leave them out. Also I don't understand why action_names in NULL
>     terminated or the names are marked for translation as it is used in
>     	trace2_cmd_mode(action_names[action]);
>     so we don't use the NULL termination or translate the names (I'd be
>     surprised if we did for tracing)

Making them for translation is a bug in the recent b3a5d5a80c ("trace2:data: add subverb for rebase", 2019-02-22) and just looks like copy/paste cargo-culting from somewhere else, and likewise with the trailing NULL which doesn't make sense in this case.

Show 200 quoted lines
>  builtin/rebase.c | 93 +++++++++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 44 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 21ccb6495e..26d25a5242 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -119,6 +119,30 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>  	return replay;
>  }
>
> +enum action {
> +	ACTION_NONE = 0,
> +	ACTION_CONTINUE,
> +	ACTION_SKIP,
> +	ACTION_ABORT,
> +	ACTION_QUIT,
> +	ACTION_EDIT_TODO,
> +	ACTION_SHOW_CURRENT_PATCH,
> +	ACTION_SHORTEN_OIDS,
> +	ACTION_EXPAND_OIDS,
> +	ACTION_CHECK_TODO_LIST,
> +	ACTION_REARRANGE_SQUASH,
> +	ACTION_ADD_EXEC
> +};
> +
> +static const char *action_names[] = { N_("undefined"),
> +				      N_("continue"),
> +				      N_("skip"),
> +				      N_("abort"),
> +				      N_("quit"),
> +				      N_("edit_todo"),
> +				      N_("show_current_patch"),
> +				      NULL };
> +
>  static int add_exec_commands(struct string_list *commands)
>  {
>  	const char *todo_file = rebase_path_todo();
> @@ -347,10 +371,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>  	unsigned flags = 0;
>  	int abbreviate_commands = 0, ret = 0;
>  	struct object_id squash_onto = null_oid;
> -	enum {
> -		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
> -		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
> -	} command = 0;
> +	enum action command = ACTION_NONE;
>  	struct option options[] = {
>  		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
>  			   REBASE_FORCE),
> @@ -367,22 +388,22 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>  			N_("display a diffstat of what changed upstream"),
>  			REBASE_NO_QUIET | REBASE_VERBOSE | REBASE_DIFFSTAT),
>  		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
> -			    CONTINUE),
> -		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
> +			    ACTION_CONTINUE),
> +		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), ACTION_SKIP),
>  		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
> -			    EDIT_TODO),
> +			    ACTION_EDIT_TODO),
>  		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
> -			    SHOW_CURRENT_PATCH),
> +			    ACTION_SHOW_CURRENT_PATCH),
>  		OPT_CMDMODE(0, "shorten-ids", &command,
> -			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
> +			N_("shorten commit ids in the todo list"), ACTION_SHORTEN_OIDS),
>  		OPT_CMDMODE(0, "expand-ids", &command,
> -			N_("expand commit ids in the todo list"), EXPAND_OIDS),
> +			N_("expand commit ids in the todo list"), ACTION_EXPAND_OIDS),
>  		OPT_CMDMODE(0, "check-todo-list", &command,
> -			N_("check the todo list"), CHECK_TODO_LIST),
> +			N_("check the todo list"), ACTION_CHECK_TODO_LIST),
>  		OPT_CMDMODE(0, "rearrange-squash", &command,
> -			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
> +			N_("rearrange fixup/squash lines"), ACTION_REARRANGE_SQUASH),
>  		OPT_CMDMODE(0, "add-exec-commands", &command,
> -			N_("insert exec commands in todo list"), ADD_EXEC),
> +			N_("insert exec commands in todo list"), ACTION_ADD_EXEC),
>  		{ OPTION_CALLBACK, 0, "onto", &opts.onto, N_("onto"), N_("onto"),
>  		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
>  		{ OPTION_CALLBACK, 0, "restrict-revision", &opts.restrict_revision,
> @@ -428,36 +449,36 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>  	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
>  	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
>  	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
> -	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
> +	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
>
>  	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
>  		warning(_("--[no-]rebase-cousins has no effect without "
>  			  "--rebase-merges"));
>
>  	switch (command) {
> -	case NONE: {
> +	case ACTION_NONE: {
>  		if (!opts.onto && !opts.upstream)
>  			die(_("a base commit must be provided with --upstream or --onto"));
>
>  		ret = do_interactive_rebase(&opts, flags);
>  		break;
>  	}
> -	case SKIP: {
> +	case ACTION_SKIP: {
>  		struct string_list merge_rr = STRING_LIST_INIT_DUP;
>
>  		rerere_clear(the_repository, &merge_rr);
>  	}
>  		/* fallthrough */
> -	case CONTINUE: {
> +	case ACTION_CONTINUE: {
>  		struct replay_opts replay_opts = get_replay_opts(&opts);
>
>  		ret = sequencer_continue(the_repository, &replay_opts);
>  		break;
>  	}
> -	case EDIT_TODO:
> +	case ACTION_EDIT_TODO:
>  		ret = edit_todo_file(flags);
>  		break;
> -	case SHOW_CURRENT_PATCH: {
> +	case ACTION_SHOW_CURRENT_PATCH: {
>  		struct child_process cmd = CHILD_PROCESS_INIT;
>
>  		cmd.git_cmd = 1;
> @@ -466,17 +487,17 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>
>  		break;
>  	}
> -	case SHORTEN_OIDS:
> -	case EXPAND_OIDS:
> +	case ACTION_SHORTEN_OIDS:
> +	case ACTION_EXPAND_OIDS:
>  		ret = transform_todo_file(flags);
>  		break;
> -	case CHECK_TODO_LIST:
> +	case ACTION_CHECK_TODO_LIST:
>  		ret = check_todo_list_from_file(the_repository);
>  		break;
> -	case REARRANGE_SQUASH:
> +	case ACTION_REARRANGE_SQUASH:
>  		ret = rearrange_squash_in_todo_file();
>  		break;
> -	case ADD_EXEC: {
> +	case ACTION_ADD_EXEC: {
>  		struct string_list commands = STRING_LIST_INIT_DUP;
>
>  		split_exec_commands(opts.cmd, &commands);
> @@ -1417,23 +1438,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	struct strbuf revisions = STRBUF_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct object_id merge_base;
> -	enum {
> -		NO_ACTION,
> -		ACTION_CONTINUE,
> -		ACTION_SKIP,
> -		ACTION_ABORT,
> -		ACTION_QUIT,
> -		ACTION_EDIT_TODO,
> -		ACTION_SHOW_CURRENT_PATCH,
> -	} action = NO_ACTION;
> -	static const char *action_names[] = { N_("undefined"),
> -					      N_("continue"),
> -					      N_("skip"),
> -					      N_("abort"),
> -					      N_("quit"),
> -					      N_("edit_todo"),
> -					      N_("show_current_patch"),
> -					      NULL };
> +	enum action action = ACTION_NONE;
>  	const char *gpg_sign = NULL;
>  	struct string_list exec = STRING_LIST_INIT_NODUP;
>  	const char *rebase_merges = NULL;
> @@ -1600,7 +1605,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			     builtin_rebase_options,
>  			     builtin_rebase_usage, 0);
>
> -	if (action != NO_ACTION && total_argc != 2) {
> +	if (action != ACTION_NONE && total_argc != 2) {
>  		usage_with_options(builtin_rebase_usage,
>  				   builtin_rebase_options);
>  	}
> @@ -1609,7 +1614,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		usage_with_options(builtin_rebase_usage,
>  				   builtin_rebase_options);
>
> -	if (action != NO_ACTION && !in_progress)
> +	if (action != ACTION_NONE && !in_progress)
>  		die(_("No rebase in progress?"));
>  	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
>
> @@ -1709,7 +1714,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		options.action = "show-current-patch";
>  		options.dont_finish_rebase = 1;
>  		goto run_rebase;
> -	case NO_ACTION:
> +	case ACTION_NONE:
>  		break;
>  	default:
>  		BUG("action: %d", action);
Phillip Wood· Mar 21, 2019, 14:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC PATCH 10/11] rebase: use a common action enum

On 19/03/2019 20:24, Ævar Arnfjörð Bjarmason wrote:
Show 26 quoted lines
> 
> On Tue, Mar 19 2019, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> cmd_rebase() and cmd_rebase__interactive() used different enums to hold
>> the current action. Change to using a common enum so the values are the
>> same when we change `rebase -i` to avoid forking `rebase--interactive`.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>
>> Notes:
>>      I'm not sure what to do with action_names, I moved it to keep it near
>>      the definition of the enum. The extra commands in the enum are not
>>      traced so they don't need to be in action_names but it looks odd to
>>      leave them out. Also I don't understand why action_names in NULL
>>      terminated or the names are marked for translation as it is used in
>>      	trace2_cmd_mode(action_names[action]);
>>      so we don't use the NULL termination or translate the names (I'd be
>>      surprised if we did for tracing)
> 
> Making them for translation is a bug in the recent b3a5d5a80c
> ("trace2:data: add subverb for rebase", 2019-02-22) and just looks like
> copy/paste cargo-culting from somewhere else, and likewise with the
> trailing NULL which doesn't make sense in this case.

Thanks for clarifying that, I'll add a preparatory patch at the beginning of the series to fix those issues

Best Wishes
Phillip
Show 200 quoted lines
>>   builtin/rebase.c | 93 +++++++++++++++++++++++++-----------------------
>>   1 file changed, 49 insertions(+), 44 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 21ccb6495e..26d25a5242 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -119,6 +119,30 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>>   	return replay;
>>   }
>>
>> +enum action {
>> +	ACTION_NONE = 0,
>> +	ACTION_CONTINUE,
>> +	ACTION_SKIP,
>> +	ACTION_ABORT,
>> +	ACTION_QUIT,
>> +	ACTION_EDIT_TODO,
>> +	ACTION_SHOW_CURRENT_PATCH,
>> +	ACTION_SHORTEN_OIDS,
>> +	ACTION_EXPAND_OIDS,
>> +	ACTION_CHECK_TODO_LIST,
>> +	ACTION_REARRANGE_SQUASH,
>> +	ACTION_ADD_EXEC
>> +};
>> +
>> +static const char *action_names[] = { N_("undefined"),
>> +				      N_("continue"),
>> +				      N_("skip"),
>> +				      N_("abort"),
>> +				      N_("quit"),
>> +				      N_("edit_todo"),
>> +				      N_("show_current_patch"),
>> +				      NULL };
>> +
>>   static int add_exec_commands(struct string_list *commands)
>>   {
>>   	const char *todo_file = rebase_path_todo();
>> @@ -347,10 +371,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>>   	unsigned flags = 0;
>>   	int abbreviate_commands = 0, ret = 0;
>>   	struct object_id squash_onto = null_oid;
>> -	enum {
>> -		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
>> -		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
>> -	} command = 0;
>> +	enum action command = ACTION_NONE;
>>   	struct option options[] = {
>>   		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
>>   			   REBASE_FORCE),
>> @@ -367,22 +388,22 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>>   			N_("display a diffstat of what changed upstream"),
>>   			REBASE_NO_QUIET | REBASE_VERBOSE | REBASE_DIFFSTAT),
>>   		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
>> -			    CONTINUE),
>> -		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
>> +			    ACTION_CONTINUE),
>> +		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), ACTION_SKIP),
>>   		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
>> -			    EDIT_TODO),
>> +			    ACTION_EDIT_TODO),
>>   		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
>> -			    SHOW_CURRENT_PATCH),
>> +			    ACTION_SHOW_CURRENT_PATCH),
>>   		OPT_CMDMODE(0, "shorten-ids", &command,
>> -			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
>> +			N_("shorten commit ids in the todo list"), ACTION_SHORTEN_OIDS),
>>   		OPT_CMDMODE(0, "expand-ids", &command,
>> -			N_("expand commit ids in the todo list"), EXPAND_OIDS),
>> +			N_("expand commit ids in the todo list"), ACTION_EXPAND_OIDS),
>>   		OPT_CMDMODE(0, "check-todo-list", &command,
>> -			N_("check the todo list"), CHECK_TODO_LIST),
>> +			N_("check the todo list"), ACTION_CHECK_TODO_LIST),
>>   		OPT_CMDMODE(0, "rearrange-squash", &command,
>> -			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
>> +			N_("rearrange fixup/squash lines"), ACTION_REARRANGE_SQUASH),
>>   		OPT_CMDMODE(0, "add-exec-commands", &command,
>> -			N_("insert exec commands in todo list"), ADD_EXEC),
>> +			N_("insert exec commands in todo list"), ACTION_ADD_EXEC),
>>   		{ OPTION_CALLBACK, 0, "onto", &opts.onto, N_("onto"), N_("onto"),
>>   		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
>>   		{ OPTION_CALLBACK, 0, "restrict-revision", &opts.restrict_revision,
>> @@ -428,36 +449,36 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>>   	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
>>   	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
>>   	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
>> -	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
>> +	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
>>
>>   	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
>>   		warning(_("--[no-]rebase-cousins has no effect without "
>>   			  "--rebase-merges"));
>>
>>   	switch (command) {
>> -	case NONE: {
>> +	case ACTION_NONE: {
>>   		if (!opts.onto && !opts.upstream)
>>   			die(_("a base commit must be provided with --upstream or --onto"));
>>
>>   		ret = do_interactive_rebase(&opts, flags);
>>   		break;
>>   	}
>> -	case SKIP: {
>> +	case ACTION_SKIP: {
>>   		struct string_list merge_rr = STRING_LIST_INIT_DUP;
>>
>>   		rerere_clear(the_repository, &merge_rr);
>>   	}
>>   		/* fallthrough */
>> -	case CONTINUE: {
>> +	case ACTION_CONTINUE: {
>>   		struct replay_opts replay_opts = get_replay_opts(&opts);
>>
>>   		ret = sequencer_continue(the_repository, &replay_opts);
>>   		break;
>>   	}
>> -	case EDIT_TODO:
>> +	case ACTION_EDIT_TODO:
>>   		ret = edit_todo_file(flags);
>>   		break;
>> -	case SHOW_CURRENT_PATCH: {
>> +	case ACTION_SHOW_CURRENT_PATCH: {
>>   		struct child_process cmd = CHILD_PROCESS_INIT;
>>
>>   		cmd.git_cmd = 1;
>> @@ -466,17 +487,17 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>>
>>   		break;
>>   	}
>> -	case SHORTEN_OIDS:
>> -	case EXPAND_OIDS:
>> +	case ACTION_SHORTEN_OIDS:
>> +	case ACTION_EXPAND_OIDS:
>>   		ret = transform_todo_file(flags);
>>   		break;
>> -	case CHECK_TODO_LIST:
>> +	case ACTION_CHECK_TODO_LIST:
>>   		ret = check_todo_list_from_file(the_repository);
>>   		break;
>> -	case REARRANGE_SQUASH:
>> +	case ACTION_REARRANGE_SQUASH:
>>   		ret = rearrange_squash_in_todo_file();
>>   		break;
>> -	case ADD_EXEC: {
>> +	case ACTION_ADD_EXEC: {
>>   		struct string_list commands = STRING_LIST_INIT_DUP;
>>
>>   		split_exec_commands(opts.cmd, &commands);
>> @@ -1417,23 +1438,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   	struct strbuf revisions = STRBUF_INIT;
>>   	struct strbuf buf = STRBUF_INIT;
>>   	struct object_id merge_base;
>> -	enum {
>> -		NO_ACTION,
>> -		ACTION_CONTINUE,
>> -		ACTION_SKIP,
>> -		ACTION_ABORT,
>> -		ACTION_QUIT,
>> -		ACTION_EDIT_TODO,
>> -		ACTION_SHOW_CURRENT_PATCH,
>> -	} action = NO_ACTION;
>> -	static const char *action_names[] = { N_("undefined"),
>> -					      N_("continue"),
>> -					      N_("skip"),
>> -					      N_("abort"),
>> -					      N_("quit"),
>> -					      N_("edit_todo"),
>> -					      N_("show_current_patch"),
>> -					      NULL };
>> +	enum action action = ACTION_NONE;
>>   	const char *gpg_sign = NULL;
>>   	struct string_list exec = STRING_LIST_INIT_NODUP;
>>   	const char *rebase_merges = NULL;
>> @@ -1600,7 +1605,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   			     builtin_rebase_options,
>>   			     builtin_rebase_usage, 0);
>>
>> -	if (action != NO_ACTION && total_argc != 2) {
>> +	if (action != ACTION_NONE && total_argc != 2) {
>>   		usage_with_options(builtin_rebase_usage,
>>   				   builtin_rebase_options);
>>   	}
>> @@ -1609,7 +1614,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		usage_with_options(builtin_rebase_usage,
>>   				   builtin_rebase_options);
>>
>> -	if (action != NO_ACTION && !in_progress)
>> +	if (action != ACTION_NONE && !in_progress)
>>   		die(_("No rebase in progress?"));
>>   	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
>>
>> @@ -1709,7 +1714,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		options.action = "show-current-patch";
>>   		options.dont_finish_rebase = 1;
>>   		goto run_rebase;
>> -	case NO_ACTION:
>> +	case ACTION_NONE:
>>   		break;
>>   	default:
>>   		BUG("action: %d", action);
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 09/11] rebase -i: use struct rebase_options in do_interactive_rebase()

From: Phillip Wood <phillip.wood@dunelm.org.uk>

All the parameters that are passed to do_interactive_rebase() apart from `flags` are already in `struct rebase_options` so there is no need to pass them separately.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 69 +++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)
Show changes to builtin/rebase.c +36 −33
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 33a2495032..21ccb6495e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -113,6 +113,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
 	replay.strategy = opts->strategy;
+	if (opts->strategy_opts)
+		parse_strategy_opts(&replay, opts->strategy_opts);
 
 	return replay;
 }
@@ -262,44 +264,50 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 	return write_basic_state(opts, head_name, onto, orig_head);
 }
 
-static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
-				 const char *switch_to, struct commit *upstream,
-				 struct commit *onto, const char *onto_name,
-				 struct object_id *squash_onto, const char *head_name,
-				 struct commit *restrict_revision, char *raw_strategies,
-				 struct string_list *commands, unsigned autosquash)
+static void split_exec_commands(const char *cmd, struct string_list *commands)
+{
+	if (cmd && *cmd) {
+		string_list_split(commands, cmd, '\n', -1);
+
+		/* rebase.c adds a new line to cmd after every command,
+		 * so here the last command is always empty */
+		string_list_remove_empty_items(commands, 0);
+	}
+}
+
+static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
 	int ret;
 	const char *head_hash = NULL;
 	char *revisions = NULL, *shortrevisions = NULL;
 	struct argv_array make_script_args = ARGV_ARRAY_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
+	struct replay_opts replay = get_replay_opts(opts);
+	struct string_list commands = STRING_LIST_INIT_DUP;
 
-	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
+	if (prepare_branch_to_be_rebased(the_repository, &replay,
+					 opts->switch_to))
 		return -1;
 
-	if (get_revision_ranges(upstream, onto, &head_hash,
+	if (get_revision_ranges(opts->upstream, opts->onto, &head_hash,
 				&revisions, &shortrevisions))
 		return -1;
 
-	if (raw_strategies)
-		parse_strategy_opts(opts, raw_strategies);
-
-	if (init_basic_state(opts, head_name, onto, head_hash)) {
+	if (init_basic_state(&replay, opts->head_name, opts->onto, head_hash)) {
 		free(revisions);
 		free(shortrevisions);
 
 		return -1;
 	}
 
-	if (!upstream && squash_onto)
+	if (!opts->upstream && opts->squash_onto)
 		write_file(path_squash_onto(), "%s\n",
-			   oid_to_hex(squash_onto));
+			   oid_to_hex(opts->squash_onto));
 
 	argv_array_pushl(&make_script_args, "", revisions, NULL);
-	if (restrict_revision)
+	if (opts->restrict_revision)
 		argv_array_push(&make_script_args,
-				oid_to_hex(&restrict_revision->object.oid));
+				oid_to_hex(&opts->restrict_revision->object.oid));
 
 	ret = sequencer_make_script(the_repository, &todo_list.buf,
 				    make_script_args.argc, make_script_args.argv,
@@ -313,10 +321,13 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 						&todo_list))
 			BUG("unusable todo list");
 
-		ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name,
-				      onto, head_hash, commands, autosquash, &todo_list);
+		split_exec_commands(opts->cmd, &commands);
+		ret = complete_action(the_repository, &replay, flags,
+			shortrevisions, opts->onto_name, opts->onto, head_hash,
+			&commands, opts->autosquash, &todo_list);
 	}
 
+	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
 	todo_list_release(&todo_list);
@@ -336,7 +347,6 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
-	struct string_list commands = STRING_LIST_INIT_DUP;
 	enum {
 		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
 		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
@@ -424,23 +434,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
-	if (opts.cmd && *opts.cmd) {
-		string_list_split(&commands, opts.cmd, '\n', -1);
-
-		/* rebase.c adds a new line to cmd after every command,
-		 * so here the last command is always empty */
-		string_list_remove_empty_items(&commands, 0);
-	}
-
 	switch (command) {
 	case NONE: {
-		struct replay_opts replay_opts = get_replay_opts(&opts);
 		if (!opts.onto && !opts.upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
-		ret = do_interactive_rebase(&replay_opts, flags, opts.switch_to, opts.upstream, opts.onto,
-					    opts.onto_name, opts.squash_onto, opts.head_name, opts.restrict_revision,
-					    opts.strategy_opts, &commands, opts.autosquash);
+		ret = do_interactive_rebase(&opts, flags);
 		break;
 	}
 	case SKIP: {
@@ -477,14 +476,18 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	case REARRANGE_SQUASH:
 		ret = rearrange_squash_in_todo_file();
 		break;
-	case ADD_EXEC:
+	case ADD_EXEC: {
+		struct string_list commands = STRING_LIST_INIT_DUP;
+
+		split_exec_commands(opts.cmd, &commands);
 		ret = add_exec_commands(&commands);
+		string_list_clear(&commands, 0);
 		break;
+	}
 	default:
 		BUG("invalid command '%d'", command);
 	}
 
-	string_list_clear(&commands, 0);
 	return !!ret;
 }
 
-- 
2.21.0
Phillip Wood· Mar 19, 2019, 19:03 UTC · re: Phillip Wood · lore

[RFC PATCH 11/11] rebase -i: run without forking rebase--interactive

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When the builtin rebase starts an interactive rebase it parses the options and then repackages them and forks `rebase--interactive`. Separate the option parsing in cmd_rebase__interactive() from the business logic to allow interactive rebases can be run without forking `rebase__interactive` by calling run_rebase_interactive() directly. This makes it easy to debug the sequencer without worrying about attaching to child processes. It also makes it easy to remove cmd_rebase__interactive() in the future when git-legacy-rebase.sh and git-rebase--preserve-merges.sh are retired.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 221 ++++++++++++++++++-----------------------------
 1 file changed, 86 insertions(+), 135 deletions(-)
Show changes to builtin/rebase.c +86 −135
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 26d25a5242..2e26d350f0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -317,7 +317,9 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 				&revisions, &shortrevisions))
 		return -1;
 
-	if (init_basic_state(&replay, opts->head_name, opts->onto, head_hash)) {
+	if (init_basic_state(&replay,
+			     opts->head_name ? opts->head_name : "detached HEAD",
+			     opts->onto, head_hash)) {
 		free(revisions);
 		free(shortrevisions);
 
@@ -360,6 +362,77 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	return ret;
 }
 
+static int run_rebase_interactive(struct rebase_options *opts,
+				  enum action command)
+{
+	unsigned flags = 0;
+	int abbreviate_commands = 0, ret = 0;
+
+	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
+
+	flags |= opts->keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
+	flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
+	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+
+	switch (command) {
+	case ACTION_NONE: {
+		if (!opts->onto && !opts->upstream)
+			die(_("a base commit must be provided with --upstream or --onto"));
+
+		ret = do_interactive_rebase(opts, flags);
+		break;
+	}
+	case ACTION_SKIP: {
+		struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+		rerere_clear(the_repository, &merge_rr);
+	}
+		/* fallthrough */
+	case ACTION_CONTINUE: {
+		struct replay_opts replay_opts = get_replay_opts(opts);
+
+		ret = sequencer_continue(the_repository, &replay_opts);
+		break;
+	}
+	case ACTION_EDIT_TODO:
+		ret = edit_todo_file(flags);
+		break;
+	case ACTION_SHOW_CURRENT_PATCH: {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
+		ret = run_command(&cmd);
+
+		break;
+	}
+	case ACTION_SHORTEN_OIDS:
+	case ACTION_EXPAND_OIDS:
+		ret = transform_todo_file(flags);
+		break;
+	case ACTION_CHECK_TODO_LIST:
+		ret = check_todo_list_from_file(the_repository);
+		break;
+	case ACTION_REARRANGE_SQUASH:
+		ret = rearrange_squash_in_todo_file();
+		break;
+	case ACTION_ADD_EXEC: {
+		struct string_list commands = STRING_LIST_INIT_DUP;
+
+		split_exec_commands(opts->cmd, &commands);
+		ret = add_exec_commands(&commands);
+		string_list_clear(&commands, 0);
+		break;
+	}
+	default:
+		BUG("invalid command '%d'", command);
+	}
+
+	return ret;
+}
+
 static const char * const builtin_rebase_interactive_usage[] = {
 	N_("git rebase--interactive [<options>]"),
 	NULL
@@ -368,8 +441,6 @@ static const char * const builtin_rebase_interactive_usage[] = {
 int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 {
 	struct rebase_options opts = REBASE_OPTIONS_INIT;
-	unsigned flags = 0;
-	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
 	enum action command = ACTION_NONE;
 	struct option options[] = {
@@ -434,8 +505,6 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 
 	opts.rebase_cousins = -1;
 
-	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
-
 	if (argc == 1)
 		usage_with_options(builtin_rebase_interactive_usage, options);
 
@@ -445,71 +514,11 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	if (!is_null_oid(&squash_onto))
 		opts.squash_onto = &squash_onto;
 
-	flags |= opts.keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
-	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
-	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
-	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
-	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
-
 	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
-	switch (command) {
-	case ACTION_NONE: {
-		if (!opts.onto && !opts.upstream)
-			die(_("a base commit must be provided with --upstream or --onto"));
-
-		ret = do_interactive_rebase(&opts, flags);
-		break;
-	}
-	case ACTION_SKIP: {
-		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-
-		rerere_clear(the_repository, &merge_rr);
-	}
-		/* fallthrough */
-	case ACTION_CONTINUE: {
-		struct replay_opts replay_opts = get_replay_opts(&opts);
-
-		ret = sequencer_continue(the_repository, &replay_opts);
-		break;
-	}
-	case ACTION_EDIT_TODO:
-		ret = edit_todo_file(flags);
-		break;
-	case ACTION_SHOW_CURRENT_PATCH: {
-		struct child_process cmd = CHILD_PROCESS_INIT;
-
-		cmd.git_cmd = 1;
-		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
-		ret = run_command(&cmd);
-
-		break;
-	}
-	case ACTION_SHORTEN_OIDS:
-	case ACTION_EXPAND_OIDS:
-		ret = transform_todo_file(flags);
-		break;
-	case ACTION_CHECK_TODO_LIST:
-		ret = check_todo_list_from_file(the_repository);
-		break;
-	case ACTION_REARRANGE_SQUASH:
-		ret = rearrange_squash_in_todo_file();
-		break;
-	case ACTION_ADD_EXEC: {
-		struct string_list commands = STRING_LIST_INIT_DUP;
-
-		split_exec_commands(opts.cmd, &commands);
-		ret = add_exec_commands(&commands);
-		string_list_clear(&commands, 0);
-		break;
-	}
-	default:
-		BUG("invalid command '%d'", command);
-	}
-
-	return !!ret;
+	return !!run_rebase_interactive(&opts, command);
 }
 
 static int use_builtin_rebase(void)
@@ -1072,7 +1081,7 @@ static int run_am(struct rebase_options *opts)
 	return status;
 }
 
-static int run_specific_rebase(struct rebase_options *opts)
+static int run_specific_rebase(struct rebase_options *opts, enum action action)
 {
 	const char *argv[] = { NULL, NULL };
 	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
@@ -1081,77 +1090,19 @@ static int run_specific_rebase(struct rebase_options *opts)
 
 	if (opts->type == REBASE_INTERACTIVE) {
 		/* Run builtin interactive rebase */
-		struct child_process child = CHILD_PROCESS_INIT;
-
-		argv_array_pushf(&child.env_array, "GIT_CHERRY_PICK_HELP=%s",
-				 resolvemsg);
+		setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
 		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
-			argv_array_push(&child.env_array,
-					"GIT_SEQUENCE_EDITOR=:");
+			setenv("GIT_SEQUENCE_EDITOR", ":", 1);
 			opts->autosquash = 0;
 		}
+		if (opts->gpg_sign_opt) {
+			/* remove the leading "-S" */
+			char *tmp = xstrdup(opts->gpg_sign_opt + 2);
+			free(opts->gpg_sign_opt);
+			opts->gpg_sign_opt = tmp;
+		}
 
-		child.git_cmd = 1;
-		argv_array_push(&child.args, "rebase--interactive");
-
-		if (opts->action)
-			argv_array_pushf(&child.args, "--%s", opts->action);
-		if (opts->keep_empty)
-			argv_array_push(&child.args, "--keep-empty");
-		if (opts->rebase_merges)
-			argv_array_push(&child.args, "--rebase-merges");
-		if (opts->rebase_cousins)
-			argv_array_push(&child.args, "--rebase-cousins");
-		if (opts->autosquash)
-			argv_array_push(&child.args, "--autosquash");
-		if (opts->flags & REBASE_VERBOSE)
-			argv_array_push(&child.args, "--verbose");
-		if (opts->flags & REBASE_FORCE)
-			argv_array_push(&child.args, "--no-ff");
-		if (opts->restrict_revision)
-			argv_array_pushf(&child.args,
-					 "--restrict-revision=^%s",
-					 oid_to_hex(&opts->restrict_revision->object.oid));
-		if (opts->upstream)
-			argv_array_pushf(&child.args, "--upstream=%s",
-					 oid_to_hex(&opts->upstream->object.oid));
-		if (opts->onto)
-			argv_array_pushf(&child.args, "--onto=%s",
-					 oid_to_hex(&opts->onto->object.oid));
-		if (opts->squash_onto)
-			argv_array_pushf(&child.args, "--squash-onto=%s",
-					 oid_to_hex(opts->squash_onto));
-		if (opts->onto_name)
-			argv_array_pushf(&child.args, "--onto-name=%s",
-					 opts->onto_name);
-		argv_array_pushf(&child.args, "--head-name=%s",
-				 opts->head_name ?
-				 opts->head_name : "detached HEAD");
-		if (opts->strategy)
-			argv_array_pushf(&child.args, "--strategy=%s",
-					 opts->strategy);
-		if (opts->strategy_opts)
-			argv_array_pushf(&child.args, "--strategy-opts=%s",
-					 opts->strategy_opts);
-		if (opts->switch_to)
-			argv_array_pushf(&child.args, "--switch-to=%s",
-					 opts->switch_to);
-		if (opts->cmd)
-			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
-		if (opts->allow_empty_message)
-			argv_array_push(&child.args, "--allow-empty-message");
-		if (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)
-			argv_array_push(&child.args, "--rerere-autoupdate");
-		else if (opts->allow_rerere_autoupdate == RERERE_NOAUTOUPDATE)
-			argv_array_push(&child.args, "--no-rerere-autoupdate");
-		if (opts->gpg_sign_opt)
-			argv_array_push(&child.args, opts->gpg_sign_opt);
-		if (opts->signoff)
-			argv_array_push(&child.args, "--signoff");
-		if (opts->reschedule_failed_exec)
-			argv_array_push(&child.args, "--reschedule-failed-exec");
-
-		status = run_command(&child);
+		status = run_rebase_interactive(opts, action);
 		goto finished_rebase;
 	}
 
@@ -2212,7 +2163,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = !!run_specific_rebase(&options);
+	ret = !!run_specific_rebase(&options, action);
 
 cleanup:
 	strbuf_release(&revisions);
-- 
2.21.0
Josh Steadmon· Mar 20, 2019, 20:50 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 00/11] rebase -i run without forking rebase--interactive

On 2019.03.19 19:03, Phillip Wood wrote:
Show 30 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When the builtin rebase starts an interactive rebase it parses the
> options and then repackages them and forks `rebase--interactive`. This
> series refactors rebase--interactive so that interactive rebases can
> be started by the builtin rebase without forking. My motivation was to
> make it easier to debug the sequencer but this should help future
> maintainability.
> 
> This series involves some code movement so viewing the diffs with
> --color-moved is recommended.
> 
> These patches are based on a merge of master [e902e9bcae ("The second
> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
> can be fetched from the tag rebase-i-no-fork/rfc at
> https://github.com/phillipwood/git.git
> 
> Phillip Wood (11):
>   sequencer: always discard index after checkout
>   rebase: rename write_basic_state()
>   rebase: use OPT_RERERE_AUTOUPDATE()
>   rebase -i: combine rebase--interactive.c with rebase.c
>   rebase -i: remove duplication
>   rebase -i: use struct commit when parsing options
>   rebase -i: use struct object_id for squash_onto
>   rebase -i: use struct rebase_options to parse args
>   rebase -i: use struct rebase_options in do_interactive_rebase()
>   rebase: use a common action enum
>   rebase -i: run without forking rebase--interactive

Although I'm not very familiar with the rebase code, this series looks good to me.

Reviewed-by: Josh Steadmon <steadmon@google.com>
Ævar Arnfjörð Bjarmason· Mar 20, 2019, 23:05 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 00/11] rebase -i run without forking rebase--interactive

On Tue, Mar 19 2019, Phillip Wood wrote:
Show 17 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When the builtin rebase starts an interactive rebase it parses the
> options and then repackages them and forks `rebase--interactive`. This
> series refactors rebase--interactive so that interactive rebases can
> be started by the builtin rebase without forking. My motivation was to
> make it easier to debug the sequencer but this should help future
> maintainability.
>
> This series involves some code movement so viewing the diffs with
> --color-moved is recommended.
>
> These patches are based on a merge of master [e902e9bcae ("The second
> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
> can be fetched from the tag rebase-i-no-fork/rfc at
> https://github.com/phillipwood/git.git

Just a that the t/perf/*rebase* numbers look much better with this. I don't have these in front of me anymore, but over 10 runs with -O3 one of those long-runnings test was 30% faster.

Another one (rebase -i) went from 0.02 to 0.01 sec, with that short amount of time I wonder (but didn't dig) if the test itself is broken...

Phillip Wood· Mar 21, 2019, 14:40 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC PATCH 00/11] rebase -i run without forking rebase--interactive

On 20/03/2019 23:05, Ævar Arnfjörð Bjarmason wrote:
Show 24 quoted lines
> 
> On Tue, Mar 19 2019, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When the builtin rebase starts an interactive rebase it parses the
>> options and then repackages them and forks `rebase--interactive`. This
>> series refactors rebase--interactive so that interactive rebases can
>> be started by the builtin rebase without forking. My motivation was to
>> make it easier to debug the sequencer but this should help future
>> maintainability.
>>
>> This series involves some code movement so viewing the diffs with
>> --color-moved is recommended.
>>
>> These patches are based on a merge of master [e902e9bcae ("The second
>> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
>> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
>> can be fetched from the tag rebase-i-no-fork/rfc at
>> https://github.com/phillipwood/git.git
> 
> Just a that the t/perf/*rebase* numbers look much better with this. I
> don't have these in front of me anymore, but over 10 runs with -O3 one
> of those long-runnings test was 30% faster.

That's surprising I wouldn't expect that much change as the time taken to fork shouldn't be that significant compared to the overall running time. I'll make a note to have a look at those tests.

> Another one (rebase -i) went from 0.02 to 0.01 sec, with that short
> amount of time I wonder (but didn't dig) if the test itself is broken...
Yeah, that sounds suspicious
Thanks for sharing those results
Phillip
Junio C Hamano· Mar 21, 2019, 01:44 UTC · re: Phillip Wood · lore

Re: [RFC PATCH 00/11] rebase -i run without forking rebase--interactive

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 17 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When the builtin rebase starts an interactive rebase it parses the
> options and then repackages them and forks `rebase--interactive`. This
> series refactors rebase--interactive so that interactive rebases can
> be started by the builtin rebase without forking. My motivation was to
> make it easier to debug the sequencer but this should help future
> maintainability.
>
> This series involves some code movement so viewing the diffs with
> --color-moved is recommended.
>
> These patches are based on a merge of master [e902e9bcae ("The second
> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
> can be fetched from the tag rebase-i-no-fork/rfc at
> https://github.com/phillipwood/git.git
Exciting.
Show 23 quoted lines
>
> Phillip Wood (11):
>   sequencer: always discard index after checkout
>   rebase: rename write_basic_state()
>   rebase: use OPT_RERERE_AUTOUPDATE()
>   rebase -i: combine rebase--interactive.c with rebase.c
>   rebase -i: remove duplication
>   rebase -i: use struct commit when parsing options
>   rebase -i: use struct object_id for squash_onto
>   rebase -i: use struct rebase_options to parse args
>   rebase -i: use struct rebase_options in do_interactive_rebase()
>   rebase: use a common action enum
>   rebase -i: run without forking rebase--interactive
>
>  Makefile                      |   1 -
>  builtin/rebase--interactive.c | 377 --------------------
>  builtin/rebase.c              | 625 ++++++++++++++++++++++++++--------
>  parse-options-cb.c            |  34 ++
>  parse-options.h               |   4 +
>  sequencer.c                   |  42 ++-
>  sequencer.h                   |   7 +-
>  7 files changed, 556 insertions(+), 534 deletions(-)
>  delete mode 100644 builtin/rebase--interactive.c
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 00/12] Run rebase -i without forking rebase--interactive

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Thanks for the feedback on the last version. I've updated the final commit message to mention the speed up reported by Ævar. I've also removed the translation of the trace strings. Apart from that this series is unchanged. There shouldn't be any merge conflicts with the updated dl/merge-cleanup-scissors-fix I posted earlier. (There are conflicts with other series but they are already resolved in pu)

This series still based on a merge of master [e902e9bcae ("The second batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841 ("rebase--interactive: move transform_todo_file()", 2019-03-05). It can be fetched from the tag rebase-i-no-fork/v1 at https://github.com/phillipwood/git.git

Phillip Wood (12):
  sequencer: always discard index after checkout
  rebase: don't translate trace strings
  rebase: rename write_basic_state()
  rebase: use OPT_RERERE_AUTOUPDATE()
  rebase -i: combine rebase--interactive.c with rebase.c
  rebase -i: remove duplication
  rebase -i: use struct commit when parsing options
  rebase -i: use struct object_id for squash_onto
  rebase -i: use struct rebase_options to parse args
  rebase -i: use struct rebase_options in do_interactive_rebase()
  rebase: use a common action enum
  rebase -i: run without forking rebase--interactive
 Makefile                      |   1 -
 builtin/rebase--interactive.c | 377 --------------------
 builtin/rebase.c              | 624 ++++++++++++++++++++++++++--------
 parse-options-cb.c            |  34 ++
 parse-options.h               |   4 +
 sequencer.c                   |  42 ++-
 sequencer.h                   |   7 +-
 7 files changed, 555 insertions(+), 534 deletions(-)
 delete mode 100644 builtin/rebase--interactive.c
Range-diff to rfc
 1:  549d45dc63 =  1:  fe51202dfb sequencer: always discard index after checkout
 -:  ---------- >  2:  2598165c0f rebase: don't translate trace strings
 2:  d896a3c76f =  3:  48801ccdd5 rebase: rename write_basic_state()
 3:  253de33871 =  4:  66972f0f7d rebase: use OPT_RERERE_AUTOUPDATE()
 4:  0aaeaf7c09 =  5:  c18aa58305 rebase -i: combine rebase--interactive.c with rebase.c
 5:  ff539c37d1 =  6:  27982630d1 rebase -i: remove duplication
 6:  71559a38db =  7:  f28e8df52a rebase -i: use struct commit when parsing options
 7:  34c9e4ae96 =  8:  1ae13b5a7d rebase -i: use struct object_id for squash_onto
 8:  4fe81c9e81 !  9:  efcc32d8e3 rebase -i: use struct rebase_options to parse args
    @@ -65,9 +65,9 @@
     +{
     +	struct replay_opts replay = REPLAY_OPTS_INIT;
     +
    -+	sequencer_init_config(&replay);
    -+
     +	replay.action = REPLAY_INTERACTIVE_REBASE;
    ++	sequencer_init_config(&replay);
    ++
     +	replay.signoff = opts->signoff;
     +	replay.allow_ff = !(opts->flags & REBASE_FORCE);
     +	if (opts->allow_rerere_autoupdate)
 9:  09343e540f = 10:  c89d815ad4 rebase -i: use struct rebase_options in do_interactive_rebase()
10:  9ad1482d7f ! 11:  fda591f006 rebase: use a common action enum
    @@ -38,14 +30,13 @@
     +	ACTION_ADD_EXEC
     +};
     +
    -+static const char *action_names[] = { N_("undefined"),
    -+				      N_("continue"),
    -+				      N_("skip"),
    -+				      N_("abort"),
    -+				      N_("quit"),
    -+				      N_("edit_todo"),
    -+				      N_("show_current_patch"),
    -+				      NULL };
    ++static const char *action_names[] = { "undefined",
    ++				      "continue",
    ++				      "skip",
    ++				      "abort",
    ++				      "quit",
    ++				      "edit_todo",
    ++				      "show_current_patch" };
     +
      static int add_exec_commands(struct string_list *commands)
      {
    @@ -173,14 +164,13 @@
     -		ACTION_EDIT_TODO,
     -		ACTION_SHOW_CURRENT_PATCH,
     -	} action = NO_ACTION;
    --	static const char *action_names[] = { N_("undefined"),
    --					      N_("continue"),
    --					      N_("skip"),
    --					      N_("abort"),
    --					      N_("quit"),
    --					      N_("edit_todo"),
    --					      N_("show_current_patch"),
    --					      NULL };
    +-	static const char *action_names[] = { "undefined",
    +-					      "continue",
    +-					      "skip",
    +-					      "abort",
    +-					      "quit",
    +-					      "edit_todo",
    +-					      "show_current_patch" };
     +	enum action action = ACTION_NONE;
      	const char *gpg_sign = NULL;
      	struct string_list exec = STRING_LIST_INIT_NODUP;
11:  ebfd7f80d9 ! 12:  d921226d32 rebase -i: run without forking rebase--interactive
    @@ -7,10 +7,18 @@
         `rebase--interactive`. Separate the option parsing in
         cmd_rebase__interactive() from the business logic to allow interactive
         rebases can be run without forking `rebase__interactive` by calling
    -    run_rebase_interactive() directly. This makes it easy to debug the
    -    sequencer without worrying about attaching to child processes. It also
    -    makes it easy to remove cmd_rebase__interactive() in the future when
    -    git-legacy-rebase.sh and git-rebase--preserve-merges.sh are retired.
    +    run_rebase_interactive() directly.
    +
    +    Starting interactive rebases without forking makes it easy to debug
    +    the sequencer without worrying about attaching to child
    +    processes. Ævar has also reported that some of the rebase perf tests
    +    are 30% faster [1].
    +
    +    This patch also makes it easy to remove cmd_rebase__interactive() in
    +    the future when git-legacy-rebase.sh and
    +    git-rebase--preserve-merges.sh are retired.
    +
    +    [1] https://public-inbox.org/git/87y359cfjj.fsf@evledraar.gmail.com/
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>

-- 2.21.0

Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 01/12] sequencer: always discard index after checkout

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As the checkout runs in a separate process our index will be out of date so it should be discarded. The existing callers are not doing this consistently so do it here to avoid the callers having to worry about it.

This fixes some test failures that happen if do_interactive_rebase() is called without forking rebase--interactive which we will implement shortly. Running

  git rebase -i master topic

starting on master created empty todo lists because all the commits in topic were marked as cherry-picks. After topic was checked out in prepare_branch_to_be_rebased() the working tree contained the contents from topic but the index contained master and the cache entries were still valid. This meant that diff_populate_filespec() which loads the blobs when calculating patch-id's ended up reading the contents for master from the working tree which actually contained topic.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase--interactive.c |  2 +-
 sequencer.c                   | 27 +++++++++++++++++----------
 sequencer.h                   |  3 ++-
 3 files changed, 20 insertions(+), 12 deletions(-)
Show changes to 3 files +20 −12

builtin/rebase--interactive.c, sequencer.c, sequencer.h

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 4535523bf5..d1a4ac1b84 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -171,7 +171,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 	struct argv_array make_script_args = ARGV_ARRAY_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
 
-	if (prepare_branch_to_be_rebased(opts, switch_to))
+	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
 		return -1;
 
 	if (get_revision_ranges(upstream, onto, &head_hash,
diff --git a/sequencer.c b/sequencer.c
index 281a8ade19..ccc0160800 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3418,10 +3418,11 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
-static int run_git_checkout(struct replay_opts *opts, const char *commit,
-			    const char *action)
+static int run_git_checkout(struct repository *r, struct replay_opts *opts,
+			    const char *commit, const char *action)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	int ret;
 
 	cmd.git_cmd = 1;
 
@@ -3430,25 +3431,31 @@ static int run_git_checkout(struct replay_opts *opts, const char *commit,
 	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
 
 	if (opts->verbose)
-		return run_command(&cmd);
+		ret = run_command(&cmd);
 	else
-		return run_command_silent_on_success(&cmd);
+		ret = run_command_silent_on_success(&cmd);
+
+	if (!ret)
+		discard_index(r->index);
+
+	return ret;
 }
 
-int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit)
+int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
+				 const char *commit)
 {
 	const char *action;
 
 	if (commit && *commit) {
 		action = reflog_message(opts, "start", "checkout %s", commit);
-		if (run_git_checkout(opts, commit, action))
+		if (run_git_checkout(r, opts, commit, action))
 			return error(_("could not checkout %s"), commit);
 	}
 
 	return 0;
 }
 
-static int checkout_onto(struct replay_opts *opts,
+static int checkout_onto(struct repository *r, struct replay_opts *opts,
 			 const char *onto_name, const char *onto,
 			 const char *orig_head)
 {
@@ -3458,7 +3465,7 @@ static int checkout_onto(struct replay_opts *opts,
 	if (get_oid(orig_head, &oid))
 		return error(_("%s: not a valid OID"), orig_head);
 
-	if (run_git_checkout(opts, onto, action)) {
+	if (run_git_checkout(r, opts, onto, action)) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
 		return error(_("could not detach HEAD"));
@@ -4786,7 +4793,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
 	    todo_list_check(todo_list, &new_todo)) {
 		fprintf(stderr, _(edit_todo_list_advice));
-		checkout_onto(opts, onto_name, onto, orig_head);
+		checkout_onto(r, opts, onto_name, onto, orig_head);
 		todo_list_release(&new_todo);
 
 		return -1;
@@ -4805,7 +4812,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 
 	todo_list_release(&new_todo);
 
-	if (checkout_onto(opts, onto_name, oid_to_hex(&oid), orig_head))
+	if (checkout_onto(r, opts, onto_name, oid_to_hex(&oid), orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
diff --git a/sequencer.h b/sequencer.h
index a515ee4457..6c55aa4200 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -175,7 +175,8 @@ void commit_post_rewrite(struct repository *r,
 			 const struct commit *current_head,
 			 const struct object_id *new_head);
 
-int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit);
+int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
+				 const char *commit);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 03/12] rebase: rename write_basic_state()

From: Phillip Wood <phillip.wood@dunelm.org.uk>
This clashes with a function in sequencer.c
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/rebase.c +2 −2
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 239a54ecfe..0b1a193a8e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -245,7 +245,7 @@ static int read_basic_state(struct rebase_options *opts)
 	return 0;
 }
 
-static int write_basic_state(struct rebase_options *opts)
+static int rebase_write_basic_state(struct rebase_options *opts)
 {
 	write_file(state_dir_path("head-name", opts), "%s",
 		   opts->head_name ? opts->head_name : "detached HEAD");
@@ -640,7 +640,7 @@ static int run_am(struct rebase_options *opts)
 	}
 
 	if (is_directory(opts->state_dir))
-		write_basic_state(opts);
+		rebase_write_basic_state(opts);
 
 	return status;
 }
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 04/12] rebase: use OPT_RERERE_AUTOUPDATE()

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As we have a macro for this it makes sense to use it. Having cmd_rebase() and cmd_rebase__interactive() use the same values for this option will be helpful when we start running interactive rebases without forking rebase--interactive.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)
Show changes to builtin/rebase.c +13 −17
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0b1a193a8e..e7b8122003 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -206,14 +206,13 @@ static int read_basic_state(struct rebase_options *opts)
 			    &buf))
 			return -1;
 		if (!strcmp(buf.buf, "--rerere-autoupdate"))
-			opts->allow_rerere_autoupdate = 1;
+			opts->allow_rerere_autoupdate = RERERE_AUTOUPDATE;
 		else if (!strcmp(buf.buf, "--no-rerere-autoupdate"))
-			opts->allow_rerere_autoupdate = 0;
+			opts->allow_rerere_autoupdate = RERERE_NOAUTOUPDATE;
 		else
 			warning(_("ignoring invalid allow_rerere_autoupdate: "
 				  "'%s'"), buf.buf);
-	} else
-		opts->allow_rerere_autoupdate = -1;
+	}
 
 	if (file_exists(state_dir_path("gpg_sign_opt", opts))) {
 		strbuf_reset(&buf);
@@ -263,10 +262,11 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	if (opts->strategy_opts)
 		write_file(state_dir_path("strategy_opts", opts), "%s",
 			   opts->strategy_opts);
-	if (opts->allow_rerere_autoupdate >= 0)
+	if (opts->allow_rerere_autoupdate > 0)
 		write_file(state_dir_path("allow_rerere_autoupdate", opts),
 			   "-%s-rerere-autoupdate",
-			   opts->allow_rerere_autoupdate ? "" : "-no");
+			   opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?
+				"" : "-no");
 	if (opts->gpg_sign_opt)
 		write_file(state_dir_path("gpg_sign_opt", opts), "%s",
 			   opts->gpg_sign_opt);
@@ -625,9 +625,9 @@ static int run_am(struct rebase_options *opts)
 	argv_array_push(&am.args, "--rebasing");
 	argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 	argv_array_push(&am.args, "--patch-format=mboxrd");
-	if (opts->allow_rerere_autoupdate > 0)
+	if (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)
 		argv_array_push(&am.args, "--rerere-autoupdate");
-	else if (opts->allow_rerere_autoupdate == 0)
+	else if (opts->allow_rerere_autoupdate == RERERE_NOAUTOUPDATE)
 		argv_array_push(&am.args, "--no-rerere-autoupdate");
 	if (opts->gpg_sign_opt)
 		argv_array_push(&am.args, opts->gpg_sign_opt);
@@ -713,9 +713,9 @@ static int run_specific_rebase(struct rebase_options *opts)
 			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
 		if (opts->allow_empty_message)
 			argv_array_push(&child.args, "--allow-empty-message");
-		if (opts->allow_rerere_autoupdate > 0)
+		if (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)
 			argv_array_push(&child.args, "--rerere-autoupdate");
-		else if (opts->allow_rerere_autoupdate == 0)
+		else if (opts->allow_rerere_autoupdate == RERERE_NOAUTOUPDATE)
 			argv_array_push(&child.args, "--no-rerere-autoupdate");
 		if (opts->gpg_sign_opt)
 			argv_array_push(&child.args, opts->gpg_sign_opt);
@@ -764,9 +764,9 @@ static int run_specific_rebase(struct rebase_options *opts)
 	add_var(&script_snippet, "action", opts->action ? opts->action : "");
 	add_var(&script_snippet, "signoff", opts->signoff ? "--signoff" : "");
 	add_var(&script_snippet, "allow_rerere_autoupdate",
-		opts->allow_rerere_autoupdate < 0 ? "" :
 		opts->allow_rerere_autoupdate ?
-		"--rerere-autoupdate" : "--no-rerere-autoupdate");
+			opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?
+			"--rerere-autoupdate" : "--no-rerere-autoupdate" : "");
 	add_var(&script_snippet, "keep_empty", opts->keep_empty ? "yes" : "");
 	add_var(&script_snippet, "autosquash", opts->autosquash ? "t" : "");
 	add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt);
@@ -1007,7 +1007,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		.type = REBASE_UNSPECIFIED,
 		.flags = REBASE_NO_QUIET,
 		.git_am_opts = ARGV_ARRAY_INIT,
-		.allow_rerere_autoupdate  = -1,
 		.allow_empty_message = 1,
 		.git_format_patch_opt = STRBUF_INIT,
 	};
@@ -1101,10 +1100,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('p', "preserve-merges", &options.type,
 			    N_("try to recreate merges instead of ignoring "
 			       "them"), REBASE_PRESERVE_MERGES),
-		OPT_BOOL(0, "rerere-autoupdate",
-			 &options.allow_rerere_autoupdate,
-			 N_("allow rerere to update index with resolved "
-			    "conflict")),
+		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
 		OPT_BOOL('k', "keep-empty", &options.keep_empty,
 			 N_("preserve empty commits during rebase")),
 		OPT_BOOL(0, "autosquash", &options.autosquash,
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 02/12] rebase: don't translate trace strings

From: Phillip Wood <phillip.wood@dunelm.org.uk>

commit b3a5d5a80c ("trace2:data: add subverb for rebase", 2019-02-22) mistakenly marked the subverb names for translation and unnecessarily NULL terminated the array.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
Show changes to builtin/rebase.c +7 −8
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 52114cbf0d..239a54ecfe 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1027,14 +1027,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		ACTION_EDIT_TODO,
 		ACTION_SHOW_CURRENT_PATCH,
 	} action = NO_ACTION;
-	static const char *action_names[] = { N_("undefined"),
-					      N_("continue"),
-					      N_("skip"),
-					      N_("abort"),
-					      N_("quit"),
-					      N_("edit_todo"),
-					      N_("show_current_patch"),
-					      NULL };
+	static const char *action_names[] = { "undefined",
+					      "continue",
+					      "skip",
+					      "abort",
+					      "quit",
+					      "edit_todo",
+					      "show_current_patch" };
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
-- 
2.21.0
Junio C Hamano· Apr 19, 2019, 05:53 UTC · re: Phillip Wood · lore

Re: [PATCH v1 02/12] rebase: don't translate trace strings

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 34 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> commit b3a5d5a80c ("trace2:data: add subverb for rebase", 2019-02-22)
> mistakenly marked the subverb names for translation and unnecessarily
> NULL terminated the array.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 52114cbf0d..239a54ecfe 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1027,14 +1027,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		ACTION_EDIT_TODO,
>  		ACTION_SHOW_CURRENT_PATCH,
>  	} action = NO_ACTION;
> -	static const char *action_names[] = { N_("undefined"),
> -					      N_("continue"),
> -					      N_("skip"),
> -					      N_("abort"),
> -					      N_("quit"),
> -					      N_("edit_todo"),
> -					      N_("show_current_patch"),
> -					      NULL };
> +	static const char *action_names[] = { "undefined",
> +					      "continue",
> +					      "skip",
> +					      "abort",
> +					      "quit",
> +					      "edit_todo",
> +					      "show_current_patch" };
That's an improvement independent from the rest of the patches.

Now we've had the C99 designated initialisers weather balloon changes for some time in our codebase, perhaps we can ensure that these entries match the intended & corresponding "enum action" constants? If we can also ensure that the array is large enough so that the trace2 call done like so

	trace2_cmd_mode(action_names[action])
is safe, that would be good, but that is secondary.
Thanks.
Phillip Wood· Apr 25, 2019, 17:47 UTC · re: Junio C Hamano · lore

Re: [PATCH v1 02/12] rebase: don't translate trace strings

On 19/04/2019 06:53, Junio C Hamano wrote:
Show 38 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> commit b3a5d5a80c ("trace2:data: add subverb for rebase", 2019-02-22)
>> mistakenly marked the subverb names for translation and unnecessarily
>> NULL terminated the array.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>  builtin/rebase.c | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 52114cbf0d..239a54ecfe 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1027,14 +1027,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>  		ACTION_EDIT_TODO,
>>  		ACTION_SHOW_CURRENT_PATCH,
>>  	} action = NO_ACTION;
>> -	static const char *action_names[] = { N_("undefined"),
>> -					      N_("continue"),
>> -					      N_("skip"),
>> -					      N_("abort"),
>> -					      N_("quit"),
>> -					      N_("edit_todo"),
>> -					      N_("show_current_patch"),
>> -					      NULL };
>> +	static const char *action_names[] = { "undefined",
>> +					      "continue",
>> +					      "skip",
>> +					      "abort",
>> +					      "quit",
>> +					      "edit_todo",
>> +					      "show_current_patch" };
> 
> That's an improvement independent from the rest of the patches.
Yes I only included it as I move the definition later in the series
Show 11 quoted lines
> Now we've had the C99 designated initialisers weather balloon
> changes for some time in our codebase, perhaps we can ensure that
> these entries match the intended & corresponding "enum action"
> constants?  If we can also ensure that the array is large enough so
> that the trace2 call done like so
> 
> 	trace2_cmd_mode(action_names[action])
> 
> is safe, that would be good, but that is secondary.
> 
> Thanks.

If what's below is ok, I'll send a re-roll, I wasn't sure if it was best to die if action is larger than the array of names or just use a default. My worrying with dying is that it wont be caught by tests and will cause a problem for users who enable tracing. At least with what's below they can still rebase and hopefully report a bug about unknown action in their trace output.

Best Wishes
Phillip
Show changes to builtin/rebase.c +12 −9
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 52114cbf0d..3f56be230e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1027,14 +1027,15 @@ int cmd_rebase(int argc, const char **argv,
const char *prefix)
                ACTION_EDIT_TODO,
                ACTION_SHOW_CURRENT_PATCH,
        } action = NO_ACTION;
-       static const char *action_names[] = { N_("undefined"),
-                                             N_("continue"),
-                                             N_("skip"),
-                                             N_("abort"),
-                                             N_("quit"),
-                                             N_("edit_todo"),
-                                             N_("show_current_patch"),
-                                             NULL };
+       static const char *action_names[] = {
+               [NO_ACTION] = "undefined",
+               [ACTION_CONTINUE] = "continue",
+               [ACTION_SKIP] = "skip",
+               [ACTION_ABORT] = "abort",
+               [ACTION_QUIT] = "quit",
+               [ACTION_EDIT_TODO] = "edit_todo",
+               [ACTION_SHOW_CURRENT_PATCH] = "show_current_patch"
+       };
        const char *gpg_sign = NULL;
        struct string_list exec = STRING_LIST_INIT_NODUP;
        const char *rebase_merges = NULL;
@@ -1225,8 +1226,10 @@ int cmd_rebase(int argc, const char **argv, const
char *prefix)
                        trace2_cmd_mode("interactive");
                else if (exec.nr)
                        trace2_cmd_mode("interactive-exec");
-               else
+               else if (action < ARRAY_SIZE(action_names))
                        trace2_cmd_mode(action_names[action]);
+               else
+                       trace2_cmd_mode("unknown rebase action");
        }

        switch (action) {
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 05/12] rebase -i: combine rebase--interactive.c with rebase.c

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In order to run `rebase -i` without forking `rebase--interactive` it will be convenient to have all the code from rebase--interactive.c in rebase.c. This is a straight forward copy of the code from rebase--interactive.c, it will be simplified slightly in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Makefile                      |   1 -
 builtin/rebase--interactive.c | 377 ----------------------------------
 builtin/rebase.c              | 367 +++++++++++++++++++++++++++++++++
 3 files changed, 367 insertions(+), 378 deletions(-)
 delete mode 100644 builtin/rebase--interactive.c
Show changes to 3 files +367 −378

Makefile, builtin/rebase--interactive.c, builtin/rebase.c

diff --git a/Makefile b/Makefile
index 537493822b..c644089244 100644
--- a/Makefile
+++ b/Makefile
@@ -1130,7 +1130,6 @@ BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/range-diff.o
 BUILTIN_OBJS += builtin/read-tree.o
 BUILTIN_OBJS += builtin/rebase.o
-BUILTIN_OBJS += builtin/rebase--interactive.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
deleted file mode 100644
index d1a4ac1b84..0000000000
--- a/builtin/rebase--interactive.c
+++ /dev/null
@@ -1,377 +0,0 @@
-#define USE_THE_INDEX_COMPATIBILITY_MACROS
-#include "builtin.h"
-#include "cache.h"
-#include "config.h"
-#include "parse-options.h"
-#include "sequencer.h"
-#include "rebase-interactive.h"
-#include "argv-array.h"
-#include "refs.h"
-#include "rerere.h"
-#include "run-command.h"
-
-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 add_exec_commands(struct string_list *commands)
-{
-	const char *todo_file = rebase_path_todo();
-	struct todo_list todo_list = TODO_LIST_INIT;
-	int res;
-
-	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
-		return error_errno(_("could not read '%s'."), todo_file);
-
-	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-					&todo_list)) {
-		todo_list_release(&todo_list);
-		return error(_("unusable todo list: '%s'"), todo_file);
-	}
-
-	todo_list_add_exec_commands(&todo_list, commands);
-	res = todo_list_write_to_file(the_repository, &todo_list,
-				      todo_file, NULL, NULL, -1, 0);
-	todo_list_release(&todo_list);
-
-	if (res)
-		return error_errno(_("could not write '%s'."), todo_file);
-	return 0;
-}
-
-static int rearrange_squash_in_todo_file(void)
-{
-	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);
-	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-					&todo_list)) {
-		todo_list_release(&todo_list);
-		return error(_("unusable todo list: '%s'"), todo_file);
-	}
-
-	res = todo_list_rearrange_squash(&todo_list);
-	if (!res)
-		res = todo_list_write_to_file(the_repository, &todo_list,
-					      todo_file, NULL, NULL, -1, 0);
-
-	todo_list_release(&todo_list);
-
-	if (res)
-		return error_errno(_("could not write '%s'."), todo_file);
-	return 0;
-}
-
-static int transform_todo_file(unsigned flags)
-{
-	const char *todo_file = rebase_path_todo();
-	struct todo_list todo_list = TODO_LIST_INIT;
-	int res;
-
-	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
-		return error_errno(_("could not read '%s'."), todo_file);
-
-	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-					&todo_list)) {
-		todo_list_release(&todo_list);
-		return error(_("unusable todo list: '%s'"), todo_file);
-	}
-
-	res = todo_list_write_to_file(the_repository, &todo_list, todo_file,
-				      NULL, NULL, -1, flags);
-	todo_list_release(&todo_list);
-
-	if (res)
-		return error_errno(_("could not write '%s'."), todo_file);
-	return 0;
-}
-
-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)
-{
-	const char *base_rev = upstream ? upstream : onto, *shorthead;
-	struct object_id orig_head;
-
-	if (get_oid("HEAD", &orig_head))
-		return error(_("no HEAD?"));
-
-	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
-	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
-
-	shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);
-
-	if (upstream) {
-		const char *shortrev;
-		struct object_id rev_oid;
-
-		get_oid(base_rev, &rev_oid);
-		shortrev = find_unique_abbrev(&rev_oid, DEFAULT_ABBREV);
-
-		*shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
-	} else
-		*shortrevisions = xstrdup(shorthead);
-
-	return 0;
-}
-
-static int init_basic_state(struct replay_opts *opts, const char *head_name,
-			    const char *onto, const char *orig_head)
-{
-	FILE *interactive;
-
-	if (!is_directory(path_state_dir()) && mkdir_in_gitdir(path_state_dir()))
-		return error_errno(_("could not create temporary %s"), path_state_dir());
-
-	delete_reflog("REBASE_HEAD");
-
-	interactive = fopen(path_interactive(), "w");
-	if (!interactive)
-		return error_errno(_("could not mark as interactive"));
-	fclose(interactive);
-
-	return write_basic_state(opts, head_name, onto, orig_head);
-}
-
-static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
-				 const char *switch_to, const char *upstream,
-				 const char *onto, const char *onto_name,
-				 const char *squash_onto, const char *head_name,
-				 const char *restrict_revision, char *raw_strategies,
-				 struct string_list *commands, unsigned autosquash)
-{
-	int ret;
-	const char *head_hash = NULL;
-	char *revisions = NULL, *shortrevisions = NULL;
-	struct argv_array make_script_args = ARGV_ARRAY_INIT;
-	struct todo_list todo_list = TODO_LIST_INIT;
-
-	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
-		return -1;
-
-	if (get_revision_ranges(upstream, onto, &head_hash,
-				&revisions, &shortrevisions))
-		return -1;
-
-	if (raw_strategies)
-		parse_strategy_opts(opts, raw_strategies);
-
-	if (init_basic_state(opts, head_name, onto, head_hash)) {
-		free(revisions);
-		free(shortrevisions);
-
-		return -1;
-	}
-
-	if (!upstream && squash_onto)
-		write_file(path_squash_onto(), "%s\n", squash_onto);
-
-	argv_array_pushl(&make_script_args, "", revisions, NULL);
-	if (restrict_revision)
-		argv_array_push(&make_script_args, restrict_revision);
-
-	ret = sequencer_make_script(the_repository, &todo_list.buf,
-				    make_script_args.argc, make_script_args.argv,
-				    flags);
-
-	if (ret)
-		error(_("could not generate todo list"));
-	else {
-		discard_cache();
-		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
-						&todo_list))
-			BUG("unusable todo list");
-
-		ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name,
-				      onto, head_hash, commands, autosquash, &todo_list);
-	}
-
-	free(revisions);
-	free(shortrevisions);
-	todo_list_release(&todo_list);
-	argv_array_clear(&make_script_args);
-
-	return ret;
-}
-
-static const char * const builtin_rebase_interactive_usage[] = {
-	N_("git rebase--interactive [<options>]"),
-	NULL
-};
-
-int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
-{
-	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
-	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
-		*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
-		*switch_to = NULL, *cmd = NULL;
-	struct string_list commands = STRING_LIST_INIT_DUP;
-	char *raw_strategies = NULL;
-	enum {
-		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
-		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
-	} command = 0;
-	struct option options[] = {
-		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
-		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
-		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
-			 N_("allow commits with empty messages")),
-		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
-		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
-			 N_("keep original branch points of cousins")),
-		OPT_BOOL(0, "autosquash", &autosquash,
-			 N_("move commits that begin with squash!/fixup!")),
-		OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")),
-		OPT__VERBOSE(&opts.verbose, N_("be verbose")),
-		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
-			    CONTINUE),
-		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
-		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
-			    EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
-			    SHOW_CURRENT_PATCH),
-		OPT_CMDMODE(0, "shorten-ids", &command,
-			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
-		OPT_CMDMODE(0, "expand-ids", &command,
-			N_("expand commit ids in the todo list"), EXPAND_OIDS),
-		OPT_CMDMODE(0, "check-todo-list", &command,
-			N_("check the todo list"), CHECK_TODO_LIST),
-		OPT_CMDMODE(0, "rearrange-squash", &command,
-			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
-		OPT_CMDMODE(0, "add-exec-commands", &command,
-			N_("insert exec commands in todo list"), ADD_EXEC),
-		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
-		OPT_STRING(0, "restrict-revision", &restrict_revision,
-			   N_("restrict-revision"), N_("restrict revision")),
-		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
-			   N_("squash onto")),
-		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
-			   N_("the upstream commit")),
-		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
-		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
-			N_("GPG-sign commits"),
-			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
-		OPT_STRING(0, "strategy", &opts.strategy, N_("strategy"),
-			   N_("rebase strategy")),
-		OPT_STRING(0, "strategy-opts", &raw_strategies, N_("strategy-opts"),
-			   N_("strategy options")),
-		OPT_STRING(0, "switch-to", &switch_to, N_("switch-to"),
-			   N_("the branch or commit to checkout")),
-		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
-		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
-		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
-		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
-			 N_("automatically re-schedule any `exec` that fails")),
-		OPT_END()
-	};
-
-	sequencer_init_config(&opts);
-	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
-
-	opts.action = REPLAY_INTERACTIVE_REBASE;
-	opts.allow_ff = 1;
-	opts.allow_empty = 1;
-
-	if (argc == 1)
-		usage_with_options(builtin_rebase_interactive_usage, options);
-
-	argc = parse_options(argc, argv, NULL, options,
-			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
-
-	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
-
-	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
-	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
-	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
-	flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
-	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
-
-	if (rebase_cousins >= 0 && !rebase_merges)
-		warning(_("--[no-]rebase-cousins has no effect without "
-			  "--rebase-merges"));
-
-	if (cmd && *cmd) {
-		string_list_split(&commands, cmd, '\n', -1);
-
-		/* rebase.c adds a new line to cmd after every command,
-		 * so here the last command is always empty */
-		string_list_remove_empty_items(&commands, 0);
-	}
-
-	switch (command) {
-	case NONE:
-		if (!onto && !upstream)
-			die(_("a base commit must be provided with --upstream or --onto"));
-
-		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
-					    onto_name, squash_onto, head_name, restrict_revision,
-					    raw_strategies, &commands, autosquash);
-		break;
-	case SKIP: {
-		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-
-		rerere_clear(the_repository, &merge_rr);
-		/* fallthrough */
-	case CONTINUE:
-		ret = sequencer_continue(the_repository, &opts);
-		break;
-	}
-	case EDIT_TODO:
-		ret = edit_todo_file(flags);
-		break;
-	case SHOW_CURRENT_PATCH: {
-		struct child_process cmd = CHILD_PROCESS_INIT;
-
-		cmd.git_cmd = 1;
-		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
-		ret = run_command(&cmd);
-
-		break;
-	}
-	case SHORTEN_OIDS:
-	case EXPAND_OIDS:
-		ret = transform_todo_file(flags);
-		break;
-	case CHECK_TODO_LIST:
-		ret = check_todo_list_from_file(the_repository);
-		break;
-	case REARRANGE_SQUASH:
-		ret = rearrange_squash_in_todo_file();
-		break;
-	case ADD_EXEC:
-		ret = add_exec_commands(&commands);
-		break;
-	default:
-		BUG("invalid command '%d'", command);
-	}
-
-	string_list_clear(&commands, 0);
-	return !!ret;
-}
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e7b8122003..57a3c9caf1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -25,6 +25,8 @@
 #include "commit-reach.h"
 #include "rerere.h"
 #include "branch.h"
+#include "sequencer.h"
+#include "rebase-interactive.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
@@ -35,6 +37,9 @@ static char const * const builtin_rebase_usage[] = {
 	NULL
 };
 
+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 GIT_PATH_FUNC(apply_dir, "rebase-apply")
 static GIT_PATH_FUNC(merge_dir, "rebase-merge")
 
@@ -46,6 +51,368 @@ enum rebase_type {
 	REBASE_PRESERVE_MERGES
 };
 
+static int add_exec_commands(struct string_list *commands)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	int res;
+
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+		return error_errno(_("could not read '%s'."), todo_file);
+
+	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+					&todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	todo_list_add_exec_commands(&todo_list, commands);
+	res = todo_list_write_to_file(the_repository, &todo_list,
+				      todo_file, NULL, NULL, -1, 0);
+	todo_list_release(&todo_list);
+
+	if (res)
+		return error_errno(_("could not write '%s'."), todo_file);
+	return 0;
+}
+
+static int rearrange_squash_in_todo_file(void)
+{
+	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);
+	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+					&todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	res = todo_list_rearrange_squash(&todo_list);
+	if (!res)
+		res = todo_list_write_to_file(the_repository, &todo_list,
+					      todo_file, NULL, NULL, -1, 0);
+
+	todo_list_release(&todo_list);
+
+	if (res)
+		return error_errno(_("could not write '%s'."), todo_file);
+	return 0;
+}
+
+static int transform_todo_file(unsigned flags)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	int res;
+
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+		return error_errno(_("could not read '%s'."), todo_file);
+
+	if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+					&todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	res = todo_list_write_to_file(the_repository, &todo_list, todo_file,
+				      NULL, NULL, -1, flags);
+	todo_list_release(&todo_list);
+
+	if (res)
+		return error_errno(_("could not write '%s'."), todo_file);
+	return 0;
+}
+
+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)
+{
+	const char *base_rev = upstream ? upstream : onto, *shorthead;
+	struct object_id orig_head;
+
+	if (get_oid("HEAD", &orig_head))
+		return error(_("no HEAD?"));
+
+	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
+	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+	shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);
+
+	if (upstream) {
+		const char *shortrev;
+		struct object_id rev_oid;
+
+		get_oid(base_rev, &rev_oid);
+		shortrev = find_unique_abbrev(&rev_oid, DEFAULT_ABBREV);
+
+		*shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
+	} else
+		*shortrevisions = xstrdup(shorthead);
+
+	return 0;
+}
+
+static int init_basic_state(struct replay_opts *opts, const char *head_name,
+			    const char *onto, const char *orig_head)
+{
+	FILE *interactive;
+
+	if (!is_directory(path_state_dir()) && mkdir_in_gitdir(path_state_dir()))
+		return error_errno(_("could not create temporary %s"), path_state_dir());
+
+	delete_reflog("REBASE_HEAD");
+
+	interactive = fopen(path_interactive(), "w");
+	if (!interactive)
+		return error_errno(_("could not mark as interactive"));
+	fclose(interactive);
+
+	return write_basic_state(opts, head_name, onto, orig_head);
+}
+
+static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
+				 const char *switch_to, const char *upstream,
+				 const char *onto, const char *onto_name,
+				 const char *squash_onto, const char *head_name,
+				 const char *restrict_revision, char *raw_strategies,
+				 struct string_list *commands, unsigned autosquash)
+{
+	int ret;
+	const char *head_hash = NULL;
+	char *revisions = NULL, *shortrevisions = NULL;
+	struct argv_array make_script_args = ARGV_ARRAY_INIT;
+	struct todo_list todo_list = TODO_LIST_INIT;
+
+	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
+		return -1;
+
+	if (get_revision_ranges(upstream, onto, &head_hash,
+				&revisions, &shortrevisions))
+		return -1;
+
+	if (raw_strategies)
+		parse_strategy_opts(opts, raw_strategies);
+
+	if (init_basic_state(opts, head_name, onto, head_hash)) {
+		free(revisions);
+		free(shortrevisions);
+
+		return -1;
+	}
+
+	if (!upstream && squash_onto)
+		write_file(path_squash_onto(), "%s\n", squash_onto);
+
+	argv_array_pushl(&make_script_args, "", revisions, NULL);
+	if (restrict_revision)
+		argv_array_push(&make_script_args, restrict_revision);
+
+	ret = sequencer_make_script(the_repository, &todo_list.buf,
+				    make_script_args.argc, make_script_args.argv,
+				    flags);
+
+	if (ret)
+		error(_("could not generate todo list"));
+	else {
+		discard_cache();
+		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
+						&todo_list))
+			BUG("unusable todo list");
+
+		ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name,
+				      onto, head_hash, commands, autosquash, &todo_list);
+	}
+
+	free(revisions);
+	free(shortrevisions);
+	todo_list_release(&todo_list);
+	argv_array_clear(&make_script_args);
+
+	return ret;
+}
+
+static const char * const builtin_rebase_interactive_usage[] = {
+	N_("git rebase--interactive [<options>]"),
+	NULL
+};
+
+int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
+{
+	struct replay_opts opts = REPLAY_OPTS_INIT;
+	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
+	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
+	const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
+		*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
+		*switch_to = NULL, *cmd = NULL;
+	struct string_list commands = STRING_LIST_INIT_DUP;
+	char *raw_strategies = NULL;
+	enum {
+		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
+		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
+	} command = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
+		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
+		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
+			 N_("allow commits with empty messages")),
+		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
+		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
+			 N_("keep original branch points of cousins")),
+		OPT_BOOL(0, "autosquash", &autosquash,
+			 N_("move commits that begin with squash!/fixup!")),
+		OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")),
+		OPT__VERBOSE(&opts.verbose, N_("be verbose")),
+		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
+			    CONTINUE),
+		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
+		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
+			    EDIT_TODO),
+		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
+			    SHOW_CURRENT_PATCH),
+		OPT_CMDMODE(0, "shorten-ids", &command,
+			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
+		OPT_CMDMODE(0, "expand-ids", &command,
+			N_("expand commit ids in the todo list"), EXPAND_OIDS),
+		OPT_CMDMODE(0, "check-todo-list", &command,
+			N_("check the todo list"), CHECK_TODO_LIST),
+		OPT_CMDMODE(0, "rearrange-squash", &command,
+			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+		OPT_CMDMODE(0, "add-exec-commands", &command,
+			N_("insert exec commands in todo list"), ADD_EXEC),
+		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
+		OPT_STRING(0, "restrict-revision", &restrict_revision,
+			   N_("restrict-revision"), N_("restrict revision")),
+		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
+			   N_("squash onto")),
+		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
+			   N_("the upstream commit")),
+		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
+		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
+			N_("GPG-sign commits"),
+			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+		OPT_STRING(0, "strategy", &opts.strategy, N_("strategy"),
+			   N_("rebase strategy")),
+		OPT_STRING(0, "strategy-opts", &raw_strategies, N_("strategy-opts"),
+			   N_("strategy options")),
+		OPT_STRING(0, "switch-to", &switch_to, N_("switch-to"),
+			   N_("the branch or commit to checkout")),
+		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
+		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
+		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
+			 N_("automatically re-schedule any `exec` that fails")),
+		OPT_END()
+	};
+
+	sequencer_init_config(&opts);
+	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
+
+	opts.action = REPLAY_INTERACTIVE_REBASE;
+	opts.allow_ff = 1;
+	opts.allow_empty = 1;
+
+	if (argc == 1)
+		usage_with_options(builtin_rebase_interactive_usage, options);
+
+	argc = parse_options(argc, argv, NULL, options,
+			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
+
+	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
+
+	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
+	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
+	flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+
+	if (rebase_cousins >= 0 && !rebase_merges)
+		warning(_("--[no-]rebase-cousins has no effect without "
+			  "--rebase-merges"));
+
+	if (cmd && *cmd) {
+		string_list_split(&commands, cmd, '\n', -1);
+
+		/* rebase.c adds a new line to cmd after every command,
+		 * so here the last command is always empty */
+		string_list_remove_empty_items(&commands, 0);
+	}
+
+	switch (command) {
+	case NONE:
+		if (!onto && !upstream)
+			die(_("a base commit must be provided with --upstream or --onto"));
+
+		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
+					    onto_name, squash_onto, head_name, restrict_revision,
+					    raw_strategies, &commands, autosquash);
+		break;
+	case SKIP: {
+		struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+		rerere_clear(the_repository, &merge_rr);
+		/* fallthrough */
+	case CONTINUE:
+		ret = sequencer_continue(the_repository, &opts);
+		break;
+	}
+	case EDIT_TODO:
+		ret = edit_todo_file(flags);
+		break;
+	case SHOW_CURRENT_PATCH: {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
+		ret = run_command(&cmd);
+
+		break;
+	}
+	case SHORTEN_OIDS:
+	case EXPAND_OIDS:
+		ret = transform_todo_file(flags);
+		break;
+	case CHECK_TODO_LIST:
+		ret = check_todo_list_from_file(the_repository);
+		break;
+	case REARRANGE_SQUASH:
+		ret = rearrange_squash_in_todo_file();
+		break;
+	case ADD_EXEC:
+		ret = add_exec_commands(&commands);
+		break;
+	default:
+		BUG("invalid command '%d'", command);
+	}
+
+	string_list_clear(&commands, 0);
+	return !!ret;
+}
+
 static int use_builtin_rebase(void)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 06/12] rebase -i: remove duplication

From: Phillip Wood <phillip.wood@dunelm.org.uk>

path_state_dir() and merge_dir() refer to the same path so remove one of them.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to builtin/rebase.c +2 −3
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 57a3c9caf1..610b67827b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -37,7 +37,6 @@ static char const * const builtin_rebase_usage[] = {
 	NULL
 };
 
-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 GIT_PATH_FUNC(apply_dir, "rebase-apply")
@@ -182,8 +181,8 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 {
 	FILE *interactive;
 
-	if (!is_directory(path_state_dir()) && mkdir_in_gitdir(path_state_dir()))
-		return error_errno(_("could not create temporary %s"), path_state_dir());
+	if (!is_directory(merge_dir()) && mkdir_in_gitdir(merge_dir()))
+		return error_errno(_("could not create temporary %s"), merge_dir());
 
 	delete_reflog("REBASE_HEAD");
 
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 08/12] rebase -i: use struct object_id for squash_onto

From: Phillip Wood <phillip.wood@dunelm.org.uk>

More preparation for using `struct rebase_options` in cmd_rebase__interactive(). Using a string was a hangover from the scripted version of rebase, update the functions that use `squash_onto` to take a `sturct object_id`.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c   | 21 +++++++++++++--------
 parse-options-cb.c | 17 +++++++++++++++++
 parse-options.h    |  3 +++
 3 files changed, 33 insertions(+), 8 deletions(-)
Show changes to 3 files +33 −8

builtin/rebase.c, parse-options-cb.c, parse-options.h

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 06f6490ca3..3b9da69473 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -198,7 +198,7 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 				 const char *switch_to, struct commit *upstream,
 				 struct commit *onto, const char *onto_name,
-				 const char *squash_onto, const char *head_name,
+				 struct object_id *squash_onto, const char *head_name,
 				 struct commit *restrict_revision, char *raw_strategies,
 				 struct string_list *commands, unsigned autosquash)
 {
@@ -226,7 +226,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 	}
 
 	if (!upstream && squash_onto)
-		write_file(path_squash_onto(), "%s\n", squash_onto);
+		write_file(path_squash_onto(), "%s\n",
+			   oid_to_hex(squash_onto));
 
 	argv_array_pushl(&make_script_args, "", revisions, NULL);
 	if (restrict_revision)
@@ -267,10 +268,11 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto_name = NULL,
-		*squash_onto = NULL, *head_name = NULL,
-		*switch_to = NULL, *cmd = NULL;
+	const char *onto_name = NULL, *head_name = NULL, *switch_to = NULL,
+		*cmd = NULL;
 	struct commit *onto = NULL, *upstream = NULL, *restrict_revision = NULL;
+	struct object_id squash_onto = null_oid;
+	struct object_id *squash_onto_opt = NULL;
 	struct string_list commands = STRING_LIST_INIT_DUP;
 	char *raw_strategies = NULL;
 	enum {
@@ -311,8 +313,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "restrict-revision", &restrict_revision,
 		  N_("restrict-revision"), N_("restrict revision"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
-		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
-			   N_("squash onto")),
+		{ OPTION_CALLBACK, 0, "squash-onto", &squash_onto, N_("squash-onto"),
+		  N_("squash onto"), PARSE_OPT_NONEG, parse_opt_object_id, 0 },
 		{ OPTION_CALLBACK, 0, "upstream", &upstream, N_("upstream"),
 		  N_("the upstream commit"), PARSE_OPT_NONEG, parse_opt_commit,
 		  0 },
@@ -349,6 +351,9 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 
 	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
 
+	if (!is_null_oid(&squash_onto))
+		squash_onto_opt = &squash_onto;
+
 	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
@@ -373,7 +378,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
 		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
-					    onto_name, squash_onto, head_name, restrict_revision,
+					    onto_name, squash_onto_opt, head_name, restrict_revision,
 					    raw_strategies, &commands, autosquash);
 		break;
 	case SKIP: {
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 2206eb763c..28ad5cd94b 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -129,6 +129,23 @@ int parse_opt_object_name(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_object_id(const struct option *opt, const char *arg, int unset)
+{
+	struct object_id oid;
+	struct object_id *target = opt->value;
+
+	if (unset) {
+		*target = null_oid;
+		return 0;
+	}
+	if (!arg)
+		return -1;
+	if (get_oid(arg, &oid))
+		return error(_("malformed object name '%s'"), arg);
+	*target = oid;
+	return 0;
+}
+
 int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
 {
 	int *target = opt->value;
diff --git a/parse-options.h b/parse-options.h
index 5a75646618..0ab1103bc7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -264,7 +264,10 @@ int parse_opt_abbrev_cb(const struct option *, const char *, int);
 int parse_opt_expiry_date_cb(const struct option *, const char *, int);
 int parse_opt_color_flag_cb(const struct option *, const char *, int);
 int parse_opt_verbosity_cb(const struct option *, const char *, int);
+/* value is struct oid_array* */
 int parse_opt_object_name(const struct option *, const char *, int);
+/* value is struct object_id* */
+int parse_opt_object_id(const struct option *, const char *, int);
 int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 07/12] rebase -i: use struct commit when parsing options

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This is in preparation for using `struct rebase_options` when parsing options in cmd_rebase__interactive(). Using a string for onto, restrict_revision and upstream, was a hangover from the scripted version of rebase. The functions that use these variables are updated to take a `struct commit`.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c   | 42 ++++++++++++++++++++++++------------------
 parse-options-cb.c | 17 +++++++++++++++++
 parse-options.h    |  1 +
 sequencer.c        | 21 +++++++++++----------
 sequencer.h        |  4 ++--
 5 files changed, 55 insertions(+), 30 deletions(-)
Show changes to 5 files +55 −30

builtin/rebase.c, parse-options-cb.c, parse-options.h, sequencer.c, sequencer.h

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 610b67827b..06f6490ca3 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -147,27 +147,28 @@ static int edit_todo_file(unsigned flags)
 	return res;
 }
 
-static int get_revision_ranges(const char *upstream, const char *onto,
+static int get_revision_ranges(struct commit *upstream, struct commit *onto,
 			       const char **head_hash,
 			       char **revisions, char **shortrevisions)
 {
-	const char *base_rev = upstream ? upstream : onto, *shorthead;
+	struct commit *base_rev = upstream ? upstream : onto;
+	const char *shorthead;
 	struct object_id orig_head;
 
 	if (get_oid("HEAD", &orig_head))
 		return error(_("no HEAD?"));
 
 	*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);
-	*revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+	*revisions = xstrfmt("%s...%s", oid_to_hex(&base_rev->object.oid),
+						   *head_hash);
 
 	shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);
 
 	if (upstream) {
 		const char *shortrev;
-		struct object_id rev_oid;
 
-		get_oid(base_rev, &rev_oid);
-		shortrev = find_unique_abbrev(&rev_oid, DEFAULT_ABBREV);
+		shortrev = find_unique_abbrev(&base_rev->object.oid,
+					      DEFAULT_ABBREV);
 
 		*shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
 	} else
@@ -177,7 +178,7 @@ static int get_revision_ranges(const char *upstream, const char *onto,
 }
 
 static int init_basic_state(struct replay_opts *opts, const char *head_name,
-			    const char *onto, const char *orig_head)
+			    struct commit *onto, const char *orig_head)
 {
 	FILE *interactive;
 
@@ -195,10 +196,10 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 }
 
 static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
-				 const char *switch_to, const char *upstream,
-				 const char *onto, const char *onto_name,
+				 const char *switch_to, struct commit *upstream,
+				 struct commit *onto, const char *onto_name,
 				 const char *squash_onto, const char *head_name,
-				 const char *restrict_revision, char *raw_strategies,
+				 struct commit *restrict_revision, char *raw_strategies,
 				 struct string_list *commands, unsigned autosquash)
 {
 	int ret;
@@ -229,7 +230,8 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 
 	argv_array_pushl(&make_script_args, "", revisions, NULL);
 	if (restrict_revision)
-		argv_array_push(&make_script_args, restrict_revision);
+		argv_array_push(&make_script_args,
+				oid_to_hex(&restrict_revision->object.oid));
 
 	ret = sequencer_make_script(the_repository, &todo_list.buf,
 				    make_script_args.argc, make_script_args.argv,
@@ -265,9 +267,10 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
 	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
-		*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
+	const char *onto_name = NULL,
+		*squash_onto = NULL, *head_name = NULL,
 		*switch_to = NULL, *cmd = NULL;
+	struct commit *onto = NULL, *upstream = NULL, *restrict_revision = NULL;
 	struct string_list commands = STRING_LIST_INIT_DUP;
 	char *raw_strategies = NULL;
 	enum {
@@ -303,13 +306,16 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			N_("insert exec commands in todo list"), ADD_EXEC),
-		OPT_STRING(0, "onto", &onto, N_("onto"), N_("onto")),
-		OPT_STRING(0, "restrict-revision", &restrict_revision,
-			   N_("restrict-revision"), N_("restrict revision")),
+		{ OPTION_CALLBACK, 0, "onto", &onto, N_("onto"), N_("onto"),
+		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
+		{ OPTION_CALLBACK, 0, "restrict-revision", &restrict_revision,
+		  N_("restrict-revision"), N_("restrict revision"),
+		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
 		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
 			   N_("squash onto")),
-		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
-			   N_("the upstream commit")),
+		{ OPTION_CALLBACK, 0, "upstream", &upstream, N_("upstream"),
+		  N_("the upstream commit"), PARSE_OPT_NONEG, parse_opt_commit,
+		  0 },
 		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
 		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
 			N_("GPG-sign commits"),
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 2733393546..2206eb763c 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -96,6 +96,23 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_commit(const struct option *opt, const char *arg, int unset)
+{
+	struct object_id oid;
+	struct commit *commit;
+	struct commit **target = opt->value;
+
+	if (!arg)
+		return -1;
+	if (get_oid(arg, &oid))
+		return error("malformed object name %s", arg);
+	commit = lookup_commit_reference(the_repository, &oid);
+	if (!commit)
+		return error("no such commit %s", arg);
+	*target = commit;
+	return 0;
+}
+
 int parse_opt_object_name(const struct option *opt, const char *arg, int unset)
 {
 	struct object_id oid;
diff --git a/parse-options.h b/parse-options.h
index 7d83e2971d..5a75646618 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -266,6 +266,7 @@ int parse_opt_color_flag_cb(const struct option *, const char *, int);
 int parse_opt_verbosity_cb(const struct option *, const char *, int);
 int parse_opt_object_name(const struct option *, const char *, int);
 int parse_opt_commits(const struct option *, const char *, int);
+int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
diff --git a/sequencer.c b/sequencer.c
index ccc0160800..610b7ece14 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2418,14 +2418,15 @@ static void write_strategy_opts(struct replay_opts *opts)
 }
 
 int write_basic_state(struct replay_opts *opts, const char *head_name,
-		      const char *onto, const char *orig_head)
+		      struct commit *onto, const char *orig_head)
 {
 	const char *quiet = getenv("GIT_QUIET");
 
 	if (head_name)
 		write_file(rebase_path_head_name(), "%s\n", head_name);
 	if (onto)
-		write_file(rebase_path_onto(), "%s\n", onto);
+		write_file(rebase_path_onto(), "%s\n",
+			   oid_to_hex(&onto->object.oid));
 	if (orig_head)
 		write_file(rebase_path_orig_head(), "%s\n", orig_head);
 
@@ -3456,7 +3457,7 @@ int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
 }
 
 static int checkout_onto(struct repository *r, struct replay_opts *opts,
-			 const char *onto_name, const char *onto,
+			 const char *onto_name, const struct object_id *onto,
 			 const char *orig_head)
 {
 	struct object_id oid;
@@ -3465,7 +3466,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
 	if (get_oid(orig_head, &oid))
 		return error(_("%s: not a valid OID"), orig_head);
 
-	if (run_git_checkout(r, opts, onto, action)) {
+	if (run_git_checkout(r, opts, oid_to_hex(onto), action)) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
 		return error(_("could not detach HEAD"));
@@ -4741,16 +4742,16 @@ static int skip_unnecessary_picks(struct repository *r,
 
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
-		    const char *onto, const char *orig_head, struct string_list *commands,
-		    unsigned autosquash, struct todo_list *todo_list)
+		    struct commit *onto, const char *orig_head,
+		    struct string_list *commands, unsigned autosquash,
+		    struct todo_list *todo_list)
 {
 	const char *shortonto, *todo_file = rebase_path_todo();
 	struct todo_list new_todo = TODO_LIST_INIT;
 	struct strbuf *buf = &todo_list->buf;
-	struct object_id oid;
+	struct object_id oid = onto->object.oid;
 	int res;
 
-	get_oid(onto, &oid);
 	shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
 
 	if (buf->len == 0) {
@@ -4793,7 +4794,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
 	    todo_list_check(todo_list, &new_todo)) {
 		fprintf(stderr, _(edit_todo_list_advice));
-		checkout_onto(r, opts, onto_name, onto, orig_head);
+		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
 		todo_list_release(&new_todo);
 
 		return -1;
@@ -4812,7 +4813,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 
 	todo_list_release(&new_todo);
 
-	if (checkout_onto(r, opts, onto_name, oid_to_hex(&oid), orig_head))
+	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
diff --git a/sequencer.h b/sequencer.h
index 6c55aa4200..e640ca21f2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -150,7 +150,7 @@ void todo_list_add_exec_commands(struct todo_list *todo_list,
 int check_todo_list_from_file(struct repository *r);
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
-		    const char *onto, const char *orig_head, struct string_list *commands,
+		    struct commit *onto, const char *orig_head, struct string_list *commands,
 		    unsigned autosquash, struct todo_list *todo_list);
 int todo_list_rearrange_squash(struct todo_list *todo_list);
 
@@ -191,4 +191,4 @@ int read_author_script(const char *path, char **name, char **email, char **date,
 
 void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
 int write_basic_state(struct replay_opts *opts, const char *head_name,
-		      const char *onto, const char *orig_head);
+		      struct commit *onto, const char *orig_head);
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 09/12] rebase -i: use struct rebase_options to parse args

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In order to run `rebase -i` without forking `rebase--interactive` it will be convenient to use the same structure when parsing the options in cmd_rebase() and cmd_rebase__interactive().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 203 ++++++++++++++++++++++++++---------------------
 1 file changed, 112 insertions(+), 91 deletions(-)
Show changes to builtin/rebase.c +112 −91
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 3b9da69473..aa97d81ab8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -50,6 +50,73 @@ enum rebase_type {
 	REBASE_PRESERVE_MERGES
 };
 
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	const char *upstream_arg;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	const char *switch_to;
+	int root;
+	struct object_id *squash_onto;
+	struct commit *restrict_revision;
+	int dont_finish_rebase;
+	enum {
+		REBASE_NO_QUIET = 1<<0,
+		REBASE_VERBOSE = 1<<1,
+		REBASE_DIFFSTAT = 1<<2,
+		REBASE_FORCE = 1<<3,
+		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
+	} flags;
+	struct argv_array git_am_opts;
+	const char *action;
+	int signoff;
+	int allow_rerere_autoupdate;
+	int keep_empty;
+	int autosquash;
+	char *gpg_sign_opt;
+	int autostash;
+	char *cmd;
+	int allow_empty_message;
+	int rebase_merges, rebase_cousins;
+	char *strategy, *strategy_opts;
+	struct strbuf git_format_patch_opt;
+	int reschedule_failed_exec;
+};
+
+#define REBASE_OPTIONS_INIT {			  	\
+		.type = REBASE_UNSPECIFIED,	  	\
+		.flags = REBASE_NO_QUIET, 		\
+		.git_am_opts = ARGV_ARRAY_INIT,		\
+		.git_format_patch_opt = STRBUF_INIT	\
+	}
+
+static struct replay_opts get_replay_opts(const struct rebase_options *opts)
+{
+	struct replay_opts replay = REPLAY_OPTS_INIT;
+
+	replay.action = REPLAY_INTERACTIVE_REBASE;
+	sequencer_init_config(&replay);
+
+	replay.signoff = opts->signoff;
+	replay.allow_ff = !(opts->flags & REBASE_FORCE);
+	if (opts->allow_rerere_autoupdate)
+		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
+	replay.allow_empty = 1;
+	replay.allow_empty_message = opts->allow_empty_message;
+	replay.verbose = opts->flags & REBASE_VERBOSE;
+	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
+	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
+	replay.strategy = opts->strategy;
+
+	return replay;
+}
+
 static int add_exec_commands(struct string_list *commands)
 {
 	const char *todo_file = rebase_path_todo();
@@ -265,32 +332,30 @@ static const char * const builtin_rebase_interactive_usage[] = {
 
 int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 {
-	struct replay_opts opts = REPLAY_OPTS_INIT;
-	unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
-	int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
-	const char *onto_name = NULL, *head_name = NULL, *switch_to = NULL,
-		*cmd = NULL;
-	struct commit *onto = NULL, *upstream = NULL, *restrict_revision = NULL;
+	struct rebase_options opts = REBASE_OPTIONS_INIT;
+	unsigned flags = 0;
+	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
-	struct object_id *squash_onto_opt = NULL;
 	struct string_list commands = STRING_LIST_INIT_DUP;
-	char *raw_strategies = NULL;
 	enum {
 		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
 		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
 	} command = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
-		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
+		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
+			   REBASE_FORCE),
+		OPT_BOOL(0, "keep-empty", &opts.keep_empty, N_("keep empty commits")),
 		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
 			 N_("allow commits with empty messages")),
-		OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
-		OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
+		OPT_BOOL(0, "rebase-merges", &opts.rebase_merges, N_("rebase merge commits")),
+		OPT_BOOL(0, "rebase-cousins", &opts.rebase_cousins,
 			 N_("keep original branch points of cousins")),
-		OPT_BOOL(0, "autosquash", &autosquash,
+		OPT_BOOL(0, "autosquash", &opts.autosquash,
 			 N_("move commits that begin with squash!/fixup!")),
 		OPT_BOOL(0, "signoff", &opts.signoff, N_("sign commits")),
-		OPT__VERBOSE(&opts.verbose, N_("be verbose")),
+		OPT_BIT('v', "verbose", &opts.flags,
+			N_("display a diffstat of what changed upstream"),
+			REBASE_NO_QUIET | REBASE_VERBOSE | REBASE_DIFFSTAT),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 			    CONTINUE),
 		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
@@ -308,86 +373,86 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
 			N_("insert exec commands in todo list"), ADD_EXEC),
-		{ OPTION_CALLBACK, 0, "onto", &onto, N_("onto"), N_("onto"),
+		{ OPTION_CALLBACK, 0, "onto", &opts.onto, N_("onto"), N_("onto"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
-		{ OPTION_CALLBACK, 0, "restrict-revision", &restrict_revision,
+		{ OPTION_CALLBACK, 0, "restrict-revision", &opts.restrict_revision,
 		  N_("restrict-revision"), N_("restrict revision"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
 		{ OPTION_CALLBACK, 0, "squash-onto", &squash_onto, N_("squash-onto"),
 		  N_("squash onto"), PARSE_OPT_NONEG, parse_opt_object_id, 0 },
-		{ OPTION_CALLBACK, 0, "upstream", &upstream, N_("upstream"),
+		{ OPTION_CALLBACK, 0, "upstream", &opts.upstream, N_("upstream"),
 		  N_("the upstream commit"), PARSE_OPT_NONEG, parse_opt_commit,
 		  0 },
-		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
-		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign, N_("key-id"),
+		OPT_STRING(0, "head-name", &opts.head_name, N_("head-name"), N_("head name")),
+		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign_opt, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_STRING(0, "strategy", &opts.strategy, N_("strategy"),
 			   N_("rebase strategy")),
-		OPT_STRING(0, "strategy-opts", &raw_strategies, N_("strategy-opts"),
+		OPT_STRING(0, "strategy-opts", &opts.strategy_opts, N_("strategy-opts"),
 			   N_("strategy options")),
-		OPT_STRING(0, "switch-to", &switch_to, N_("switch-to"),
+		OPT_STRING(0, "switch-to", &opts.switch_to, N_("switch-to"),
 			   N_("the branch or commit to checkout")),
-		OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
-		OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
-		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+		OPT_STRING(0, "onto-name", &opts.onto_name, N_("onto-name"), N_("onto name")),
+		OPT_STRING(0, "cmd", &opts.cmd, N_("cmd"), N_("the command to run")),
+		OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_autoupdate),
 		OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
 			 N_("automatically re-schedule any `exec` that fails")),
 		OPT_END()
 	};
 
-	sequencer_init_config(&opts);
+	opts.rebase_cousins = -1;
+
 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
-	opts.action = REPLAY_INTERACTIVE_REBASE;
-	opts.allow_ff = 1;
-	opts.allow_empty = 1;
-
 	if (argc == 1)
 		usage_with_options(builtin_rebase_interactive_usage, options);
 
 	argc = parse_options(argc, argv, NULL, options,
 			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
 
-	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
-
 	if (!is_null_oid(&squash_onto))
-		squash_onto_opt = &squash_onto;
+		opts.squash_onto = &squash_onto;
 
-	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= opts.keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
-	flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
-	flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
+	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
 	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
-	if (rebase_cousins >= 0 && !rebase_merges)
+	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
-	if (cmd && *cmd) {
-		string_list_split(&commands, cmd, '\n', -1);
+	if (opts.cmd && *opts.cmd) {
+		string_list_split(&commands, opts.cmd, '\n', -1);
 
 		/* rebase.c adds a new line to cmd after every command,
 		 * so here the last command is always empty */
 		string_list_remove_empty_items(&commands, 0);
 	}
 
 	switch (command) {
-	case NONE:
-		if (!onto && !upstream)
+	case NONE: {
+		struct replay_opts replay_opts = get_replay_opts(&opts);
+		if (!opts.onto && !opts.upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
-		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
-					    onto_name, squash_onto_opt, head_name, restrict_revision,
-					    raw_strategies, &commands, autosquash);
+		ret = do_interactive_rebase(&replay_opts, flags, opts.switch_to, opts.upstream, opts.onto,
+					    opts.onto_name, opts.squash_onto, opts.head_name, opts.restrict_revision,
+					    opts.strategy_opts, &commands, opts.autosquash);
 		break;
+	}
 	case SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		rerere_clear(the_repository, &merge_rr);
+	}
 		/* fallthrough */
-	case CONTINUE:
-		ret = sequencer_continue(the_repository, &opts);
+	case CONTINUE: {
+		struct replay_opts replay_opts = get_replay_opts(&opts);
+
+		ret = sequencer_continue(the_repository, &replay_opts);
 		break;
 	}
 	case EDIT_TODO:
@@ -446,45 +511,6 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
-struct rebase_options {
-	enum rebase_type type;
-	const char *state_dir;
-	struct commit *upstream;
-	const char *upstream_name;
-	const char *upstream_arg;
-	char *head_name;
-	struct object_id orig_head;
-	struct commit *onto;
-	const char *onto_name;
-	const char *revisions;
-	const char *switch_to;
-	int root;
-	struct object_id *squash_onto;
-	struct commit *restrict_revision;
-	int dont_finish_rebase;
-	enum {
-		REBASE_NO_QUIET = 1<<0,
-		REBASE_VERBOSE = 1<<1,
-		REBASE_DIFFSTAT = 1<<2,
-		REBASE_FORCE = 1<<3,
-		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
-	} flags;
-	struct argv_array git_am_opts;
-	const char *action;
-	int signoff;
-	int allow_rerere_autoupdate;
-	int keep_empty;
-	int autosquash;
-	char *gpg_sign_opt;
-	int autostash;
-	char *cmd;
-	int allow_empty_message;
-	int rebase_merges, rebase_cousins;
-	char *strategy, *strategy_opts;
-	struct strbuf git_format_patch_opt;
-	int reschedule_failed_exec;
-};
-
 static int is_interactive(struct rebase_options *opts)
 {
 	return opts->type == REBASE_INTERACTIVE ||
@@ -1380,13 +1406,7 @@ static int check_exec_cmd(const char *cmd)
 
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
-	struct rebase_options options = {
-		.type = REBASE_UNSPECIFIED,
-		.flags = REBASE_NO_QUIET,
-		.git_am_opts = ARGV_ARRAY_INIT,
-		.allow_empty_message = 1,
-		.git_format_patch_opt = STRBUF_INIT,
-	};
+	struct rebase_options options = REBASE_OPTIONS_INIT;
 	const char *branch_name;
 	int ret, flags, total_argc, in_progress = 0;
 	int ok_to_skip_pre_rebase = 0;
@@ -1539,6 +1559,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
+	options.allow_empty_message = 1;
 	git_config(rebase_config, &options);
 
 	strbuf_reset(&buf);
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 10/12] rebase -i: use struct rebase_options in do_interactive_rebase()

From: Phillip Wood <phillip.wood@dunelm.org.uk>

All the parameters that are passed to do_interactive_rebase() apart from `flags` are already in `struct rebase_options` so there is no need to pass them separately.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 69 +++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)
Show changes to builtin/rebase.c +36 −33
diff --git a/builtin/rebase.c b/builtin/rebase.c
index aa97d81ab8..277dbaadf4 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -113,6 +113,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
 	replay.strategy = opts->strategy;
+	if (opts->strategy_opts)
+		parse_strategy_opts(&replay, opts->strategy_opts);
 
 	return replay;
 }
@@ -262,44 +264,50 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 	return write_basic_state(opts, head_name, onto, orig_head);
 }
 
-static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
-				 const char *switch_to, struct commit *upstream,
-				 struct commit *onto, const char *onto_name,
-				 struct object_id *squash_onto, const char *head_name,
-				 struct commit *restrict_revision, char *raw_strategies,
-				 struct string_list *commands, unsigned autosquash)
+static void split_exec_commands(const char *cmd, struct string_list *commands)
+{
+	if (cmd && *cmd) {
+		string_list_split(commands, cmd, '\n', -1);
+
+		/* rebase.c adds a new line to cmd after every command,
+		 * so here the last command is always empty */
+		string_list_remove_empty_items(commands, 0);
+	}
+}
+
+static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
 	int ret;
 	const char *head_hash = NULL;
 	char *revisions = NULL, *shortrevisions = NULL;
 	struct argv_array make_script_args = ARGV_ARRAY_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
+	struct replay_opts replay = get_replay_opts(opts);
+	struct string_list commands = STRING_LIST_INIT_DUP;
 
-	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
+	if (prepare_branch_to_be_rebased(the_repository, &replay,
+					 opts->switch_to))
 		return -1;
 
-	if (get_revision_ranges(upstream, onto, &head_hash,
+	if (get_revision_ranges(opts->upstream, opts->onto, &head_hash,
 				&revisions, &shortrevisions))
 		return -1;
 
-	if (raw_strategies)
-		parse_strategy_opts(opts, raw_strategies);
-
-	if (init_basic_state(opts, head_name, onto, head_hash)) {
+	if (init_basic_state(&replay, opts->head_name, opts->onto, head_hash)) {
 		free(revisions);
 		free(shortrevisions);
 
 		return -1;
 	}
 
-	if (!upstream && squash_onto)
+	if (!opts->upstream && opts->squash_onto)
 		write_file(path_squash_onto(), "%s\n",
-			   oid_to_hex(squash_onto));
+			   oid_to_hex(opts->squash_onto));
 
 	argv_array_pushl(&make_script_args, "", revisions, NULL);
-	if (restrict_revision)
+	if (opts->restrict_revision)
 		argv_array_push(&make_script_args,
-				oid_to_hex(&restrict_revision->object.oid));
+				oid_to_hex(&opts->restrict_revision->object.oid));
 
 	ret = sequencer_make_script(the_repository, &todo_list.buf,
 				    make_script_args.argc, make_script_args.argv,
@@ -313,10 +321,13 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 						&todo_list))
 			BUG("unusable todo list");
 
-		ret = complete_action(the_repository, opts, flags, shortrevisions, onto_name,
-				      onto, head_hash, commands, autosquash, &todo_list);
+		split_exec_commands(opts->cmd, &commands);
+		ret = complete_action(the_repository, &replay, flags,
+			shortrevisions, opts->onto_name, opts->onto, head_hash,
+			&commands, opts->autosquash, &todo_list);
 	}
 
+	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
 	todo_list_release(&todo_list);
@@ -336,7 +347,6 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
-	struct string_list commands = STRING_LIST_INIT_DUP;
 	enum {
 		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
 		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
@@ -424,23 +434,12 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
-	if (opts.cmd && *opts.cmd) {
-		string_list_split(&commands, opts.cmd, '\n', -1);
-
-		/* rebase.c adds a new line to cmd after every command,
-		 * so here the last command is always empty */
-		string_list_remove_empty_items(&commands, 0);
-	}
-
 	switch (command) {
 	case NONE: {
-		struct replay_opts replay_opts = get_replay_opts(&opts);
 		if (!opts.onto && !opts.upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
-		ret = do_interactive_rebase(&replay_opts, flags, opts.switch_to, opts.upstream, opts.onto,
-					    opts.onto_name, opts.squash_onto, opts.head_name, opts.restrict_revision,
-					    opts.strategy_opts, &commands, opts.autosquash);
+		ret = do_interactive_rebase(&opts, flags);
 		break;
 	}
 	case SKIP: {
@@ -477,14 +476,18 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	case REARRANGE_SQUASH:
 		ret = rearrange_squash_in_todo_file();
 		break;
-	case ADD_EXEC:
+	case ADD_EXEC: {
+		struct string_list commands = STRING_LIST_INIT_DUP;
+
+		split_exec_commands(opts.cmd, &commands);
 		ret = add_exec_commands(&commands);
+		string_list_clear(&commands, 0);
 		break;
+	}
 	default:
 		BUG("invalid command '%d'", command);
 	}
 
-	string_list_clear(&commands, 0);
 	return !!ret;
 }
 
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 12/12] rebase -i: run without forking rebase--interactive

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When the builtin rebase starts an interactive rebase it parses the options and then repackages them and forks `rebase--interactive`. Separate the option parsing in cmd_rebase__interactive() from the business logic to allow interactive rebases can be run without forking `rebase__interactive` by calling run_rebase_interactive() directly.

Starting interactive rebases without forking makes it easy to debug the sequencer without worrying about attaching to child processes. Ævar has also reported that some of the rebase perf tests are 30% faster [1].

This patch also makes it easy to remove cmd_rebase__interactive() in the future when git-legacy-rebase.sh and git-rebase--preserve-merges.sh are retired.

[1] https://public-inbox.org/git/87y359cfjj.fsf@evledraar.gmail.com/
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 221 ++++++++++++++++++-----------------------------
 1 file changed, 86 insertions(+), 135 deletions(-)
Show changes to builtin/rebase.c +86 −135
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0020e1ecf6..cd233c6118 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -316,7 +316,9 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 				&revisions, &shortrevisions))
 		return -1;
 
-	if (init_basic_state(&replay, opts->head_name, opts->onto, head_hash)) {
+	if (init_basic_state(&replay,
+			     opts->head_name ? opts->head_name : "detached HEAD",
+			     opts->onto, head_hash)) {
 		free(revisions);
 		free(shortrevisions);
 
@@ -359,6 +361,77 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	return ret;
 }
 
+static int run_rebase_interactive(struct rebase_options *opts,
+				  enum action command)
+{
+	unsigned flags = 0;
+	int abbreviate_commands = 0, ret = 0;
+
+	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
+
+	flags |= opts->keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
+	flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
+	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+
+	switch (command) {
+	case ACTION_NONE: {
+		if (!opts->onto && !opts->upstream)
+			die(_("a base commit must be provided with --upstream or --onto"));
+
+		ret = do_interactive_rebase(opts, flags);
+		break;
+	}
+	case ACTION_SKIP: {
+		struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+		rerere_clear(the_repository, &merge_rr);
+	}
+		/* fallthrough */
+	case ACTION_CONTINUE: {
+		struct replay_opts replay_opts = get_replay_opts(opts);
+
+		ret = sequencer_continue(the_repository, &replay_opts);
+		break;
+	}
+	case ACTION_EDIT_TODO:
+		ret = edit_todo_file(flags);
+		break;
+	case ACTION_SHOW_CURRENT_PATCH: {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
+		ret = run_command(&cmd);
+
+		break;
+	}
+	case ACTION_SHORTEN_OIDS:
+	case ACTION_EXPAND_OIDS:
+		ret = transform_todo_file(flags);
+		break;
+	case ACTION_CHECK_TODO_LIST:
+		ret = check_todo_list_from_file(the_repository);
+		break;
+	case ACTION_REARRANGE_SQUASH:
+		ret = rearrange_squash_in_todo_file();
+		break;
+	case ACTION_ADD_EXEC: {
+		struct string_list commands = STRING_LIST_INIT_DUP;
+
+		split_exec_commands(opts->cmd, &commands);
+		ret = add_exec_commands(&commands);
+		string_list_clear(&commands, 0);
+		break;
+	}
+	default:
+		BUG("invalid command '%d'", command);
+	}
+
+	return ret;
+}
+
 static const char * const builtin_rebase_interactive_usage[] = {
 	N_("git rebase--interactive [<options>]"),
 	NULL
@@ -367,8 +440,6 @@ static const char * const builtin_rebase_interactive_usage[] = {
 int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 {
 	struct rebase_options opts = REBASE_OPTIONS_INIT;
-	unsigned flags = 0;
-	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
 	enum action command = ACTION_NONE;
 	struct option options[] = {
@@ -433,8 +504,6 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 
 	opts.rebase_cousins = -1;
 
-	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
-
 	if (argc == 1)
 		usage_with_options(builtin_rebase_interactive_usage, options);
 
@@ -444,71 +513,11 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	if (!is_null_oid(&squash_onto))
 		opts.squash_onto = &squash_onto;
 
-	flags |= opts.keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
-	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
-	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
-	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
-	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
-
 	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
-	switch (command) {
-	case ACTION_NONE: {
-		if (!opts.onto && !opts.upstream)
-			die(_("a base commit must be provided with --upstream or --onto"));
-
-		ret = do_interactive_rebase(&opts, flags);
-		break;
-	}
-	case ACTION_SKIP: {
-		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-
-		rerere_clear(the_repository, &merge_rr);
-	}
-		/* fallthrough */
-	case ACTION_CONTINUE: {
-		struct replay_opts replay_opts = get_replay_opts(&opts);
-
-		ret = sequencer_continue(the_repository, &replay_opts);
-		break;
-	}
-	case ACTION_EDIT_TODO:
-		ret = edit_todo_file(flags);
-		break;
-	case ACTION_SHOW_CURRENT_PATCH: {
-		struct child_process cmd = CHILD_PROCESS_INIT;
-
-		cmd.git_cmd = 1;
-		argv_array_pushl(&cmd.args, "show", "REBASE_HEAD", "--", NULL);
-		ret = run_command(&cmd);
-
-		break;
-	}
-	case ACTION_SHORTEN_OIDS:
-	case ACTION_EXPAND_OIDS:
-		ret = transform_todo_file(flags);
-		break;
-	case ACTION_CHECK_TODO_LIST:
-		ret = check_todo_list_from_file(the_repository);
-		break;
-	case ACTION_REARRANGE_SQUASH:
-		ret = rearrange_squash_in_todo_file();
-		break;
-	case ACTION_ADD_EXEC: {
-		struct string_list commands = STRING_LIST_INIT_DUP;
-
-		split_exec_commands(opts.cmd, &commands);
-		ret = add_exec_commands(&commands);
-		string_list_clear(&commands, 0);
-		break;
-	}
-	default:
-		BUG("invalid command '%d'", command);
-	}
-
-	return !!ret;
+	return !!run_rebase_interactive(&opts, command);
 }
 
 static int use_builtin_rebase(void)
@@ -1071,7 +1080,7 @@ static int run_am(struct rebase_options *opts)
 	return status;
 }
 
-static int run_specific_rebase(struct rebase_options *opts)
+static int run_specific_rebase(struct rebase_options *opts, enum action action)
 {
 	const char *argv[] = { NULL, NULL };
 	struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
@@ -1080,77 +1089,19 @@ static int run_specific_rebase(struct rebase_options *opts)
 
 	if (opts->type == REBASE_INTERACTIVE) {
 		/* Run builtin interactive rebase */
-		struct child_process child = CHILD_PROCESS_INIT;
-
-		argv_array_pushf(&child.env_array, "GIT_CHERRY_PICK_HELP=%s",
-				 resolvemsg);
+		setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
 		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
-			argv_array_push(&child.env_array,
-					"GIT_SEQUENCE_EDITOR=:");
+			setenv("GIT_SEQUENCE_EDITOR", ":", 1);
 			opts->autosquash = 0;
 		}
+		if (opts->gpg_sign_opt) {
+			/* remove the leading "-S" */
+			char *tmp = xstrdup(opts->gpg_sign_opt + 2);
+			free(opts->gpg_sign_opt);
+			opts->gpg_sign_opt = tmp;
+		}
 
-		child.git_cmd = 1;
-		argv_array_push(&child.args, "rebase--interactive");
-
-		if (opts->action)
-			argv_array_pushf(&child.args, "--%s", opts->action);
-		if (opts->keep_empty)
-			argv_array_push(&child.args, "--keep-empty");
-		if (opts->rebase_merges)
-			argv_array_push(&child.args, "--rebase-merges");
-		if (opts->rebase_cousins)
-			argv_array_push(&child.args, "--rebase-cousins");
-		if (opts->autosquash)
-			argv_array_push(&child.args, "--autosquash");
-		if (opts->flags & REBASE_VERBOSE)
-			argv_array_push(&child.args, "--verbose");
-		if (opts->flags & REBASE_FORCE)
-			argv_array_push(&child.args, "--no-ff");
-		if (opts->restrict_revision)
-			argv_array_pushf(&child.args,
-					 "--restrict-revision=^%s",
-					 oid_to_hex(&opts->restrict_revision->object.oid));
-		if (opts->upstream)
-			argv_array_pushf(&child.args, "--upstream=%s",
-					 oid_to_hex(&opts->upstream->object.oid));
-		if (opts->onto)
-			argv_array_pushf(&child.args, "--onto=%s",
-					 oid_to_hex(&opts->onto->object.oid));
-		if (opts->squash_onto)
-			argv_array_pushf(&child.args, "--squash-onto=%s",
-					 oid_to_hex(opts->squash_onto));
-		if (opts->onto_name)
-			argv_array_pushf(&child.args, "--onto-name=%s",
-					 opts->onto_name);
-		argv_array_pushf(&child.args, "--head-name=%s",
-				 opts->head_name ?
-				 opts->head_name : "detached HEAD");
-		if (opts->strategy)
-			argv_array_pushf(&child.args, "--strategy=%s",
-					 opts->strategy);
-		if (opts->strategy_opts)
-			argv_array_pushf(&child.args, "--strategy-opts=%s",
-					 opts->strategy_opts);
-		if (opts->switch_to)
-			argv_array_pushf(&child.args, "--switch-to=%s",
-					 opts->switch_to);
-		if (opts->cmd)
-			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
-		if (opts->allow_empty_message)
-			argv_array_push(&child.args, "--allow-empty-message");
-		if (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)
-			argv_array_push(&child.args, "--rerere-autoupdate");
-		else if (opts->allow_rerere_autoupdate == RERERE_NOAUTOUPDATE)
-			argv_array_push(&child.args, "--no-rerere-autoupdate");
-		if (opts->gpg_sign_opt)
-			argv_array_push(&child.args, opts->gpg_sign_opt);
-		if (opts->signoff)
-			argv_array_push(&child.args, "--signoff");
-		if (opts->reschedule_failed_exec)
-			argv_array_push(&child.args, "--reschedule-failed-exec");
-
-		status = run_command(&child);
+		status = run_rebase_interactive(opts, action);
 		goto finished_rebase;
 	}
 
@@ -2211,7 +2162,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = !!run_specific_rebase(&options);
+	ret = !!run_specific_rebase(&options, action);
 
 cleanup:
 	strbuf_release(&revisions);
-- 
2.21.0
Phillip Wood· Apr 17, 2019, 14:30 UTC · re: Phillip Wood · lore

[PATCH v1 11/12] rebase: use a common action enum

From: Phillip Wood <phillip.wood@dunelm.org.uk>

cmd_rebase() and cmd_rebase__interactive() used different enums to hold the current action. Change to using a common enum so the values are the same when we change `rebase -i` to avoid forking `rebase--interactive`.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 91 +++++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 43 deletions(-)
Show changes to builtin/rebase.c +48 −43
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 277dbaadf4..0020e1ecf6 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -119,6 +119,29 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	return replay;
 }
 
+enum action {
+	ACTION_NONE = 0,
+	ACTION_CONTINUE,
+	ACTION_SKIP,
+	ACTION_ABORT,
+	ACTION_QUIT,
+	ACTION_EDIT_TODO,
+	ACTION_SHOW_CURRENT_PATCH,
+	ACTION_SHORTEN_OIDS,
+	ACTION_EXPAND_OIDS,
+	ACTION_CHECK_TODO_LIST,
+	ACTION_REARRANGE_SQUASH,
+	ACTION_ADD_EXEC
+};
+
+static const char *action_names[] = { "undefined",
+				      "continue",
+				      "skip",
+				      "abort",
+				      "quit",
+				      "edit_todo",
+				      "show_current_patch" };
+
 static int add_exec_commands(struct string_list *commands)
 {
 	const char *todo_file = rebase_path_todo();
@@ -347,10 +370,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
 	struct object_id squash_onto = null_oid;
-	enum {
-		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
-		SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC
-	} command = 0;
+	enum action command = ACTION_NONE;
 	struct option options[] = {
 		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
 			   REBASE_FORCE),
@@ -367,22 +387,22 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			N_("display a diffstat of what changed upstream"),
 			REBASE_NO_QUIET | REBASE_VERBOSE | REBASE_DIFFSTAT),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
-			    CONTINUE),
-		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), SKIP),
+			    ACTION_CONTINUE),
+		OPT_CMDMODE(0, "skip", &command, N_("skip commit"), ACTION_SKIP),
 		OPT_CMDMODE(0, "edit-todo", &command, N_("edit the todo list"),
-			    EDIT_TODO),
+			    ACTION_EDIT_TODO),
 		OPT_CMDMODE(0, "show-current-patch", &command, N_("show the current patch"),
-			    SHOW_CURRENT_PATCH),
+			    ACTION_SHOW_CURRENT_PATCH),
 		OPT_CMDMODE(0, "shorten-ids", &command,
-			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
+			N_("shorten commit ids in the todo list"), ACTION_SHORTEN_OIDS),
 		OPT_CMDMODE(0, "expand-ids", &command,
-			N_("expand commit ids in the todo list"), EXPAND_OIDS),
+			N_("expand commit ids in the todo list"), ACTION_EXPAND_OIDS),
 		OPT_CMDMODE(0, "check-todo-list", &command,
-			N_("check the todo list"), CHECK_TODO_LIST),
+			N_("check the todo list"), ACTION_CHECK_TODO_LIST),
 		OPT_CMDMODE(0, "rearrange-squash", &command,
-			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+			N_("rearrange fixup/squash lines"), ACTION_REARRANGE_SQUASH),
 		OPT_CMDMODE(0, "add-exec-commands", &command,
-			N_("insert exec commands in todo list"), ADD_EXEC),
+			N_("insert exec commands in todo list"), ACTION_ADD_EXEC),
 		{ OPTION_CALLBACK, 0, "onto", &opts.onto, N_("onto"), N_("onto"),
 		  PARSE_OPT_NONEG, parse_opt_commit, 0 },
 		{ OPTION_CALLBACK, 0, "restrict-revision", &opts.restrict_revision,
@@ -428,36 +448,36 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= opts.rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 	flags |= opts.rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
-	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
 	if (opts.rebase_cousins >= 0 && !opts.rebase_merges)
 		warning(_("--[no-]rebase-cousins has no effect without "
 			  "--rebase-merges"));
 
 	switch (command) {
-	case NONE: {
+	case ACTION_NONE: {
 		if (!opts.onto && !opts.upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
 
 		ret = do_interactive_rebase(&opts, flags);
 		break;
 	}
-	case SKIP: {
+	case ACTION_SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		rerere_clear(the_repository, &merge_rr);
 	}
 		/* fallthrough */
-	case CONTINUE: {
+	case ACTION_CONTINUE: {
 		struct replay_opts replay_opts = get_replay_opts(&opts);
 
 		ret = sequencer_continue(the_repository, &replay_opts);
 		break;
 	}
-	case EDIT_TODO:
+	case ACTION_EDIT_TODO:
 		ret = edit_todo_file(flags);
 		break;
-	case SHOW_CURRENT_PATCH: {
+	case ACTION_SHOW_CURRENT_PATCH: {
 		struct child_process cmd = CHILD_PROCESS_INIT;
 
 		cmd.git_cmd = 1;
@@ -466,17 +486,17 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 
 		break;
 	}
-	case SHORTEN_OIDS:
-	case EXPAND_OIDS:
+	case ACTION_SHORTEN_OIDS:
+	case ACTION_EXPAND_OIDS:
 		ret = transform_todo_file(flags);
 		break;
-	case CHECK_TODO_LIST:
+	case ACTION_CHECK_TODO_LIST:
 		ret = check_todo_list_from_file(the_repository);
 		break;
-	case REARRANGE_SQUASH:
+	case ACTION_REARRANGE_SQUASH:
 		ret = rearrange_squash_in_todo_file();
 		break;
-	case ADD_EXEC: {
+	case ACTION_ADD_EXEC: {
 		struct string_list commands = STRING_LIST_INIT_DUP;
 
 		split_exec_commands(opts.cmd, &commands);
@@ -1417,22 +1437,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id merge_base;
-	enum {
-		NO_ACTION,
-		ACTION_CONTINUE,
-		ACTION_SKIP,
-		ACTION_ABORT,
-		ACTION_QUIT,
-		ACTION_EDIT_TODO,
-		ACTION_SHOW_CURRENT_PATCH,
-	} action = NO_ACTION;
-	static const char *action_names[] = { "undefined",
-					      "continue",
-					      "skip",
-					      "abort",
-					      "quit",
-					      "edit_todo",
-					      "show_current_patch" };
+	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
@@ -1599,7 +1604,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
 
-	if (action != NO_ACTION && total_argc != 2) {
+	if (action != ACTION_NONE && total_argc != 2) {
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 	}
@@ -1608,7 +1613,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 
-	if (action != NO_ACTION && !in_progress)
+	if (action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
 	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
@@ -1708,7 +1713,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.action = "show-current-patch";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
-	case NO_ACTION:
+	case ACTION_NONE:
 		break;
 	default:
 		BUG("action: %d", action);
-- 
2.21.0

← back to recent threads