[PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Phil Hord]

From: Phil Hord <phil.hord@gmail.com>

I have a local patch to rebase--interactive.sh that adds "edit" switches
to rebase, permitting me to say, for example,

    git rebase --drop $sha

This command creates a todo-list but drops the mentioned commit from the
list by changing the "pick" to "drop". Other switches let me edit or
reword a commit in my local history, greatly simplifying my branch
grooming workflow.

With the conversion to rebase.c (yay!) my tools are going away. I'm
porting them to the rewritten rebase*.c and I want to submit them here.
But rebase*.c is still in flux, and my changes have many conflicts with
other inflight changes. I'm happy to wait for those, but in the
meantime, I'd appreciate some feedback on the utility and acceptability
of my plan.

Here's my patch series as it stands today. It lacks documentation and
tests, but it mostly works. Errors are not handled gracefully, but this
will be rectified after I rebase onto pw/rebase-i-internal-rfc.

Currently it supports these switches:

    usage: git rebase [-i] [options] [--exec <cmd>] ...
       :
    --break <revision>    stop before the mentioned ref
    --drop <revision>     drop the mentioned ref from the todo list
    --edit <revision>     edit the mentioned ref instead of picking it
    --reword <revision>   reword the mentioned ref instead of picking it

I have plans to add these, but I don't like how their "onto" will be
controlled. More thinking is needed here.

    --fixup <revision>    fixup the mentioned ref instead of picking it
    --squash <revision>   squash the mentioned ref instead of picking it
    --pick <revision>     pick the mentioned ref onto the start of the list


Phil Hord (2):
  rebase: add switches for drop, edit and reword
  rebase: add --break switch

 builtin/rebase--interactive.c |  49 +++++++++++++++-
 builtin/rebase.c              |  48 ++++++++++++++++
 sequencer.c                   | 105 +++++++++++++++++++++++++++++-----
 sequencer.h                   |  22 ++++++-
 4 files changed, 207 insertions(+), 17 deletions(-)

--
2.20.1

[PATCH/RFC 1/2] rebase: add switches for drop, edit and reword

[Phil Hord]

From: Phil Hord <phil.hord@gmail.com>

A common use for rebase is to drop or edit some commit in a
feature branch.  The commit to be changed is known in advance,
so a user will say 'git rebase -i that-commit^' to see the todo
list in her editor.  Then she will change the command on the line
for "that-commit" to edit instead of pick, or delete the line
altogether.

This involves 'git log' to find the commit in the first place,
the cli to start the rebase, the editor to make a single change, and
some mental context switch from "that-commit" to its hash so she can
be sure to edit the correct line.

Introduce some new "edit-todo" switches to the rebase command to
simplify this cycle.  Add 3 new switches to support common
todo-list operations.
    '--drop <ref>' to drop a specific commit,
    '--edit <ref>' to edit a specific commit,
    '--reword <ref>' to reword a specific commit,

Allow each switch to be used mutliple times on the command line so more
than one ref could be dropped, for example.

Complain and abort the rebase if a mentioned ref is not in the
todo-list in the first place so the user doesn't get a wrong idea
from a successful 'git rebase --drop foo' that did nothing since
no foo was encountered.

Signed-off-by: Phil Hord <phil.hord@gmail.com>
---
 builtin/rebase--interactive.c | 46 +++++++++++++++-
 builtin/rebase.c              | 44 ++++++++++++++++
 sequencer.c                   | 98 ++++++++++++++++++++++++++++++-----
 sequencer.h                   | 21 +++++++-
 4 files changed, 192 insertions(+), 17 deletions(-)

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 4535523bf5..9285d05443 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -140,6 +140,34 @@ static int get_revision_ranges(const char *upstream, const char *onto,
 	return 0;
 }

+static int resolve_commit_list(const struct string_list *str,
+			       struct commit_list **revs)
+{
+	struct object_id oid;
+	int i;
+	for (i = 0; i < str->nr; i++) {
+		struct commit *r;
+		const char * ref = str->items[i].string;
+		if (get_oid(ref, &oid))
+			return error(_("cannot resolve %s"), ref);
+
+		r = lookup_commit_reference(the_repository, &oid);
+		if (!r)
+			return error(_("%s is not a commit"), ref);
+
+		commit_list_insert(r, revs);
+		str->items[i].util = &(*revs)->item->object.oid;
+	}
+	return 0;
+}
+
+static int resolve_edits_commit_list(struct sequence_edits *edits)
+{
+	return resolve_commit_list(&edits->drop, &edits->revs) ||
+	       resolve_commit_list(&edits->edit, &edits->revs) ||
+	       resolve_commit_list(&edits->reword, &edits->revs);
+}
+
 static int init_basic_state(struct replay_opts *opts, const char *head_name,
 			    const char *onto, const char *orig_head)
 {
@@ -163,6 +191,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 				 const char *onto, const char *onto_name,
 				 const char *squash_onto, const char *head_name,
 				 const char *restrict_revision, char *raw_strategies,
+				 struct sequence_edits *edits,
 				 struct string_list *commands, unsigned autosquash)
 {
 	int ret;
@@ -197,7 +226,7 @@ static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,

 	ret = sequencer_make_script(the_repository, &todo_list.buf,
 				    make_script_args.argc, make_script_args.argv,
-				    flags);
+				    edits, flags);

 	if (ret)
 		error(_("could not generate todo list"));
@@ -233,6 +262,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
 		*switch_to = NULL, *cmd = NULL;
 	struct string_list commands = STRING_LIST_INIT_DUP;
+	struct sequence_edits edits = SEQUENCE_EDITS_INIT;
 	char *raw_strategies = NULL;
 	enum {
 		NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH,
@@ -272,6 +302,15 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			   N_("restrict-revision"), N_("restrict revision")),
 		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
 			   N_("squash onto")),
+		OPT_STRING_LIST(0, "drop", &edits.drop, N_("revision"),
+				N_("drop the mentioned ref from the "
+				   "todo list")),
+		OPT_STRING_LIST(0, "edit", &edits.edit, N_("revision"),
+				N_("edit the mentioned ref instead of "
+				   "picking it")),
+		OPT_STRING_LIST(0, "reword", &edits.reword, N_("revision"),
+				N_("reword the mentioned ref instead of "
+				   "picking it")),
 		OPT_STRING(0, "upstream", &upstream, N_("upstream"),
 			   N_("the upstream commit")),
 		OPT_STRING(0, "head-name", &head_name, N_("head-name"), N_("head name")),
@@ -325,6 +364,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		string_list_remove_empty_items(&commands, 0);
 	}

+	resolve_edits_commit_list(&edits);
+
 	switch (command) {
 	case NONE:
 		if (!onto && !upstream)
@@ -332,7 +373,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)

 		ret = do_interactive_rebase(&opts, flags, switch_to, upstream, onto,
 					    onto_name, squash_onto, head_name, restrict_revision,
-					    raw_strategies, &commands, autosquash);
+					    raw_strategies, &edits, &commands, autosquash);
 		break;
 	case SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
@@ -373,5 +414,6 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	}

 	string_list_clear(&commands, 0);
+	free_sequence_edits(&edits);
 	return !!ret;
 }
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2e41ad5644..a8101630cf 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -78,6 +78,7 @@ struct rebase_options {
 	char *gpg_sign_opt;
 	int autostash;
 	char *cmd;
+	char *edit_switches;
 	int allow_empty_message;
 	int rebase_merges, rebase_cousins;
 	char *strategy, *strategy_opts;
@@ -694,6 +695,8 @@ static int run_specific_rebase(struct rebase_options *opts)
 					 opts->switch_to);
 		if (opts->cmd)
 			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
+		if (opts->edit_switches)
+			argv_array_split(&child.args, opts->edit_switches);
 		if (opts->allow_empty_message)
 			argv_array_push(&child.args, "--allow-empty-message");
 		if (opts->allow_rerere_autoupdate > 0)
@@ -711,6 +714,9 @@ static int run_specific_rebase(struct rebase_options *opts)
 		goto finished_rebase;
 	}

+	if (opts->edit_switches)
+		BUG("Unexpected rebase type with switches %d", opts->type);
+
 	if (opts->type == REBASE_AM) {
 		status = run_am(opts);
 		goto finished_rebase;
@@ -988,6 +994,28 @@ static int check_exec_cmd(const char *cmd)
 	return 0;
 }

+static void forward_switches(struct rebase_options *options,
+			     const char *sw_name,
+			     struct string_list *values)
+{
+	int i;
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!values->nr)
+		return;
+
+	imply_interactive(options, sw_name);
+
+	if (!!options->edit_switches)
+		strbuf_addf(&buf, "%s ", options->edit_switches);
+
+	for (i = 0; i < values->nr; i++)
+		strbuf_addf(&buf, "%s %s ", sw_name, values->items[i].string);
+	strbuf_rtrim(&buf);
+	options->edit_switches = xstrdup(buf.buf);
+
+	strbuf_release(&buf);
+}

 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
@@ -1025,6 +1053,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 					      NULL };
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
+	struct string_list reword = STRING_LIST_INIT_NODUP;
+	struct string_list edit = STRING_LIST_INIT_NODUP;
+	struct string_list drop = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
 	int fork_point = -1;
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
@@ -1107,6 +1138,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
 				N_("add exec lines after each commit of the "
 				   "editable list")),
+		OPT_STRING_LIST(0, "drop", &drop, N_("revision"),
+				N_("drop the mentioned ref from the "
+				   "todo list")),
+		OPT_STRING_LIST(0, "edit", &edit, N_("revision"),
+				N_("edit the mentioned ref instead of "
+				   "picking it")),
+		OPT_STRING_LIST(0, "reword", &reword, N_("revision"),
+				N_("reword the mentioned ref instead of "
+				   "picking it")),
 		OPT_BOOL(0, "allow-empty-message",
 			 &options.allow_empty_message,
 			 N_("allow rebasing commits with empty messages")),
@@ -1364,6 +1404,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.cmd = xstrdup(buf.buf);
 	}

+	forward_switches(&options, "--drop", &drop);
+	forward_switches(&options, "--edit", &edit);
+	forward_switches(&options, "--reword", &reword);
+
 	if (rebase_merges) {
 		if (!*rebase_merges)
 			; /* default mode; do nothing */
diff --git a/sequencer.c b/sequencer.c
index 546f281898..d7384d987c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4259,9 +4259,81 @@ static const char *label_oid(struct object_id *oid, const char *label,
 	return string_entry->string;
 }

+void free_sequence_edits(struct sequence_edits *edits)
+{
+	string_list_clear(&edits->drop, 0);
+	string_list_clear(&edits->edit, 0);
+	string_list_clear(&edits->reword, 0);
+	free_commit_list(edits->revs);
+}
+
+/* Find an unmatched oid in the util pointer of our edit refs. If it's found
+   reset util to NULL and return 1. */
+static int consume_oid(const struct object_id *oid,
+		       const struct string_list *refs)
+{
+	int i;
+	for (i = 0; i < refs->nr; i++)
+		if (refs->items[i].util && oideq(oid, refs->items[i].util)) {
+			refs->items[i].util = NULL;
+			return 1;
+		}
+	return 0;
+}
+
+static int check_unused_refs(const struct string_list *refs)
+{
+	int i;
+	for (i = 0; i < refs->nr; i++)
+		if (refs->items[i].util)
+			return error(_("did not find '%s' in todo list"),
+					refs->items[i].string);
+	return 0;
+}
+
+static int check_unused_edits(const struct sequence_edits *edits)
+{
+	return check_unused_refs(&edits->drop) ||
+		check_unused_refs(&edits->edit) ||
+		check_unused_refs(&edits->reword);
+}
+
+static void add_todo_cmd(struct strbuf *buf, enum todo_command cmd,
+			      unsigned flags)
+{
+	if (flags & TODO_LIST_ABBREVIATE_CMDS)
+		strbuf_addch(buf, command_to_char(cmd));
+	else
+		strbuf_addstr(buf, command_to_string(cmd));
+}
+
+static void add_todo_cmd_oid(struct strbuf *buf, enum todo_command cmd,
+			     unsigned flags, const struct object_id *oid)
+{
+	add_todo_cmd(buf, cmd, flags);
+	strbuf_addf(buf, " %s ", oid_to_hex(oid));
+}
+
+static void add_edit_todo_inst(struct strbuf *buf, const struct object_id *oid,
+				const struct sequence_edits *edits,
+				unsigned flags)
+{
+	enum todo_command cmd = TODO_PICK;
+
+	if (consume_oid(oid, &edits->drop))
+		cmd = TODO_DROP;
+	else if (consume_oid(oid, &edits->edit))
+		cmd = TODO_EDIT;
+	else if (consume_oid(oid, &edits->reword))
+		cmd = TODO_REWORD;
+
+	add_todo_cmd_oid(buf, cmd, flags, oid);
+}
+
 static int make_script_with_merges(struct pretty_print_context *pp,
-				   struct rev_info *revs, struct strbuf *out,
-				   unsigned flags)
+				   struct rev_info *revs,
+				   const struct sequence_edits *edits,
+				   struct strbuf *out, unsigned flags)
 {
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
@@ -4277,8 +4349,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	struct label_state state = { OIDMAP_INIT, { NULL }, STRBUF_INIT };

 	int abbr = flags & TODO_LIST_ABBREVIATE_CMDS;
-	const char *cmd_pick = abbr ? "p" : "pick",
-		*cmd_label = abbr ? "l" : "label",
+	const char *cmd_label = abbr ? "l" : "label",
 		*cmd_reset = abbr ? "t" : "reset",
 		*cmd_merge = abbr ? "m" : "merge";

@@ -4322,9 +4393,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 			strbuf_reset(&buf);
 			if (!keep_empty && is_empty)
 				strbuf_addf(&buf, "%c ", comment_line_char);
-			strbuf_addf(&buf, "%s %s %s", cmd_pick,
-				    oid_to_hex(&commit->object.oid),
-				    oneline.buf);
+			add_edit_todo_inst(&buf, &commit->object.oid, edits,
+					   flags);
+			strbuf_addbuf(&buf, &oneline);

 			FLEX_ALLOC_STR(entry, string, buf.buf);
 			oidcpy(&entry->entry.oid, &commit->object.oid);
@@ -4481,18 +4552,18 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	hashmap_free(&state.labels, 1);
 	strbuf_release(&state.buf);

-	return 0;
+	return check_unused_edits(edits);
 }

 int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
-			  const char **argv, unsigned flags)
+			  const char **argv, const struct sequence_edits *edits,
+			  unsigned flags)
 {
 	char *format = NULL;
 	struct pretty_print_context pp = {0};
 	struct rev_info revs;
 	struct commit *commit;
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
-	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;

 	repo_init_revisions(r, &revs, NULL);
@@ -4524,7 +4595,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 		return error(_("make_script: error preparing revisions"));

 	if (rebase_merges)
-		return make_script_with_merges(&pp, &revs, out, flags);
+		return make_script_with_merges(&pp, &revs, edits, out, flags);

 	while ((commit = get_revision(&revs))) {
 		int is_empty  = is_original_commit_empty(commit);
@@ -4533,12 +4604,11 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 			continue;
 		if (!keep_empty && is_empty)
 			strbuf_addf(out, "%c ", comment_line_char);
-		strbuf_addf(out, "%s %s ", insn,
-			    oid_to_hex(&commit->object.oid));
+		add_edit_todo_inst(out, &commit->object.oid, edits, flags);
 		pretty_print_commit(&pp, commit, out);
 		strbuf_addch(out, '\n');
 	}
-	return 0;
+	return check_unused_edits(edits);
 }

 /*
diff --git a/sequencer.h b/sequencer.h
index a515ee4457..7887509fea 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -130,6 +130,24 @@ int sequencer_continue(struct repository *repo, struct replay_opts *opts);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);

+/*
+ * The string_lists here contain the edit refs from the command-line. Each
+ * string_list_item's util pointer is pointed to the struct object_id * of
+ * the ref's oid by resolve_oids(), and util is set back to NULL when the
+ * ref is consumed during the todo-list generation.  After the todo-list is
+ * generated, any !!util strings were not encountered.
+ */
+struct sequence_edits {
+	struct commit_list *revs;
+	struct string_list drop;
+	struct string_list edit;
+	struct string_list reword;
+};
+#define SEQUENCE_EDITS_INIT { NULL, STRING_LIST_INIT_NODUP, \
+		STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP }
+void free_sequence_edits(struct sequence_edits *edits);
+
+
 #define TODO_LIST_KEEP_EMPTY (1U << 0)
 #define TODO_LIST_SHORTEN_IDS (1U << 1)
 #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
@@ -143,7 +161,8 @@ int sequencer_remove_state(struct replay_opts *opts);
 #define TODO_LIST_APPEND_TODO_HELP (1U << 5)

 int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
-			  const char **argv, unsigned flags);
+			  const char **argv, const struct sequence_edits *edits,
+			  unsigned flags);

 void todo_list_add_exec_commands(struct todo_list *todo_list,
 				 struct string_list *commands);
--
2.20.1

[PATCH/RFC 2/2] rebase: add --break switch

[Phil Hord]

From: Phil Hord <phil.hord@gmail.com>

Expand the rebase edit switches to include the break switch. This
switch lets the user add a "break" instruction to the todo-list
before the mentioned reference.

This switch is a little different from the ones added so far because
it adds a new instruction between commits in the todo-list instead of
changing the command used to include a commit. It is a little like
"exec" in this regard, except it doesn't add the command after every
commit.

It is not immediately clear whether we should add the break command
before or after the referenced commit.  That is, when the user says
'--break ref', does she mean to break after ref is picked or before
it?  The answer comes when we realize that a 'break' after a ref
is functionally the same as '--edit ref'. Since the user didn't
say '--edit ref', clearly she must have wanted to break _before_ ref
is picked.  So, insert the break before the mentioned ref.

Annoyingly, however, when git stops at a break, it declares that the
previous commit is the one we stopped on, which is always different
from the one the user specified. Does anyone care?  Should --break
effectively be an alias for --edit?

    '--break <ref>' to stop the rebase before the mentioned commit

Signed-off-by: Phil Hord <phil.hord@gmail.com>
---
 builtin/rebase--interactive.c | 3 +++
 builtin/rebase.c              | 4 ++++
 sequencer.c                   | 7 +++++++
 sequencer.h                   | 1 +
 4 files changed, 15 insertions(+)

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 9285d05443..a81fa9c1c5 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -164,6 +164,7 @@ static int resolve_commit_list(const struct string_list *str,
 static int resolve_edits_commit_list(struct sequence_edits *edits)
 {
 	return resolve_commit_list(&edits->drop, &edits->revs) ||
+	       resolve_commit_list(&edits->breaks, &edits->revs) ||
 	       resolve_commit_list(&edits->edit, &edits->revs) ||
 	       resolve_commit_list(&edits->reword, &edits->revs);
 }
@@ -302,6 +303,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 			   N_("restrict-revision"), N_("restrict revision")),
 		OPT_STRING(0, "squash-onto", &squash_onto, N_("squash-onto"),
 			   N_("squash onto")),
+		OPT_STRING_LIST(0, "break", &edits.breaks, N_("revision"),
+				N_("stop before the mentioned ref")),
 		OPT_STRING_LIST(0, "drop", &edits.drop, N_("revision"),
 				N_("drop the mentioned ref from the "
 				   "todo list")),
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a8101630cf..02079c4172 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1053,6 +1053,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 					      NULL };
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
+	struct string_list breaks = STRING_LIST_INIT_NODUP;
 	struct string_list reword = STRING_LIST_INIT_NODUP;
 	struct string_list edit = STRING_LIST_INIT_NODUP;
 	struct string_list drop = STRING_LIST_INIT_NODUP;
@@ -1138,6 +1139,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
 				N_("add exec lines after each commit of the "
 				   "editable list")),
+		OPT_STRING_LIST(0, "break", &breaks, N_("revision"),
+				N_("stop before the mentioned ref")),
 		OPT_STRING_LIST(0, "drop", &drop, N_("revision"),
 				N_("drop the mentioned ref from the "
 				   "todo list")),
@@ -1404,6 +1407,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.cmd = xstrdup(buf.buf);
 	}

+	forward_switches(&options, "--break", &breaks);
 	forward_switches(&options, "--drop", &drop);
 	forward_switches(&options, "--edit", &edit);
 	forward_switches(&options, "--reword", &reword);
diff --git a/sequencer.c b/sequencer.c
index d7384d987c..4a1a371757 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4261,6 +4261,7 @@ static const char *label_oid(struct object_id *oid, const char *label,

 void free_sequence_edits(struct sequence_edits *edits)
 {
+	string_list_clear(&edits->breaks, 0);
 	string_list_clear(&edits->drop, 0);
 	string_list_clear(&edits->edit, 0);
 	string_list_clear(&edits->reword, 0);
@@ -4294,6 +4295,7 @@ static int check_unused_refs(const struct string_list *refs)
 static int check_unused_edits(const struct sequence_edits *edits)
 {
 	return check_unused_refs(&edits->drop) ||
+		check_unused_refs(&edits->breaks) ||
 		check_unused_refs(&edits->edit) ||
 		check_unused_refs(&edits->reword);
 }
@@ -4320,6 +4322,11 @@ static void add_edit_todo_inst(struct strbuf *buf, const struct object_id *oid,
 {
 	enum todo_command cmd = TODO_PICK;

+	if (consume_oid(oid, &edits->breaks)) {
+		add_todo_cmd(buf, TODO_BREAK, flags);
+		strbuf_addstr(buf, "\n");
+	}
+
 	if (consume_oid(oid, &edits->drop))
 		cmd = TODO_DROP;
 	else if (consume_oid(oid, &edits->edit))
diff --git a/sequencer.h b/sequencer.h
index 7887509fea..310829f222 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -139,6 +139,7 @@ int sequencer_remove_state(struct replay_opts *opts);
  */
 struct sequence_edits {
 	struct commit_list *revs;
+	struct string_list breaks;
 	struct string_list drop;
 	struct string_list edit;
 	struct string_list reword;
--
2.20.1

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> Currently it supports these switches:
>
>     usage: git rebase [-i] [options] [--exec <cmd>] ...
>        :
>     --break <revision>    stop before the mentioned ref
>     --drop <revision>     drop the mentioned ref from the todo list
>     --edit <revision>     edit the mentioned ref instead of picking it
>     --reword <revision>   reword the mentioned ref instead of picking it
>
> I have plans to add these, but I don't like how their "onto" will be
> controlled. More thinking is needed here.
>
>     --fixup <revision>    fixup the mentioned ref instead of picking it
>     --squash <revision>   squash the mentioned ref instead of picking it
>     --pick <revision>     pick the mentioned ref onto the start of the list

Yeah, I can see that it may be very useful to shorten the sequence
to (1) learn what commits there are and think what you want to do
with each of them by looking at "git log --oneline master.." output
and then to (2) look at and edit todo in "git rebase -i master".

I personally would be fine without the step (1), as what "rebase -i"
gives me in step (2) essentially is "log --oneline master..".  So I
am not quite getting in what way these command line options would be
more useful than without them, though, especially since I do not see
how well an option to reorder commits would fit with the way you
structured your UI.

Having already said that, if I were to get in the habit of looking
at "log" first to decide and then running "rebase -i" after I made
up my mind, using a tweaked "log --oneline" output that looks
perhaps like this:

	$ git log --oneline master.. | tac | cat -n
	1 xxxxxx prelim cleanly
	2 xxxxxx implement the feature
	3 xxxxxx document and test the feature
	4 xxxxxx the final step
	5 xxxxxx fixup! implement the feature

I think I may appreciate such a feature in "rebase -i" even more, if
the UI were done a bit differently, e.g.

	$ git rebase -i --edit="1 3 2 b f5 b r4" master..

to mean "pick the first (i.e. bottommost) one, pick the third one
for testing, pick the second one, then break so that I can test,
fixup the fifth one, break to test, and finally pick the fourth
one but reword its log message", to come up with:

	pick xxxxxx prelim cleanly
	pick xxxxxx document and test the feature
	pick xxxxxx implement the feature
	break
	fixup xxxxxx oops, the second one needs fixing
        break
	reword xxxxxx the final step

I am guessing that the way you did it, the above would be impossible
(as it requires reordering) but given that you would leave most of
the 'pick's intact and only tweak them in-place into drop, edit,
reword, etc., that may not be too bad, but I suspect that it would
become very verbose.

	$ git rebase -i \
		--pick HEAD~4 --pick HEAD~3 --break --fixup HEAD \
		...

The --edit alternative I threw in in the above would make it
necessary for the user to spell out all the picks, and that would be
more cumbersome given our assumption that most picks will be left
intact, but then we could do something like

	--edit="1-4 5e 6 8-" master..

to say "pick 1 thru 4, edit 5, pick 6, drop 7 and pick 8 thru the
end".

I dunno.

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Phillip Wood]

On 22/04/2019 02:13, Junio C Hamano wrote:
> Phil Hord <phil.hord@gmail.com> writes:
> 
>> Currently it supports these switches:
>>
>>      usage: git rebase [-i] [options] [--exec <cmd>] ...
>>         :
>>      --break <revision>    stop before the mentioned ref
>>      --drop <revision>     drop the mentioned ref from the todo list
>>      --edit <revision>     edit the mentioned ref instead of picking it
>>      --reword <revision>   reword the mentioned ref instead of picking it
>>
>> I have plans to add these, but I don't like how their "onto" will be
>> controlled. More thinking is needed here.
>>
>>      --fixup <revision>    fixup the mentioned ref instead of picking it
>>      --squash <revision>   squash the mentioned ref instead of picking it
>>      --pick <revision>     pick the mentioned ref onto the start of the list
> 
> Yeah, I can see that it may be very useful to shorten the sequence
> to (1) learn what commits there are and think what you want to do
> with each of them by looking at "git log --oneline master.." output
> and then to (2) look at and edit todo in "git rebase -i master".
 >> I personally would be fine without the step (1), as what "rebase -i"
> gives me in step (2) essentially is "log --oneline master..".  So I
> am not quite getting in what way these command line options would be
> more useful than without them, though, especially since I do not see
> how well an option to reorder commits would fit with the way you
> structured your UI.

Doing "git rebase -i master" and then editing the todo list has the side 
effect of rebasing the branch. Often I find I want to amend or reword a 
commit without rebasing (for instance when preparing a re-roll). To do 
this I use a script that runs something like

GIT_SEQUENCE_EDITOR="sed -i s/pick $sha/edit $sha/" git rebase -i $sha^

and I have my shell set up to interactively select a commit[1] so I 
don't have to cut and paste the output from git log. I've found this 
really useful as most of the time I just want to amend or reword a 
commit or squash fixups rather than rearranging commits. The script 
knows how to rewind a running rebase so I can amend several commits 
without having to start a new rebase each time.

So I can see a use for --edit, --reword & --drop if they selected a 
suitable upstream to avoid unwanted rebases (I'm not so sure about the 
others though). If you want to rebase as well then I agree you might as 
well just edit the todo list.

Best Wishes

Phillip

[1] Something like 
https://public-inbox.org/git/87k3xli6mn.fsf@thomas.inf.ethz.ch/

> Having already said that, if I were to get in the habit of looking
> at "log" first to decide and then running "rebase -i" after I made
> up my mind, using a tweaked "log --oneline" output that looks
> perhaps like this:
> 
> 	$ git log --oneline master.. | tac | cat -n
> 	1 xxxxxx prelim cleanly
> 	2 xxxxxx implement the feature
> 	3 xxxxxx document and test the feature
> 	4 xxxxxx the final step
> 	5 xxxxxx fixup! implement the feature
> 
> I think I may appreciate such a feature in "rebase -i" even more, if
> the UI were done a bit differently, e.g.
> 
> 	$ git rebase -i --edit="1 3 2 b f5 b r4" master.. >
> to mean "pick the first (i.e. bottommost) one, pick the third one
> for testing, pick the second one, then break so that I can test,
> fixup the fifth one, break to test, and finally pick the fourth
> one but reword its log message", to come up with:
> 
> 	pick xxxxxx prelim cleanly
> 	pick xxxxxx document and test the feature
> 	pick xxxxxx implement the feature
> 	break
> 	fixup xxxxxx oops, the second one needs fixing
>          break
> 	reword xxxxxx the final step
> 
> I am guessing that the way you did it, the above would be impossible
> (as it requires reordering) but given that you would leave most of
> the 'pick's intact and only tweak them in-place into drop, edit,
> reword, etc., that may not be too bad, but I suspect that it would
> become very verbose.
> 
> 	$ git rebase -i \
> 		--pick HEAD~4 --pick HEAD~3 --break --fixup HEAD \
> 		...
> 
> The --edit alternative I threw in in the above would make it
> necessary for the user to spell out all the picks, and that would be
> more cumbersome given our assumption that most picks will be left
> intact, but then we could do something like
> 
> 	--edit="1-4 5e 6 8-" master..
> 
> to say "pick 1 thru 4, edit 5, pick 6, drop 7 and pick 8 thru the
> end".
> 
> I dunno.
> 

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Phil Hord]

On Sun, Apr 21, 2019 at 6:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phil Hord <phil.hord@gmail.com> writes:
>
> > Currently it supports these switches:
> >
> >     usage: git rebase [-i] [options] [--exec <cmd>] ...
> >        :
> >     --break <revision>    stop before the mentioned ref
> >     --drop <revision>     drop the mentioned ref from the todo list
> >     --edit <revision>     edit the mentioned ref instead of picking it
> >     --reword <revision>   reword the mentioned ref instead of picking it
> >
> > I have plans to add these, but I don't like how their "onto" will be
> > controlled. More thinking is needed here.
> >
> >     --fixup <revision>    fixup the mentioned ref instead of picking it
> >     --squash <revision>   squash the mentioned ref instead of picking it
> >     --pick <revision>     pick the mentioned ref onto the start of the list
>
> Yeah, I can see that it may be very useful to shorten the sequence
> to (1) learn what commits there are and think what you want to do
> with each of them by looking at "git log --oneline master.." output
> and then to (2) look at and edit todo in "git rebase -i master".
>
> I personally would be fine without the step (1), as what "rebase -i"
> gives me in step (2) essentially is "log --oneline master..".

My example of "drop" is probably the worst one for simplification, but
it nicely reduces the operation to one step if --interactive is not
given.  More commonly I discover something I want to improve in commit
message or in the code so I use --edit or --reword to fix it up before
I submit it for review.

> So I
> am not quite getting in what way these command line options would be
> more useful than without them, though, especially since I do not see
> how well an option to reorder commits would fit with the way you
> structured your UI.

I thought I might have "--pick-onto foo bar" but that requires two
arguments to one switch, which I think is confusing and unprecedented.
My current thinking is that --pick could pick some commit onto the
beginning of the todo list, thereby picking it onto 'upstream'.  If
the same commit appears later in the todo-list, I am inclined to drop
it; but I might want to pick it anyway and let it evaporate as an
empty commit, giving me the opportunity to split a commit in two
places.  But this is still more exotic and confusing.

I tried to have --fixup and --squash simply do their actions in-place,
but that seems useless.  So I thought I might treat the same as pick,
picking them onto upstream. But it's meaningless to fixup or squash on
the first step in the todo, so it would have to be on the first child
of the upstream.  This still feels forced and useless.

A compromise that feels nice in practice is to do the fixup and squash
in-place and then to use --interactive to open the editor.  Since I
have syntax highlighting, the fixup and squash lines stand out boldly
and I find it easier to move these into the right place as needed. But
I think this mode could be confusing for users trying to understand
the utility of these switches.

> Having already said that, if I were to get in the habit of looking
> at "log" first to decide and then running "rebase -i" after I made
> up my mind, using a tweaked "log --oneline" output that looks
> perhaps like this:
>
>         $ git log --oneline master.. | tac | cat -n
>         1 xxxxxx prelim cleanly
>         2 xxxxxx implement the feature
>         3 xxxxxx document and test the feature
>         4 xxxxxx the final step
>         5 xxxxxx fixup! implement the feature
>
> I think I may appreciate such a feature in "rebase -i" even more, if
> the UI were done a bit differently, e.g.
>
>         $ git rebase -i --edit="1 3 2 b f5 b r4" master..
>
> to mean "pick the first (i.e. bottommost) one, pick the third one
> for testing, pick the second one, then break so that I can test,
> fixup the fifth one, break to test, and finally pick the fourth
> one but reword its log message", to come up with:
>
>         pick xxxxxx prelim cleanly
>         pick xxxxxx document and test the feature
>         pick xxxxxx implement the feature
>         break
>         fixup xxxxxx oops, the second one needs fixing
>         break
>         reword xxxxxx the final step

This kind of gui brings more power and flexibility. I don't think I
would use it since reordering in the editor feels right to me.  Maybe
that's the real problem with reordering at all with these switches,
and I should leave fixup/squash/pick out for good.

> I am guessing that the way you did it, the above would be impossible
> (as it requires reordering) but given that you would leave most of
> the 'pick's intact and only tweak them in-place into drop, edit,
> reword, etc., that may not be too bad, but I suspect that it would
> become very verbose.
>
>         $ git rebase -i \
>                 --pick HEAD~4 --pick HEAD~3 --break --fixup HEAD \
>                 ...
>
> The --edit alternative I threw in in the above would make it
> necessary for the user to spell out all the picks, and that would be
> more cumbersome given our assumption that most picks will be left
> intact, but then we could do something like
>
>         --edit="1-4 5e 6 8-" master..
>
> to say "pick 1 thru 4, edit 5, pick 6, drop 7 and pick 8 thru the
> end".

My own itch is to help the case when I am leaving most of the lines as
picks.  That is, I don't really want to rebase to some new upstream
commit; I don't want to enumerate all the changes that
rebase--interactive helpfully chooses for me; I only want to fix one
or two warts.

As a trivial example, I can say

        $ git commit --amend

to reword my HEAD commit.  But I have no easy way to say that for
HEAD^.  With this change I can.

        $ git rebase --edit HEAD^ HEAD^^

Ideally I wouldn't need to specify HEAD^^.  I'm thinking of a switch
to say "use the mergebase of my mentioned edits", but that's still a
wip.

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Phil Hord]

On Mon, Apr 22, 2019 at 7:44 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> Doing "git rebase -i master" and then editing the todo list has the side
> effect of rebasing the branch. Often I find I want to amend or reword a
> commit without rebasing (for instance when preparing a re-roll). To do
> this I use a script that runs something like
>
> GIT_SEQUENCE_EDITOR="sed -i s/pick $sha/edit $sha/" git rebase -i $sha^
>
> and I have my shell set up to interactively select a commit[1] so I
> don't have to cut and paste the output from git log. I've found this
> really useful as most of the time I just want to amend or reword a
> commit or squash fixups rather than rearranging commits. The script
> knows how to rewind a running rebase so I can amend several commits
> without having to start a new rebase each time.
>
> So I can see a use for --edit, --reword & --drop if they selected a
> suitable upstream to avoid unwanted rebases (I'm not so sure about the
> others though). If you want to rebase as well then I agree you might as
> well just edit the todo list.

I have the same need.  I plan to have some switch that invokes this
"in-place rebase" behavior so that git can choose the upstream for me
as `mergebase $sequence-edits`.  In fact, I want to make that the
default for these switches, but that feels too surprising for the
rebase command. I plan to progress like this:

    # --in-place switch is not supported; manual upstream is given by user
    git rebase --edit foo foo^

     # --in-place switch is added; now we can say this
     git rebase --edit foo --in-place

     # prefer in-place edits as default when editing
     git config --add rebase.in-place-edits true
     git rebase --edit foo

This --in-place switch would use `mergebase $sequence-edits` to find
my upstream parameter if I didn't give one explicitly.

This config option set to true would tell git to assume I meant to use
--in-place whenever I use some sequence-edit switch and I don't
specify an upstream.

I have written some of this code, but since I am running into
conflicts with next and pu, I haven't ironed it out yet.

Phil

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Phil Hord]

On Mon, Apr 22, 2019 at 12:16 PM Phil Hord <phil.hord@gmail.com> wrote:
>
> I have the same need.  I plan to have some switch that invokes this
> "in-place rebase" behavior so that git can choose the upstream for me
> as `mergebase $sequence-edits`.  In fact, I want to make that the
> default for these switches, but that feels too surprising for the
> rebase command. I plan to progress like this:
>
>     # --in-place switch is not supported; manual upstream is given by user
>     git rebase --edit foo foo^
>
>      # --in-place switch is added; now we can say this
>      git rebase --edit foo --in-place

I originally CC'ed Denton on this thread because he recently added
--keep-base.  I initially hoped it would do something similar to
--in-place, but on reading the patch discussion, I think it's for
something different altogether.  :-\   It's similar, though, in the
same way that --fork-point is; which may be another way to say "not
very."

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Denton Liu]

Hi Phil,

On Mon, Apr 22, 2019 at 12:20:29PM -0700, Phil Hord wrote:
> On Mon, Apr 22, 2019 at 12:16 PM Phil Hord <phil.hord@gmail.com> wrote:
> >
> > I have the same need.  I plan to have some switch that invokes this
> > "in-place rebase" behavior so that git can choose the upstream for me
> > as `mergebase $sequence-edits`.  In fact, I want to make that the
> > default for these switches, but that feels too surprising for the
> > rebase command. I plan to progress like this:
> >
> >     # --in-place switch is not supported; manual upstream is given by user
> >     git rebase --edit foo foo^
> >
> >      # --in-place switch is added; now we can say this
> >      git rebase --edit foo --in-place
> 
> I originally CC'ed Denton on this thread because he recently added
> --keep-base.  I initially hoped it would do something similar to
> --in-place, but on reading the patch discussion, I think it's for
> something different altogether.  :-\   It's similar, though, in the
> same way that --fork-point is; which may be another way to say "not
> very."

You're correct, --keep-base is a little more explicit than your proposed
--in-place switch in that the former requires an upstream revision be
specified whereas yours implicitly finds the base using the
$sequence-edits. I suppose until --in-place is implemented, users could
always use explicitly specify the upstream branch, such as:

	$ git rebase --edit foo --keep-base master

Anyway, I've been following along with the discussion and although there
are kinks to iron out, I like the general idea. Although I use fixup and
squash commits + rebase -i --keep-base for major branch polishing,
sometimes after the branch is mostly polished, there are a few
last-minute changes to be made. I think that your proposed solution
would also match my use-case nicely.

Thanks,

Denton

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Doing "git rebase -i master" and then editing the todo list has the
> side effect of rebasing the branch. Often I find I want to amend or
> reword a commit without rebasing (for instance when preparing a
> re-roll).

I am not sure what you mean by "not rebasing".  Are you talking
about --keep-base that uses the same --onto as the previous?

I think that is often desired, but I do not think it has much to do
with the topic of the proposal these two patches raises.

And that (i.e. "this has nothing to do with the choice of 'onto'")
was why I used the casual "rebase -i master" in my illustrations.

Re: [PATCH/RFC 0/2] rebase: add switches to control todo-list setup

[Phil Hord]

On Mon, Apr 22, 2019 at 6:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Doing "git rebase -i master" and then editing the todo list has the
> > side effect of rebasing the branch. Often I find I want to amend or
> > reword a commit without rebasing (for instance when preparing a
> > re-roll).
>
> I am not sure what you mean by "not rebasing".  Are you talking
> about --keep-base that uses the same --onto as the previous?
>
> I think that is often desired, but I do not think it has much to do
> with the topic of the proposal these two patches raises.
>
> And that (i.e. "this has nothing to do with the choice of 'onto'")
> was why I used the casual "rebase -i master" in my illustrations.

I know exactly what he means, because it usually is exactly what I
want to do here.  In fact, I almost always want `rebase --interactive`
to do this "in-place" editing of the history.  Sure, I may want to
`rebase @{upstream}` someday, but I seldom use --interactive for that.

Rebase invites conflicts.  It's nice to invite as few as possible at once.

P